Proper gRPC error handling

Author: cmaglie

cli/compile/compile.go

@@ -27,6 +27,7 @@ import (
 	"github.com/arduino/arduino-cli/cli/output"
 	"github.com/arduino/arduino-cli/configuration"
 	"github.com/arduino/arduino-cli/i18n"
+	"google.golang.org/grpc/status"
 
 	"github.com/arduino/arduino-cli/cli/errorcodes"
 	"github.com/arduino/arduino-cli/cli/instance"
@@ -200,7 +201,7 @@ func run(cmd *cobra.Command, args []string) {
 			ImportDir:  buildPath,
 			Programmer: programmer,
 		}
-		var err error
+		var err *status.Status
 		if output.OutputFormat == "json" {
 			// TODO: do not print upload output in json mode
 			uploadOut := new(bytes.Buffer)

client_example/main.go

@@ -32,6 +32,7 @@ import (
 	dbg "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/debug/v1"
 	"github.com/arduino/arduino-cli/rpc/cc/arduino/cli/settings/v1"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/status"
 )
 
 var (

commands/daemon/daemon.go

@@ -317,7 +317,7 @@ func (s *ArduinoCoreServerImpl) Upload(req *rpc.UploadRequest, stream rpc.Arduin
 		utils.FeedStreamTo(func(data []byte) { stream.Send(&rpc.UploadResponse{ErrStream: data}) }),
 	)
 	if err != nil {
-		return err
+		return err.Err()
 	}
 	return stream.Send(resp)
 }
@@ -330,7 +330,7 @@ func (s *ArduinoCoreServerImpl) UploadUsingProgrammer(req *rpc.UploadUsingProgra
 		utils.FeedStreamTo(func(data []byte) { stream.Send(&rpc.UploadUsingProgrammerResponse{ErrStream: data}) }),
 	)
 	if err != nil {
-		return err
+		return err.Err()
 	}
 	return stream.Send(resp)
 }
@@ -343,7 +343,7 @@ func (s *ArduinoCoreServerImpl) BurnBootloader(req *rpc.BurnBootloaderRequest, s
 		utils.FeedStreamTo(func(data []byte) { stream.Send(&rpc.BurnBootloaderResponse{ErrStream: data}) }),
 	)
 	if err != nil {
-		return err
+		return err.Err()
 	}
 	return stream.Send(resp)
 }

commands/upload/burnbootloader.go

@@ -22,10 +22,11 @@ import (
 	"github.com/arduino/arduino-cli/commands"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
 	"github.com/sirupsen/logrus"
+	"google.golang.org/grpc/status"
 )
 
 // BurnBootloader FIXMEDOC
