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

… of error
arduino · cmaglie · Aug 30, 2021 · Feb 10, 2021 · Jun 25, 2021 · Jun 30, 2021
commit 67c19f2cbfce593e1ab6b383e5616c4a644cac6a
diff --git a/cli/compile/compile.go b/cli/compile/compile.go
@@ -183,16 +183,16 @@ func run(cmd *cobra.Command, args []string) {
 	compileErr := new(bytes.Buffer)
 	verboseCompile := configuration.Settings.GetString("logging.level") == "debug"
 	var compileRes *rpc.CompileResponse
-	var err error
+	var st *status.Status
 	if output.OutputFormat == "json" {
-		compileRes, err = compile.Compile(context.Background(), compileRequest, compileOut, compileErr, verboseCompile)
+		compileRes, st = compile.Compile(context.Background(), compileRequest, compileOut, compileErr, verboseCompile)
 	} else {
-		compileRes, err = compile.Compile(context.Background(), compileRequest, os.Stdout, os.Stderr, verboseCompile)
+		compileRes, st = compile.Compile(context.Background(), compileRequest, os.Stdout, os.Stderr, verboseCompile)
 	}

-	if err == nil && uploadAfterCompile {
+	if st == 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)
@@ -250,10 +250,10 @@ func run(cmd *cobra.Command, args []string) {
 		CompileOut:    compileOut.String(),
 		CompileErr:    compileErr.String(),
 		BuilderResult: compileRes,
-		Success:       err == nil,
+		Success:       st == nil,
 	})
-	if err != nil && output.OutputFormat != "json" {
-		feedback.Errorf(tr("Error during build: %v"), err)
+	if st != nil && output.OutputFormat != "json" {
+		feedback.Errorf(tr("Error during build: %v"), st.Message())
 		os.Exit(errorcodes.ErrGeneric)
 	}
 }

diff --git a/cli/core/upgrade.go b/cli/core/upgrade.go
@@ -95,7 +95,7 @@ func runUpgradeCommand(cmd *cobra.Command, args []string) {
 		}

 		_, err := core.PlatformUpgrade(context.Background(), r, output.ProgressBar(), output.TaskProgress())
-		if err == core.ErrAlreadyLatest {
+		if err.Message() == core.ErrAlreadyLatest.Error() {
 			feedback.Printf(tr("Platform %s is already at the latest version"), platformRef)
 		} else if err != nil {
 			feedback.Errorf(tr("Error during upgrade: %v"), err)

diff --git a/cli/debug/debug.go b/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)
+			feedback.Errorf(tr("Error getting Debug info: %v"), err.Message())
+			errorcodes.ExitWithGrpcStatus(err)
 		} else {
 			feedback.PrintResult(&debugInfoResult{res})
 		}

diff --git a/cli/instance/instance.go b/cli/instance/instance.go
@@ -27,7 +27,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"
 )

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


diff --git a/cli/lib/args.go b/cli/lib/args.go
@@ -75,13 +75,13 @@ 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,
 	})
 	if err != nil {
-		return nil, err
+		return nil, err.Err()
 	}

 	candidates := []*rpc.SearchedLibrary{}

diff --git a/cli/lib/check_deps.go b/cli/lib/check_deps.go
@@ -51,13 +51,13 @@ func runDepsCommand(cmd *cobra.Command, args []string) {
 		os.Exit(errorcodes.ErrBadArgument)
 	}

