[skip-changleog] Better gRPC error handling (#1251)

* Proper gRPC error handling

* Update gRPC API of board command to return status.Status instead of error

* Update gRPC API of remaining commands to return status.Status instead of error

* Replace custom error with protobuf message

Previously, a custom error was returned when attempting to upgrade a platform that was already at the latest available
version. There is dedicated code for handling this specific error.

Now that the function has been changed to return a status.Status instead of error, the previous check for the return
being this error is no longer possible. The capability is restored by replacing the error with a protocol buffer message.

* Handle details of any type in `core.PlatformUpgrade()` status

The status details of the function are used to identify the specific cause of a non-nil status. This is done via a type
assertion. Previously, this type assertion was configured such that a details of any type other than the expected would
result in a panic. At the moment, that will not occur because we only add details of one type. However, the whole point
of the type assertion is to support details of multiple types, and if other types are added a panic will not be the
appropriate behavior.

A better approach is to check the result of the type assertion, handling the non-nil status as a generic error if its
details are of a different type.

* Return nil on program action if an error occurred

* Refactoring 'upload' commands

* Refactoring 'board' commands

* Refactoring 'compile' commands

* Refactoring 'core' commands

* Refactoring 'debug' commands

* Refactoring 'lib' commands

* Refactoring 'sketch' commands

* Refactoring 'commands' commands

* updated tests and fixed some error wording

* fixed go lint warnings

* Apply suggestions from code review

Co-authored-by: per1234 <[email protected]>

* Apply changes from code review

Co-authored-by: Silvano Cerza <[email protected]>

* fix i18n

Co-authored-by: per1234 <[email protected]>
Co-authored-by: Silvano Cerza <[email protected]>

Author: 3 people

arduino/cores/fqbn.go

@@ -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{
@@ -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)
 		}

arduino/libraries/libraries.go

@@ -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)
 		}
 	}
 

cli/compile/compile.go

@@ -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)
@@ -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)
 	}
 }

cli/core/upgrade.go

@@ -17,6 +17,7 @@ package core
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 
@@ -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"
@@ -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)
 		}
 	}

cli/debug/debug.go

@@ -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 (
@@ -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})
 		}

cli/errorcodes/errorcodes.go

@@ -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
@@ -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)
-	}
-}

cli/instance/instance.go

@@ -17,6 +17,7 @@ package instance
 
 import (
 	"context"
+	"errors"
 	"os"
 
 	"github.com/arduino/arduino-cli/cli/errorcodes"
@@ -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
@@ -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
@@ -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 {
@@ -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 {
@@ -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"))
 
@@ -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
 		}
 	}
 
@@ -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
 		}
 	}
 

cli/lib/args.go

@@ -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)
 	res, err := lib.LibrarySearch(context.Background(), &rpc.LibrarySearchRequest{
 		Instance: instance,
 		Query:    libRef.Name,

cli/lib/check_deps.go

@@ -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})

cli/lib/search.go

@@ -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)
 	}

commands/board/attach.go

@@ -17,7 +17,6 @@ package board
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"net/url"
 	"strings"
@@ -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()
@@ -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
@@ -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())
@@ -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())})
 
@@ -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