-func BurnBootloader(ctx context.Context, req *rpc.BurnBootloaderRequest, outStream io.Writer, errStream io.Writer) (*rpc.BurnBootloaderResponse, error) {
+func BurnBootloader(ctx context.Context, req *rpc.BurnBootloaderRequest, outStream io.Writer, errStream io.Writer) (*rpc.BurnBootloaderResponse, *status.Status) {
 	logrus.
 		WithField("fqbn", req.GetFqbn()).
 		WithField("port", req.GetPort()).

commands/upload/upload.go

@@ -36,25 +36,27 @@ import (
 	properties "github.com/arduino/go-properties-orderedmap"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 var tr = i18n.Tr
 
 // Upload FIXMEDOC
-func Upload(ctx context.Context, req *rpc.UploadRequest, outStream io.Writer, errStream io.Writer) (*rpc.UploadResponse, error) {
+func Upload(ctx context.Context, req *rpc.UploadRequest, outStream io.Writer, errStream io.Writer) (*rpc.UploadResponse, *status.Status) {
 	logrus.Tracef("Upload %s on %s started", req.GetSketchPath(), req.GetFqbn())
 
 	// TODO: make a generic function to extract sketch from request
 	// and remove duplication in commands/compile.go
 	sketchPath := paths.New(req.GetSketchPath())
 	sk, err := sketch.New(sketchPath)
 	if err != nil && req.GetImportDir() == "" && req.GetImportFile() == "" {
-		return nil, fmt.Errorf(tr("opening sketch: %s"), err)
+		return nil, status.Newf(codes.InvalidArgument, tr("Sketch not found: %s"), err)
 	}
 
 	pm := commands.GetPackageManager(req.GetInstance().GetId())
 
-	err = runProgramAction(
+	return &rpc.UploadResponse{}, runProgramAction(
 		pm,
 		sk,
 		req.GetImportFile(),
@@ -69,18 +71,14 @@ func Upload(ctx context.Context, req *rpc.UploadRequest, outStream io.Writer, er
 		errStream,
 		req.GetDryRun(),
 	)
-	if err != nil {
-		return nil, err
-	}
-	return &rpc.UploadResponse{}, nil
 }
 
 // UsingProgrammer FIXMEDOC
-func UsingProgrammer(ctx context.Context, req *rpc.UploadUsingProgrammerRequest, outStream io.Writer, errStream io.Writer) (*rpc.UploadUsingProgrammerResponse, error) {
+func UsingProgrammer(ctx context.Context, req *rpc.UploadUsingProgrammerRequest, outStream io.Writer, errStream io.Writer) (*rpc.UploadUsingProgrammerResponse, *status.Status) {
 	logrus.Tracef("Upload using programmer %s on %s started", req.GetSketchPath(), req.GetFqbn())
 
 	if req.GetProgrammer() == "" {
-		return nil, errors.New(tr("programmer not specified"))
+		return nil, status.New(codes.InvalidArgument, tr("Programmer not specified"))
 	}
 	_, err := Upload(ctx, &rpc.UploadRequest{
 		Instance:   req.GetInstance(),
@@ -102,17 +100,17 @@ func runProgramAction(pm *packagemanager.PackageManager,
 	programmerID string,
 	verbose, verify, burnBootloader bool,
 	outStream, errStream io.Writer,
-	dryRun bool) error {
+	dryRun bool) *status.Status {
 
 	if burnBootloader && programmerID == "" {
-		return fmt.Errorf(tr("no programmer specified for burning bootloader"))
+		return status.New(codes.InvalidArgument, tr("No programmer specified for burning bootloader"))
 	}
 
 	// FIXME: make a specification on how a port is specified via command line
 	if port == "" && sk != nil && sk.Metadata != nil {
 		deviceURI, err := url.Parse(sk.Metadata.CPU.Port)
 		if err != nil {
-			return fmt.Errorf(tr("invalid Device URL format: %s"), err)
+			return status.Newf(codes.InvalidArgument, tr("Invalid Device URL format: %s"), err)
 		}
 		if deviceURI.Scheme == "serial" {
 			port = deviceURI.Host + deviceURI.Path
@@ -124,18 +122,18 @@ func runProgramAction(pm *packagemanager.PackageManager,
 		fqbnIn = sk.Metadata.CPU.Fqbn
 	}
 	if fqbnIn == "" {
-		return fmt.Errorf(tr("no Fully Qualified Board Name provided"))
+		return status.New(codes.InvalidArgument, tr("No FQBN (Fully Qualified Board Name) provided"))
 	}
 	fqbn, err := cores.ParseFQBN(fqbnIn)
 	if err != nil {
-		return fmt.Errorf(tr("incorrect FQBN: %s"), err)
+		return status.Newf(codes.InvalidArgument, fmt.Sprintf(tr("Invalid FQBN: %s"), err))
 	}
 	logrus.WithField("fqbn", fqbn).Tracef("Detected FQBN")
 
 	// Find target board and board properties
 	_, boardPlatform, board, boardProperties, buildPlatform, err := pm.ResolveFQBN(fqbn)
 	if err != nil {
-		return fmt.Errorf(tr("incorrect FQBN: %s"), err)
+		return status.Newf(codes.InvalidArgument, tr("Could not resolve FQBN: %s"), err)
 	}
 	logrus.
 		WithField("boardPlatform", boardPlatform).
@@ -152,7 +150,7 @@ func runProgramAction(pm *packagemanager.PackageManager,
 			programmer = buildPlatform.Programmers[programmerID]
 		}
 		if programmer == nil {
-			return fmt.Errorf(tr("programmer '%s' not available"), programmerID)
+			return status.Newf(codes.InvalidArgument, tr("Programmer '%s' not available"), programmerID)
 		}
 	}
 
@@ -177,7 +175,7 @@ func runProgramAction(pm *packagemanager.PackageManager,
 		if t, ok := props.GetOk(toolProperty); ok {
 			uploadToolID = t
 		} else {
-			return fmt.Errorf(tr("cannot get programmer tool: undefined '%s' property"), toolProperty)
+			return status.Newf(codes.FailedPrecondition, tr("Cannot get programmer tool: undefined '%s' property"), toolProperty)
 		}
 	}
 
@@ -193,7 +191,7 @@ func runProgramAction(pm *packagemanager.PackageManager,
 		Trace("Upload tool")
 
 	if split := strings.Split(uploadToolID, ":"); len(split) > 2 {
-		return fmt.Errorf(tr("invalid 'upload.tool' property: %s"), uploadToolID)
+		return status.Newf(codes.FailedPrecondition, tr("Invalid 'upload.tool' property: %s"), uploadToolID)
 	} else if len(split) == 2 {
 		uploadToolID = split[1]
 		uploadToolPlatform = pm.GetInstalledPlatformRelease(
@@ -232,7 +230,10 @@ func runProgramAction(pm *packagemanager.PackageManager,
 	}
 
 	if !uploadProperties.ContainsKey("upload.protocol") && programmer == nil {
-		return fmt.Errorf(tr("a programmer is required to upload for this board"))
+		err, _ := status.
+			Newf(codes.InvalidArgument, tr("A programmer is required to upload on this board")).
+			WithDetails(&rpc.ProgrammerIsRequiredForUploadError{})
+		return err
 	}
 
 	// Set properties for verbose upload
@@ -280,13 +281,13 @@ func runProgramAction(pm *packagemanager.PackageManager,
 	if !burnBootloader {
 		importPath, sketchName, err := determineBuildPathAndSketchName(importFile, importDir, sk, fqbn)
 		if err != nil {
-			return errors.Errorf(tr("retrieving build artifacts: %s"), err)
+			return status.Newf(codes.Internal, tr("Error finding build artifacts: %s"), err)
 		}
 		if !importPath.Exist() {
-			return fmt.Errorf(tr("compiled sketch not found in %s"), importPath)
+			return status.Newf(codes.Internal, tr("Compiled sketch not found in %s"), importPath)
 		}
 		if !importPath.IsDir() {
-			return fmt.Errorf(tr("expected compiled sketch in directory %s, but is a file instead"), importPath)
+			return status.Newf(codes.Internal, tr("Expected compiled sketch in directory %s, but is a file instead"), importPath)
 		}
 		uploadProperties.SetPath("build.path", importPath)
 		uploadProperties.Set("build.project_name", sketchName)
@@ -370,18 +371,18 @@ func runProgramAction(pm *packagemanager.PackageManager,
 	// Run recipes for upload
 	if burnBootloader {
 		if err := runTool("erase.pattern", uploadProperties, outStream, errStream, verbose, dryRun); err != nil {
-			return fmt.Errorf(tr("chip erase error: %s"), err)
+			return status.Newf(codes.Internal, tr("Failed chip erase: %s"), err)
 		}
 		if err := runTool("bootloader.pattern", uploadProperties, outStream, errStream, verbose, dryRun); err != nil {
-			return fmt.Errorf(tr("burn bootloader error: %s"), err)
+			return status.Newf(codes.Internal, tr("Failed to burn bootloader: %s"), err)
 		}
 	} else if programmer != nil {
 		if err := runTool("program.pattern", uploadProperties, outStream, errStream, verbose, dryRun); err != nil {
-			return fmt.Errorf(tr("programming error: %s"), err)
+			return status.Newf(codes.Internal, tr("Failed programming: %s"), err)
 		}
 	} else {
 		if err := runTool("upload.pattern", uploadProperties, outStream, errStream, verbose, dryRun); err != nil {
-			return fmt.Errorf(tr("uploading error: %s"), err)
+			return status.Newf(codes.Internal, tr("Failed uploading: %s"), err)
 		}
 	}
 

commands/upload/upload_test.go

@@ -177,7 +177,7 @@ func TestUploadPropertiesComposition(t *testing.T) {
 	testRunner := func(t *testing.T, test test, verboseVerify bool) {
 		outStream := &bytes.Buffer{}
 		errStream := &bytes.Buffer{}
-		err := runProgramAction(
+		status := runProgramAction(
 			pm,
 			nil,                     // sketch
 			"",                      // importFile
@@ -197,9 +197,9 @@ func TestUploadPropertiesComposition(t *testing.T) {
 			verboseVerifyOutput = "quiet noverify"
 		}
 		if test.expectedOutput == "FAIL" {
-			require.Error(t, err)
+			require.NotNil(t, status)
 		} else {
-			require.NoError(t, err)
+			require.Nil(t, status)
 			outFiltered := strings.ReplaceAll(outStream.String(), "\r", "")
 			outFiltered = strings.ReplaceAll(outFiltered, "\\", "/")
 			require.Contains(t, outFiltered, strings.ReplaceAll(test.expectedOutput, "$$VERBOSE-VERIFY$$", verboseVerifyOutput))