-	deps, err := lib.LibraryResolveDependencies(context.Background(), &rpc.LibraryResolveDependenciesRequest{
+	deps, stat := lib.LibraryResolveDependencies(context.Background(), &rpc.LibraryResolveDependenciesRequest{
 		Instance: instance,
 		Name:     libRef.Name,
 		Version:  libRef.Version,
 	})
-	if err != nil {
-		feedback.Errorf(tr("Error resolving dependencies for %[1]s: %[2]s"), libRef, err)
+	if stat != nil {
+		feedback.Errorf(tr("Error resolving dependencies for %[1]s: %[2]s", libRef, err))
 	}

 	feedback.PrintResult(&checkDepResult{deps: deps})

diff --git a/commands/compile/compile.go b/commands/compile/compile.go
@@ -17,7 +17,6 @@ package compile

 import (
 	"context"
-	"fmt"
 	"io"
 	"path/filepath"
 	"sort"
@@ -41,12 +40,14 @@ import (
 	"github.com/pkg/errors"
 	"github.com/segmentio/stats/v4"
 	"github.com/sirupsen/logrus"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )

 var tr = i18n.Tr

 // Compile FIXMEDOC
-func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream io.Writer, debug bool) (r *rpc.CompileResponse, e error) {
+func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream io.Writer, debug bool) (r *rpc.CompileResponse, e *status.Status) {

 	// There is a binding between the export binaries setting and the CLI flag to explicitly set it,
 	// since we want this binding to work also for the gRPC interface we must read it here in this
@@ -90,29 +91,29 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream

 	pm := commands.GetPackageManager(req.GetInstance().GetId())
 	if pm == nil {
-		return nil, errors.New(tr("invalid instance"))
+		return nil, status.New(codes.InvalidArgument, tr("Invalid instance"))
 	}

 	logrus.Tracef("Compile %s for %s started", req.GetSketchPath(), req.GetFqbn())
 	if req.GetSketchPath() == "" {
-		return nil, fmt.Errorf(tr("missing sketchPath"))
+		return nil, status.New(codes.InvalidArgument, tr("Missing sketch path"))
 	}
 	sketchPath := paths.New(req.GetSketchPath())
 	sk, err := sketch.New(sketchPath)
 	if err != nil {
-		return nil, fmt.Errorf(tr("opening sketch: %s"), err)
+		return nil, status.Newf(codes.NotFound, tr("Error opening sketch: %s"), err)
 	}

 	fqbnIn := req.GetFqbn()
 	if fqbnIn == "" && sk != nil && sk.Metadata != nil {
 		fqbnIn = sk.Metadata.CPU.Fqbn
 	}
 	if fqbnIn == "" {
-		return nil, fmt.Errorf(tr("no FQBN provided"))
+		return nil, status.New(codes.InvalidArgument, tr("No FQBN (Fully Qualified Board Name) provided"))
 	}
 	fqbn, err := cores.ParseFQBN(fqbnIn)
 	if err != nil {
-		return nil, fmt.Errorf(tr("incorrect FQBN: %s"), err)
+		return nil, status.Newf(codes.InvalidArgument, tr("Invalid FQBN: %s"), err)
 	}

 	targetPlatform := pm.FindPlatform(&packagemanager.PlatformReference{
@@ -125,7 +126,7 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream
 		// 	"\"%[1]s:%[2]s\" platform is not installed, please install it by running \""+
 		// 		version.GetAppName()+" core install %[1]s:%[2]s\".", fqbn.Package, fqbn.PlatformArch)
 		// feedback.Error(errorMessage)
-		return nil, fmt.Errorf(tr("platform not installed"))
+		return nil, status.New(codes.NotFound, tr("Platform not installed"))
 	}

 	builderCtx := &types.Context{}
@@ -148,7 +149,7 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream
 		builderCtx.BuildPath = paths.New(req.GetBuildPath())
 	}
 	if err = builderCtx.BuildPath.MkdirAll(); err != nil {
-		return nil, fmt.Errorf(tr("cannot create build directory: %s"), err)
+		return nil, status.Newf(codes.PermissionDenied, tr("Cannot create build directory: %s"), err)
 	}
 	builderCtx.CompilationDatabase = bldr.NewCompilationDatabase(
 		builderCtx.BuildPath.Join("compile_commands.json"),
@@ -178,7 +179,7 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream
 		builderCtx.BuildCachePath = paths.New(req.GetBuildCachePath())
 		err = builderCtx.BuildCachePath.MkdirAll()
 		if err != nil {
-			return nil, fmt.Errorf(tr("cannot create build cache directory: %s"), err)
+			return nil, status.Newf(codes.PermissionDenied, tr("Cannot create build cache directory: %s"), err)
 		}
 	}

@@ -221,14 +222,14 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream

 	// if --preprocess or --show-properties were passed, we can stop here
 	if req.GetShowProperties() {
-		return r, builder.RunParseHardwareAndDumpBuildProperties(builderCtx)
+		return r, status.Convert(builder.RunParseHardwareAndDumpBuildProperties(builderCtx))
 	} else if req.GetPreprocess() {
-		return r, builder.RunPreprocess(builderCtx)
+		return r, status.Convert(builder.RunPreprocess(builderCtx))
 	}

 	// if it's a regular build, go on...
 	if err := builder.RunBuilder(builderCtx); err != nil {
-		return r, err
+		return r, status.Convert(err)
 	}

 	// If the export directory is set we assume you want to export the binaries
@@ -250,17 +251,17 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream
 		}
 		logrus.WithField("path", exportPath).Trace("Saving sketch to export path.")
 		if err := exportPath.MkdirAll(); err != nil {
-			return r, errors.Wrap(err, tr("creating output dir"))
+			return r, status.New(codes.PermissionDenied, errors.Wrap(err, tr("Error creating output dir")).Error())
 		}

 		// Copy all "sketch.ino.*" artifacts to the export directory
 		baseName, ok := builderCtx.BuildProperties.GetOk("build.project_name") // == "sketch.ino"
 		if !ok {
-			return r, errors.New(tr("missing 'build.project_name' build property"))
+			return r, status.New(codes.Internal, tr("Missing 'build.project_name' build property"))
 		}
 		buildFiles, err := builderCtx.BuildPath.ReadDir()
 		if err != nil {
-			return r, errors.Errorf(tr("reading build directory: %s"), err)
+			return r, status.Newf(codes.PermissionDenied, tr("Error reading build directory: %s"), err)
 		}
 		buildFiles.FilterPrefix(baseName)
 		for _, buildFile := range buildFiles {
@@ -270,7 +271,7 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream
 				WithField("dest", exportedFile).
 				Trace("Copying artifact.")
 			if err = buildFile.CopyTo(exportedFile); err != nil {
-				return r, errors.Wrapf(err, tr("copying output file %s"), buildFile)
+				return r, status.New(codes.PermissionDenied, tr("Error copying output file %[1]s: %[2]s", buildFile, err))
 			}
 		}
 	}
@@ -279,7 +280,7 @@ func Compile(ctx context.Context, req *rpc.CompileRequest, outStream, errStream
 	for _, lib := range builderCtx.ImportedLibraries {
 		rpcLib, err := lib.ToRPCLibrary()
 		if err != nil {
-			return r, fmt.Errorf(tr("converting library %[1]s to rpc struct: %[2]w"), lib.Name, err)
+			return r, status.Newf(codes.PermissionDenied, tr("Error converting library %[1]s to rpc struct: %[2]s", lib.Name, err))
 		}
 		importedLibs = append(importedLibs, rpcLib)
 	}

diff --git a/commands/core/download.go b/commands/core/download.go
@@ -17,28 +17,29 @@ package core

 import (
 	"context"
-	"errors"
 	"fmt"

 	"github.com/arduino/arduino-cli/arduino/cores"
 	"github.com/arduino/arduino-cli/arduino/cores/packagemanager"
 	"github.com/arduino/arduino-cli/commands"
 	"github.com/arduino/arduino-cli/i18n"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )

 var tr = i18n.Tr

 // PlatformDownload FIXMEDOC
-func PlatformDownload(ctx context.Context, req *rpc.PlatformDownloadRequest, downloadCB commands.DownloadProgressCB) (*rpc.PlatformDownloadResponse, error) {
+func PlatformDownload(ctx context.Context, req *rpc.PlatformDownloadRequest, downloadCB commands.DownloadProgressCB) (*rpc.PlatformDownloadResponse, *status.Status) {
 	pm := commands.GetPackageManager(req.GetInstance().GetId())
 	if pm == nil {
-		return nil, errors.New(tr("invalid instance"))
+		return nil, status.New(codes.InvalidArgument, tr("Invalid instance"))
 	}

 	version, err := commands.ParseVersion(req)
 	if err != nil {
-		return nil, fmt.Errorf(tr("invalid version: %s"), err)
+		return nil, status.Newf(codes.InvalidArgument, tr("Invalid version: %s"), err)
 	}

 	platform, tools, err := pm.FindPlatformReleaseDependencies(&packagemanager.PlatformReference{
@@ -47,18 +48,18 @@ func PlatformDownload(ctx context.Context, req *rpc.PlatformDownloadRequest, dow
 		PlatformVersion:      version,
 	})
 	if err != nil {
-		return nil, fmt.Errorf(tr("find platform dependencies: %s"), err)
+		return nil, status.Newf(codes.InvalidArgument, tr("Error finding platform dependencies: %s"), err)
 	}

 	err = downloadPlatform(pm, platform, downloadCB)
 	if err != nil {
-		return nil, err
+		return nil, status.New(codes.Unavailable, err.Error())
 	}

 	for _, tool := range tools {
 		err := downloadTool(pm, tool, downloadCB)
 		if err != nil {
-			return nil, fmt.Errorf(tr("downloading tool %[1]s: %[2]s"), tool, err)
+			return nil, status.Newf(codes.Unavailable, tr("Error downloading tool %[1]s: %[2]s"), tool, err)
 		}
 	}


diff --git a/commands/core/install.go b/commands/core/install.go
@@ -23,21 +23,22 @@ import (
 	"github.com/arduino/arduino-cli/arduino/cores/packagemanager"
 	"github.com/arduino/arduino-cli/commands"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
-	"github.com/pkg/errors"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )

 // PlatformInstall FIXMEDOC
 func PlatformInstall(ctx context.Context, req *rpc.PlatformInstallRequest,
-	downloadCB commands.DownloadProgressCB, taskCB commands.TaskProgressCB) (*rpc.PlatformInstallResponse, error) {
+	downloadCB commands.DownloadProgressCB, taskCB commands.TaskProgressCB) (*rpc.PlatformInstallResponse, *status.Status) {

 	pm := commands.GetPackageManager(req.GetInstance().GetId())
 	if pm == nil {
-		return nil, errors.New(tr("invalid instance"))
+		return nil, status.New(codes.InvalidArgument, tr("Invalid instance"))
 	}

 	version, err := commands.ParseVersion(req)
 	if err != nil {
-		return nil, fmt.Errorf(tr("invalid version: %s"), err)
+		return nil, status.Newf(codes.InvalidArgument, tr("Invalid version: %s"), err)
 	}

 	platform, tools, err := pm.FindPlatformReleaseDependencies(&packagemanager.PlatformReference{
@@ -46,17 +47,17 @@ func PlatformInstall(ctx context.Context, req *rpc.PlatformInstallRequest,
 		PlatformVersion:      version,
 	})
 	if err != nil {
-		return nil, fmt.Errorf(tr("finding platform dependencies: %s"), err)
+		return nil, status.Newf(codes.InvalidArgument, tr("Error finding platform dependencies: %s"), err)
 	}

 	err = installPlatform(pm, platform, tools, downloadCB, taskCB, req.GetSkipPostInstall())
 	if err != nil {
-		return nil, err
+		return nil, status.Convert(err)
 	}

 	status := commands.Init(&rpc.InitRequest{Instance: req.Instance}, nil)
 	if status != nil {
-		return nil, status.Err()
+		return nil, status
 	}

 	return &rpc.PlatformInstallResponse{}, nil
-Original file line number
+Diff line change
@@ -75,13 +75,13 @@ 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


      

  

  
    
      

      
            silvanocerza
  

      

      

      


        Aug 11, 2021


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    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?
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



    
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
    Contributor


      

  

  
    
      

      
            per1234
  

      

      

      


        Aug 11, 2021


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    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.
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



    
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  Author


  

  
    
      

      
            cmaglie
  

      

      

      


        Aug 11, 2021


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    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.
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
     	res, err := lib.LibrarySearch(context.Background(), &rpc.LibrarySearchRequest{
     		Instance: instance,
     		Query:    libRef.Name,
     	})
     	if err != nil {
-    		return nil, err
+    		return nil, err.Err()
     	}
     	candidates := []*rpc.SearchedLibrary{}
-          Expand Down