Proper gRPC error handling

Author: cmaglie

Diff for: cli/compile/compile.go

@@ -25,6 +25,7 @@ import (
 	"github.com/arduino/arduino-cli/cli/feedback"
 	"github.com/arduino/arduino-cli/cli/output"
 	"github.com/arduino/arduino-cli/configuration"
+	"google.golang.org/grpc/status"
 
 	"github.com/arduino/arduino-cli/cli/errorcodes"
 	"github.com/arduino/arduino-cli/cli/instance"
@@ -193,7 +194,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)

Diff for: cli/feedback/feedback.go

@@ -17,11 +17,13 @@ package feedback
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"os"
 
 	"github.com/sirupsen/logrus"
+	"google.golang.org/grpc/status"
 )
 
 // OutputFormat is used to determine the output format
@@ -102,6 +104,16 @@ func (fb *Feedback) Print(v interface{}) {
 // Errorf behaves like fmt.Printf but writes on the error writer and adds a
 // newline. It also logs the error.
 func (fb *Feedback) Errorf(format string, v ...interface{}) {
+	// Unbox grpc status errors
+	for i := range v {
+		if s, isStatus := v[i].(*status.Status); isStatus {
+			v[i] = errors.New(s.Message())
+		} else if err, isErr := v[i].(error); isErr {
+			if s, isStatus := status.FromError(err); isStatus {
+				v[i] = errors.New(s.Message())
+			}
+		}
+	}
 	fb.Error(fmt.Sprintf(format, v...))
 }
 

Diff for: 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 (

Diff for: commands/daemon/daemon.go

@@ -284,7 +284,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)
 }
@@ -297,7 +297,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)
 }
@@ -310,7 +310,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)
 }

Diff for: 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()).

Diff for: commands/upload/upload.go

