Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better gRPC error handling #1251

Merged
merged 19 commits into from
Aug 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions arduino/cores/fqbn.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func ParseFQBN(fqbnIn string) (*FQBN, error) {
// Split fqbn
fqbnParts := strings.Split(fqbnIn, ":")
if len(fqbnParts) < 3 || len(fqbnParts) > 4 {
return nil, fmt.Errorf("invalid fqbn: %s", fqbnIn)
return nil, fmt.Errorf("not an FQBN: %s", fqbnIn)
}

fqbn := &FQBN{
Expand All @@ -45,18 +45,18 @@ func ParseFQBN(fqbnIn string) (*FQBN, error) {
Configs: properties.NewMap(),
}
if fqbn.BoardID == "" {
return nil, fmt.Errorf(tr("invalid fqbn: empty board identifier"))
return nil, fmt.Errorf(tr("empty board identifier"))
}
if len(fqbnParts) > 3 {
for _, pair := range strings.Split(fqbnParts[3], ",") {
parts := strings.SplitN(pair, "=", 2)
if len(parts) != 2 {
return nil, fmt.Errorf(tr("invalid fqbn config: %s"), pair)
return nil, fmt.Errorf(tr("invalid config option: %s"), pair)
}
k := strings.TrimSpace(parts[0])
v := strings.TrimSpace(parts[1])
if k == "" {
return nil, fmt.Errorf(tr("invalid fqbn config: %s"), pair)
return nil, fmt.Errorf(tr("invalid config option: %s"), pair)
}
fqbn.Configs.Set(k, v)
}
Expand Down
2 changes: 1 addition & 1 deletion arduino/libraries/libraries.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func (library *Library) ToRPCLibrary() (*rpc.Library, error) {
var err error
headers, err = library.SourceHeaders()
if err != nil {
return nil, fmt.Errorf(tr("gathering library headers: %w"), err)
return nil, fmt.Errorf(tr("reading library headers: %w"), err)
}
}

Expand Down
38 changes: 20 additions & 18 deletions cli/compile/compile.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,20 +178,20 @@ func run(cmd *cobra.Command, args []string) {
SourceOverride: overrides,
Library: library,
}
compileOut := new(bytes.Buffer)
compileErr := new(bytes.Buffer)
compileStdOut := new(bytes.Buffer)
compileStdErr := new(bytes.Buffer)
verboseCompile := configuration.Settings.GetString("logging.level") == "debug"
var compileRes *rpc.CompileResponse
var err error
var compileError error
if output.OutputFormat == "json" {
compileRes, err = compile.Compile(context.Background(), compileRequest, compileOut, compileErr, verboseCompile)
compileRes, compileError = compile.Compile(context.Background(), compileRequest, compileStdOut, compileStdErr, verboseCompile)
} else {
compileRes, err = compile.Compile(context.Background(), compileRequest, os.Stdout, os.Stderr, verboseCompile)
compileRes, compileError = compile.Compile(context.Background(), compileRequest, os.Stdout, os.Stderr, verboseCompile)
}

if err == nil && uploadAfterCompile {
if compileError == nil && uploadAfterCompile {
var sk *sketch.Sketch
sk, err = sketch.New(sketchPath)
sk, err := sketch.New(sketchPath)
if err != nil {
feedback.Errorf(tr("Error during Upload: %v"), err)
os.Exit(errorcodes.ErrGeneric)
Expand Down Expand Up @@ -230,28 +230,30 @@ func run(cmd *cobra.Command, args []string) {
Programmer: programmer,
UserFields: fields,
}

var uploadError error
if output.OutputFormat == "json" {
// TODO: do not print upload output in json mode
uploadOut := new(bytes.Buffer)
uploadErr := new(bytes.Buffer)
_, err = upload.Upload(context.Background(), uploadRequest, uploadOut, uploadErr)
uploadStdOut := new(bytes.Buffer)
uploadStdErr := new(bytes.Buffer)
_, uploadError = upload.Upload(context.Background(), uploadRequest, uploadStdOut, uploadStdErr)
} else {
_, err = upload.Upload(context.Background(), uploadRequest, os.Stdout, os.Stderr)
_, uploadError = upload.Upload(context.Background(), uploadRequest, os.Stdout, os.Stderr)
}
if err != nil {
feedback.Errorf(tr("Error during Upload: %v"), err)
if uploadError != nil {
feedback.Errorf(tr("Error during Upload: %v"), uploadError)
os.Exit(errorcodes.ErrGeneric)
}
}

feedback.PrintResult(&compileResult{
CompileOut: compileOut.String(),
CompileErr: compileErr.String(),
CompileOut: compileStdOut.String(),
CompileErr: compileStdErr.String(),
BuilderResult: compileRes,
Success: err == nil,
Success: compileError == nil,
})
if err != nil && output.OutputFormat != "json" {
feedback.Errorf(tr("Error during build: %v"), err)
if compileError != nil && output.OutputFormat != "json" {
feedback.Errorf(tr("Error during build: %v"), compileError)
os.Exit(errorcodes.ErrGeneric)
}
}
Expand Down
14 changes: 9 additions & 5 deletions cli/core/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package core

import (
"context"
"errors"
"fmt"
"os"

Expand All @@ -25,6 +26,7 @@ import (
"github.com/arduino/arduino-cli/cli/feedback"
"github.com/arduino/arduino-cli/cli/instance"
"github.com/arduino/arduino-cli/cli/output"
"github.com/arduino/arduino-cli/commands"
"github.com/arduino/arduino-cli/commands/core"
rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -94,11 +96,13 @@ func runUpgradeCommand(cmd *cobra.Command, args []string) {
SkipPostInstall: DetectSkipPostInstallValue(),
}

_, err := core.PlatformUpgrade(context.Background(), r, output.ProgressBar(), output.TaskProgress())
if err == core.ErrAlreadyLatest {
feedback.Printf(tr("Platform %s is already at the latest version"), platformRef)
} else if err != nil {
feedback.Errorf(tr("Error during upgrade: %v"), err)
if _, err := core.PlatformUpgrade(context.Background(), r, output.ProgressBar(), output.TaskProgress()); err != nil {
if errors.Is(err, &commands.PlatformAlreadyAtTheLatestVersionError{}) {
feedback.Print(err.Error())
continue
}

feedback.Errorf(tr("Error during upgrade: %v", err))
os.Exit(errorcodes.ErrGeneric)
}
}
Expand Down
7 changes: 1 addition & 6 deletions cli/debug/debug.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import (
"github.com/arduino/go-properties-orderedmap"
"github.com/fatih/color"
"github.com/spf13/cobra"
"google.golang.org/grpc/status"
)

var (
Expand Down Expand Up @@ -101,12 +100,8 @@ func run(command *cobra.Command, args []string) {
if printInfo {

if res, err := debug.GetDebugConfig(context.Background(), debugConfigRequested); err != nil {
if status, ok := status.FromError(err); ok {
feedback.Errorf(tr("Error getting Debug info: %v"), status.Message())
errorcodes.ExitWithGrpcStatus(status)
}
feedback.Errorf(tr("Error getting Debug info: %v"), err)
os.Exit(errorcodes.ErrGeneric)
os.Exit(errorcodes.ErrBadArgument)
} else {
feedback.PrintResult(&debugInfoResult{res})
}
Expand Down
18 changes: 0 additions & 18 deletions cli/errorcodes/errorcodes.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,6 @@

package errorcodes

import (
"os"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)

// Error codes to be used for os.Exit().
const (
_ = iota // 0 is not a valid exit error code
Expand All @@ -36,14 +29,3 @@ const (
ErrCoreConfig
ErrBadArgument
)

// ExitWithGrpcStatus will terminate the current process by returing the correct
// error code closest matching the gRPC status.
func ExitWithGrpcStatus(s *status.Status) {
switch s.Code() {
case codes.Unimplemented:
os.Exit(ErrBadArgument)
default:
os.Exit(ErrGeneric)
}
}
21 changes: 10 additions & 11 deletions cli/instance/instance.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package instance

import (
"context"
"errors"
"os"

"github.com/arduino/arduino-cli/cli/errorcodes"
Expand All @@ -27,8 +28,6 @@ import (
"github.com/arduino/arduino-cli/i18n"
rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
"github.com/arduino/go-paths-helper"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)

var tr = i18n.Tr
Expand All @@ -50,7 +49,7 @@ func CreateAndInit() *rpc.Instance {
}

// Create and return a new Instance.
func Create() (*rpc.Instance, *status.Status) {
func Create() (*rpc.Instance, error) {
res, err := commands.Create(&rpc.CreateRequest{})
if err != nil {
return nil, err
Expand All @@ -59,12 +58,12 @@ func Create() (*rpc.Instance, *status.Status) {
}

// Init initializes instance by loading installed libraries and platforms.
// In case of loading failures return a list gRPC Status errors for each
// In case of loading failures return a list of errors for each
// platform or library that we failed to load.
// Package and library indexes files are automatically updated if the
// CLI is run for the first time.
func Init(instance *rpc.Instance) []*status.Status {
errs := []*status.Status{}
func Init(instance *rpc.Instance) []error {
errs := []error{}

// In case the CLI is executed for the first time
if err := FirstUpdate(instance); err != nil {
Expand All @@ -77,8 +76,8 @@ func Init(instance *rpc.Instance) []*status.Status {
err := commands.Init(&rpc.InitRequest{
Instance: instance,
}, func(res *rpc.InitResponse) {
if err := res.GetError(); err != nil {
errs = append(errs, status.FromProto(err))
if st := res.GetError(); st != nil {
errs = append(errs, errors.New(st.Message))
}

if progress := res.GetInitProgress(); progress != nil {
Expand All @@ -99,7 +98,7 @@ func Init(instance *rpc.Instance) []*status.Status {

// FirstUpdate downloads libraries and packages indexes if they don't exist.
// This ideally is only executed the first time the CLI is run.
func FirstUpdate(instance *rpc.Instance) *status.Status {
func FirstUpdate(instance *rpc.Instance) error {
// Gets the data directory to verify if library_index.json and package_index.json exist
dataDir := paths.New(configuration.Settings.GetString("directories.data"))

Expand All @@ -120,7 +119,7 @@ func FirstUpdate(instance *rpc.Instance) *status.Status {
output.ProgressBar(),
)
if err != nil {
return status.Newf(codes.FailedPrecondition, err.Error())
return err
}
}

Expand All @@ -135,7 +134,7 @@ func FirstUpdate(instance *rpc.Instance) *status.Status {
output.ProgressBar(),
)
if err != nil {
return status.Newf(codes.FailedPrecondition, err.Error())
return err
}
}

Expand Down
2 changes: 1 addition & 1 deletion cli/lib/args.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func ParseLibraryReferenceArgs(args []string) ([]*LibraryReferenceArg, error) {
// ParseLibraryReferenceArgAndAdjustCase parse a command line argument that reference a
// library and possibly adjust the case of the name to match a library in the index
func ParseLibraryReferenceArgAndAdjustCase(instance *rpc.Instance, arg string) (*LibraryReferenceArg, error) {
libRef, err := ParseLibraryReferenceArg(arg)
libRef, _ := ParseLibraryReferenceArg(arg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we fix this by handling this err instead of ignoring it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably already clear, but I did this because this assignment of err was ignored and the error type from the declaration was incompatible with the change of lib.LibrarySearch()'s return type from error to *status.Status.

If it's to be ignored, I think it's best to make that explicit by using _. But if an error return from ParseLibraryReferenceArg() is significant in this context, then of course it's better to handle it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the err was ignored also before this PR (it was overwritten without being checked). This should be fixed in another PR BTW.

res, err := lib.LibrarySearch(context.Background(), &rpc.LibrarySearchRequest{
Instance: instance,
Query: libRef.Name,
Expand Down
2 changes: 1 addition & 1 deletion cli/lib/check_deps.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func runDepsCommand(cmd *cobra.Command, args []string) {
Version: libRef.Version,
})
if err != nil {
feedback.Errorf(tr("Error resolving dependencies for %[1]s: %[2]s"), libRef, err)
feedback.Errorf(tr("Error resolving dependencies for %[1]s: %[2]s", libRef, err))
}

feedback.PrintResult(&checkDepResult{deps: deps})
Expand Down
9 changes: 5 additions & 4 deletions cli/lib/search.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,11 @@ func runSearchCommand(cmd *cobra.Command, args []string) {
os.Exit(errorcodes.ErrGeneric)
}

err := commands.UpdateLibrariesIndex(context.Background(), &rpc.UpdateLibrariesIndexRequest{
Instance: inst,
}, output.ProgressBar())
if err != nil {
if err := commands.UpdateLibrariesIndex(
context.Background(),
&rpc.UpdateLibrariesIndexRequest{Instance: inst},
output.ProgressBar(),
); err != nil {
feedback.Errorf(tr("Error updating library index: %v"), err)
os.Exit(errorcodes.ErrGeneric)
}
Expand Down
13 changes: 6 additions & 7 deletions commands/board/attach.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package board

import (
"context"
"errors"
"fmt"
"net/url"
"strings"
Expand All @@ -39,15 +38,15 @@ var tr = i18n.Tr
func Attach(ctx context.Context, req *rpc.BoardAttachRequest, taskCB commands.TaskProgressCB) (*rpc.BoardAttachResponse, error) {
pm := commands.GetPackageManager(req.GetInstance().GetId())
if pm == nil {
return nil, errors.New(tr("invalid instance"))
return nil, &commands.InvalidInstanceError{}
}
var sketchPath *paths.Path
if req.GetSketchPath() != "" {
sketchPath = paths.New(req.GetSketchPath())
}
sk, err := sketch.New(sketchPath)
if err != nil {
return nil, fmt.Errorf(tr("opening sketch: %s"), err)
return nil, &commands.CantOpenSketchError{Cause: err}
}

boardURI := req.GetBoardUri()
Expand All @@ -63,7 +62,7 @@ func Attach(ctx context.Context, req *rpc.BoardAttachRequest, taskCB commands.Ta
} else {
deviceURI, err := url.Parse(boardURI)
if err != nil {
return nil, fmt.Errorf(tr("invalid Device URL format: %s"), err)
return nil, &commands.InvalidArgumentError{Message: tr("Invalid Device URL format"), Cause: err}
}

var findBoardFunc func(*packagemanager.PackageManager, *discovery.Monitor, *url.URL) *cores.Board
Expand All @@ -73,7 +72,7 @@ func Attach(ctx context.Context, req *rpc.BoardAttachRequest, taskCB commands.Ta
case "http", "https", "tcp", "udp":
findBoardFunc = findNetworkConnectedBoard
default:
return nil, fmt.Errorf(tr("invalid device port type provided"))
return nil, &commands.InvalidArgumentError{Message: tr("Invalid device port type provided")}
}

duration, err := time.ParseDuration(req.GetSearchTimeout())
Expand All @@ -89,7 +88,7 @@ func Attach(ctx context.Context, req *rpc.BoardAttachRequest, taskCB commands.Ta
// TODO: Handle the case when no board is found.
board := findBoardFunc(pm, monitor, deviceURI)
if board == nil {
return nil, fmt.Errorf(tr("no supported board found at %s"), deviceURI.String())
return nil, &commands.InvalidArgumentError{Message: tr("No supported board found at %s", deviceURI)}
}
taskCB(&rpc.TaskProgress{Name: fmt.Sprintf(tr("Board found: %s"), board.Name())})

Expand All @@ -104,7 +103,7 @@ func Attach(ctx context.Context, req *rpc.BoardAttachRequest, taskCB commands.Ta

err = sk.ExportMetadata()
if err != nil {
return nil, fmt.Errorf(tr("cannot export sketch metadata: %s"), err)
return nil, &commands.PermissionDeniedError{Message: tr("Cannot export sketch metadata"), Cause: err}
}
taskCB(&rpc.TaskProgress{Name: fmt.Sprintf(tr("Selected fqbn: %s"), sk.Metadata.CPU.Fqbn), Completed: true})
return &rpc.BoardAttachResponse{}, nil
Expand Down
Loading