@@ -37,23 +37,25 @@ import (
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"go.bug.st/serial"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 // 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())
 	sketch, err := sketches.NewSketchFromPath(sketchPath)
 	if err != nil && req.GetImportDir() == "" && req.GetImportFile() == "" {
-		return nil, fmt.Errorf("opening sketch: %s", err)
+		return nil, status.Newf(codes.InvalidArgument, "Sketch not found: %s", err)
 	}
 
 	pm := commands.GetPackageManager(req.GetInstance().GetId())
 
-	err = runProgramAction(
+	return &rpc.UploadResponse{}, runProgramAction(
 		pm,
 		sketch,
 		req.GetImportFile(),
@@ -67,18 +69,14 @@ func Upload(ctx context.Context, req *rpc.UploadRequest, outStream io.Writer, er
 		outStream,
 		errStream,
 	)
-	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("programmer not specified")
+		return nil, status.New(codes.InvalidArgument, "Programmer not specified")
 	}
 	_, err := Upload(ctx, &rpc.UploadRequest{
 		Instance:   req.GetInstance(),
@@ -99,17 +97,17 @@ func runProgramAction(pm *packagemanager.PackageManager,
 	importFile, importDir, fqbnIn, port string,
 	programmerID string,
 	verbose, verify, burnBootloader bool,
-	outStream, errStream io.Writer) error {
+	outStream, errStream io.Writer) *status.Status {
 
 	if burnBootloader && programmerID == "" {
-		return fmt.Errorf("no programmer specified for burning bootloader")
+		return status.New(codes.InvalidArgument, "Programmer not specified")
 	}
 
 	// FIXME: make a specification on how a port is specified via command line
 	if port == "" && sketch != nil && sketch.Metadata != nil {
 		deviceURI, err := url.Parse(sketch.Metadata.CPU.Port)
 		if err != nil {
-			return fmt.Errorf("invalid Device URL format: %s", err)
+			return status.Newf(codes.InvalidArgument, "Invalid Device URL format: %s", err)
 		}
 		if deviceURI.Scheme == "serial" {
 			port = deviceURI.Host + deviceURI.Path
@@ -121,18 +119,18 @@ func runProgramAction(pm *packagemanager.PackageManager,
 		fqbnIn = sketch.Metadata.CPU.Fqbn
 	}
 	if fqbnIn == "" {
-		return fmt.Errorf("no Fully Qualified Board Name provided")
+		return status.New(codes.InvalidArgument, "No FQBN (Fully Qualified Board Name) provided")
 	}
 	fqbn, err := cores.ParseFQBN(fqbnIn)
 	if err != nil {
-		return fmt.Errorf("incorrect FQBN: %s", err)
+		return status.Newf(codes.InvalidArgument, fmt.Sprintf("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("incorrect FQBN: %s", err)
+		return status.Newf(codes.InvalidArgument, "Could not resolve FQBN: %s", err)
 	}
 	logrus.
 		WithField("boardPlatform", boardPlatform).
@@ -149,7 +147,7 @@ func runProgramAction(pm *packagemanager.PackageManager,
 			programmer = buildPlatform.Programmers[programmerID]
 		}
 		if programmer == nil {
-			return fmt.Errorf("programmer '%s' not available", programmerID)
+			return status.Newf(codes.InvalidArgument, "Programmer '%s' not available", programmerID)
 		}
 	}
 
@@ -174,7 +172,7 @@ func runProgramAction(pm *packagemanager.PackageManager,
 		if t, ok := props.GetOk(toolProperty); ok {
 			uploadToolID = t
 		} else {
-			return fmt.Errorf("cannot get programmer tool: undefined '%s' property", toolProperty)
+			return status.Newf(codes.FailedPrecondition, "Cannot get programmer tool: undefined '%s' property", toolProperty)
 		}
 	}
 
@@ -190,7 +188,7 @@ func runProgramAction(pm *packagemanager.PackageManager,
 		Trace("Upload tool")
 
 	if split := strings.Split(uploadToolID, ":"); len(split) > 2 {
-		return fmt.Errorf("invalid 'upload.tool' property: %s", uploadToolID)
+		return status.Newf(codes.FailedPrecondition, "Invalid 'upload.tool' property: %s", uploadToolID)
 	} else if len(split) == 2 {
 		uploadToolID = split[1]
 		uploadToolPlatform = pm.GetInstalledPlatformRelease(
@@ -229,7 +227,10 @@ func runProgramAction(pm *packagemanager.PackageManager,
 	}
 
 	if !uploadProperties.ContainsKey("upload.protocol") && programmer == nil {
-		return fmt.Errorf("a programmer is required to upload for this board")
+		err, _ := status.
+			Newf(codes.InvalidArgument, "A programmer is required to upload on this board").
+			WithDetails(&rpc.UploadError{Code: rpc.UploadError_CODE_PROGRAMMER_REQUIRED_FOR_UPLOAD})
+		return err
 	}
 
 	// Set properties for verbose upload
@@ -277,13 +278,13 @@ func runProgramAction(pm *packagemanager.PackageManager,
 	if !burnBootloader {
 		importPath, sketchName, err := determineBuildPathAndSketchName(importFile, importDir, sketch, fqbn)
 		if err != nil {
-			return errors.Errorf("retrieving build artifacts: %s", err)
+			return status.Newf(codes.Internal, "Error finding build artifacts: %s", err)
 		}
 		if !importPath.Exist() {
-			return fmt.Errorf("compiled sketch not found in %s", importPath)
+			return status.Newf(codes.Internal, "Compiled sketch not found in %s", importPath)
 		}
 		if !importPath.IsDir() {
-			return fmt.Errorf("expected compiled sketch in directory %s, but is a file instead", importPath)
+			return status.Newf(codes.Internal, "Expected compiled sketch in directory %s, but is a file instead", importPath)
 		}
 		uploadProperties.SetPath("build.path", importPath)
 		uploadProperties.Set("build.project_name", sketchName)
@@ -296,12 +297,12 @@ func runProgramAction(pm *packagemanager.PackageManager,
 		// Perform reset via 1200bps touch if requested
 		if uploadProperties.GetBoolean("upload.use_1200bps_touch") {
 			if port == "" {
-				return fmt.Errorf("no upload port provided")
+				return status.New(codes.InvalidArgument, "No upload port specified")
 			}
 
 			ports, err := serial.GetPortsList()
 			if err != nil {
-				return fmt.Errorf("cannot get serial port list: %s", err)
+				return status.Newf(codes.Internal, "Cannot get serial port list: %s", err)
 			}
 			for _, p := range ports {
 				if p == port {
@@ -327,7 +328,7 @@ func runProgramAction(pm *packagemanager.PackageManager,
 
 			actualPort, err = serialutils.WaitForNewSerialPortOrDefaultTo(actualPort)
 			if err != nil {
-				return errors.WithMessage(err, "detecting serial port")
+				return status.Newf(codes.Internal, "Failed detecting serial port: %s", err)
 			}
 		}
 	}
@@ -345,18 +346,18 @@ func runProgramAction(pm *packagemanager.PackageManager,
 	// Run recipes for upload
 	if burnBootloader {
 		if err := runTool("erase.pattern", uploadProperties, outStream, errStream, verbose); err != nil {
-			return fmt.Errorf("chip erase error: %s", err)
+			return status.Newf(codes.Internal, "Failed chip erase: %s", err)
 		}
 		if err := runTool("bootloader.pattern", uploadProperties, outStream, errStream, verbose); err != nil {
-			return fmt.Errorf("burn bootloader error: %s", err)
+			return status.Newf(codes.Internal, "Failed to burn bootloader: %s", err)
 		}
 	} else if programmer != nil {
 		if err := runTool("program.pattern", uploadProperties, outStream, errStream, verbose); err != nil {
-			return fmt.Errorf("programming error: %s", err)
+			return status.Newf(codes.Internal, "Failed programming: %s", err)
 		}
 	} else {
 		if err := runTool("upload.pattern", uploadProperties, outStream, errStream, verbose); err != nil {
-			return fmt.Errorf("uploading error: %s", err)
+			return status.Newf(codes.Internal, "Failed uploading: %s", err)
 		}
 	}
 

Diff for: commands/upload/upload_test.go

@@ -178,7 +178,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))

Diff for: go.mod

@@ -46,6 +46,7 @@ require (
 	golang.org/x/crypto v0.0.0-20200406173513-056763e48d71
 	golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e
 	golang.org/x/text v0.3.2
+	google.golang.org/genproto v0.0.0-20200526211855-cb27e3aa2013
 	google.golang.org/grpc v1.27.0
 	google.golang.org/protobuf v1.25.0
 	gopkg.in/mgo.v2 v2.0.0-20180705113604-9856a29383ce // indirect