[breaking] Refactor errors handling in packagemanager.Load* golang API (#1682)

* Removed `error` return from `discovery.New(...)`

The `New` function never fails.

* Replaced *status.Status with errors in packagamanager

* Apply suggestions from code review

Author: cmaglie

arduino/cores/packagemanager/loader.go

@@ -140,10 +140,10 @@ func TestLoadDiscoveries(t *testing.T) {
 		"pluggable_discovery.required": "arduino:ble-discovery",
 	})
 
-	errs := packageManager.LoadDiscoveries()
-	require.Len(t, errs, 2)
-	require.Equal(t, errs[0].Message(), "discovery not found: builtin:serial-discovery")
-	require.Equal(t, errs[1].Message(), "discovery not found: builtin:mdns-discovery")
+	err := packageManager.LoadDiscoveries()
+	require.Len(t, err, 2)
+	require.Equal(t, err[0].Error(), "discovery builtin:serial-discovery not found")
+	require.Equal(t, err[1].Error(), "discovery builtin:mdns-discovery not found")
 	discoveries := packageManager.DiscoveryManager().IDs()
 	require.Len(t, discoveries, 1)
 	require.Contains(t, discoveries, "arduino:ble-discovery")
@@ -155,10 +155,10 @@ func TestLoadDiscoveries(t *testing.T) {
 		"pluggable_discovery.required.1": "arduino:serial-discovery",
 	})
 
-	errs = packageManager.LoadDiscoveries()
-	require.Len(t, errs, 2)
-	require.Equal(t, errs[0].Message(), "discovery not found: builtin:serial-discovery")
-	require.Equal(t, errs[1].Message(), "discovery not found: builtin:mdns-discovery")
+	err = packageManager.LoadDiscoveries()
+	require.Len(t, err, 2)
+	require.Equal(t, err[0].Error(), "discovery builtin:serial-discovery not found")
+	require.Equal(t, err[1].Error(), "discovery builtin:mdns-discovery not found")
 	discoveries = packageManager.DiscoveryManager().IDs()
 	require.Len(t, discoveries, 2)
 	require.Contains(t, discoveries, "arduino:ble-discovery")
@@ -172,10 +172,10 @@ func TestLoadDiscoveries(t *testing.T) {
 		"pluggable_discovery.teensy.pattern": "\"{runtime.tools.teensy_ports.path}/hardware/tools/teensy_ports\" -J2",
 	})
 
-	errs = packageManager.LoadDiscoveries()
-	require.Len(t, errs, 2)
-	require.Equal(t, errs[0].Message(), "discovery not found: builtin:serial-discovery")
-	require.Equal(t, errs[1].Message(), "discovery not found: builtin:mdns-discovery")
+	err = packageManager.LoadDiscoveries()
+	require.Len(t, err, 2)
+	require.Equal(t, err[0].Error(), "discovery builtin:serial-discovery not found")
+	require.Equal(t, err[1].Error(), "discovery builtin:mdns-discovery not found")
 	discoveries = packageManager.DiscoveryManager().IDs()
 	require.Len(t, discoveries, 3)
 	require.Contains(t, discoveries, "arduino:ble-discovery")
@@ -191,10 +191,10 @@ func TestLoadDiscoveries(t *testing.T) {
 		"pluggable_discovery.teensy.pattern": "\"{runtime.tools.teensy_ports.path}/hardware/tools/teensy_ports\" -J2",
 	})
 
-	errs = packageManager.LoadDiscoveries()
-	require.Len(t, errs, 2)
-	require.Equal(t, errs[0].Message(), "discovery not found: builtin:serial-discovery")
-	require.Equal(t, errs[1].Message(), "discovery not found: builtin:mdns-discovery")
+	err = packageManager.LoadDiscoveries()
+	require.Len(t, err, 2)
+	require.Equal(t, err[0].Error(), "discovery builtin:serial-discovery not found")
+	require.Equal(t, err[1].Error(), "discovery builtin:mdns-discovery not found")
 	discoveries = packageManager.DiscoveryManager().IDs()
 	require.Len(t, discoveries, 3)
 	require.Contains(t, discoveries, "arduino:ble-discovery")

arduino/cores/packagemanager/loader_test.go

@@ -126,14 +126,13 @@ type Event struct {
 }
 
 // New create and connect to the given pluggable discovery
-func New(id string, args ...string) (*PluggableDiscovery, error) {
-	disc := &PluggableDiscovery{
+func New(id string, args ...string) *PluggableDiscovery {
+	return &PluggableDiscovery{
 		id:          id,
 		processArgs: args,
 		state:       Dead,
 		cachedPorts: map[string]*Port{},
 	}
-	return disc, nil
 }
 
 // GetID returns the identifier for this discovery

arduino/discovery/discovery.go

@@ -32,11 +32,7 @@ func main() {
 	discoveries := []*discovery.PluggableDiscovery{}
 	discEvent := make(chan *discovery.Event)
 	for _, discCmd := range os.Args[1:] {
-		disc, err := discovery.New("", discCmd)
-		if err != nil {
-			log.Fatal("Error initializing discovery:", err)
-		}
-
+		disc := discovery.New("", discCmd)
 		if err := disc.Run(); err != nil {
 			log.Fatal("Error starting discovery:", err)
 		}

arduino/discovery/discovery_client/main.go

@@ -32,8 +32,7 @@ func TestDiscoveryStdioHandling(t *testing.T) {
 	require.NoError(t, builder.Run())
 
 	// Run cat and test if streaming json works as expected
-	disc, err := New("test", "testdata/cat/cat") // copy stdin to stdout
-	require.NoError(t, err)
+	disc := New("test", "testdata/cat/cat") // copy stdin to stdout
 
 	err = disc.runProcess()
 	require.NoError(t, err)

arduino/discovery/discovery_test.go

@@ -338,6 +338,24 @@ func (e *PlatformNotFoundError) Unwrap() error {
 	return e.Cause
 }
 
+// PlatformLoadingError is returned when a platform has fatal errors that prevents loading
+type PlatformLoadingError struct {
+	Cause error
+}
+
+func (e *PlatformLoadingError) Error() string {
+	return composeErrorMsg(tr("Error loading hardware platform"), e.Cause)
+}
+
+// ToRPCStatus converts the error into a *status.Status
+func (e *PlatformLoadingError) ToRPCStatus() *status.Status {
+	return status.New(codes.FailedPrecondition, e.Error())
+}
+
+func (e *PlatformLoadingError) Unwrap() error {
+	return e.Cause
+}
+
 // LibraryNotFoundError is returned when a platform is not found
 type LibraryNotFoundError struct {
 	Library string

arduino/errors.go

@@ -233,9 +233,10 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro
 	// otherwise we wouldn't find them and reinstall them each time
 	// and they would never get reloaded.
 	for _, err := range instance.PackageManager.LoadHardware() {
+		s := &arduino.PlatformLoadingError{Cause: err}
 		responseCallback(&rpc.InitResponse{
 			Message: &rpc.InitResponse_Error{
-				Error: err.Proto(),
+				Error: s.ToRPCStatus().Proto(),
 			},
 		})
 	}
@@ -296,18 +297,20 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro
 		// We installed at least one new tool after loading hardware
 		// so we must reload again otherwise we would never found them.
 		for _, err := range instance.PackageManager.LoadHardware() {
+			s := &arduino.PlatformLoadingError{Cause: err}
 			responseCallback(&rpc.InitResponse{
 				Message: &rpc.InitResponse_Error{
-					Error: err.Proto(),
+					Error: s.ToRPCStatus().Proto(),
 				},
 			})
 		}
 	}
 
 	for _, err := range instance.PackageManager.LoadDiscoveries() {
+		s := &arduino.PlatformLoadingError{Cause: err}
 		responseCallback(&rpc.InitResponse{
 			Message: &rpc.InitResponse_Error{
-				Error: err.Proto(),
+				Error: s.ToRPCStatus().Proto(),
 			},
 		})
 	}

commands/instances.go

@@ -2,6 +2,46 @@
 
 Here you can find a list of migration guides to handle breaking changes between releases of the CLI.
 
+## 0.22.0
+
+### `packagemanager.Load*` functions now returns `error` instead of `*status.Status`
+
+The following functions signature:
+
+```go
+func (pm *PackageManager) LoadHardware() []*status.Status { ... }
+func (pm *PackageManager) LoadHardwareFromDirectories(hardwarePaths paths.PathList) []*status.Status { ... }
+func (pm *PackageManager) LoadHardwareFromDirectory(path *paths.Path) []*status.Status { ... }
+func (pm *PackageManager) LoadToolsFromBundleDirectories(dirs paths.PathList) []*status.Status { ... }
+func (pm *PackageManager) LoadDiscoveries() []*status.Status { ... }
+```
+
+have been changed to:
+
+```go
+func (pm *PackageManager) LoadHardware() []error { ... }
+func (pm *PackageManager) LoadHardwareFromDirectories(hardwarePaths paths.PathList) []error { ... }
+func (pm *PackageManager) LoadHardwareFromDirectory(path *paths.Path) []error { ... }
+func (pm *PackageManager) LoadToolsFromBundleDirectories(dirs paths.PathList) []error { ... }
+func (pm *PackageManager) LoadDiscoveries() []error { ... }
+```
+
+These function no longer returns a gRPC status, so the errors can be handled as any other `error`.
+
+### Removed `error` return from `discovery.New(...)` function
+
+The `discovery.New(...)` function never fails, so the error has been removed, the old signature:
+
+```go
+func New(id string, args ...string) (*PluggableDiscovery, error) { ... }
+```
+
+is now:
+
+```go
+func New(id string, args ...string) *PluggableDiscovery { ... }
+```
+
 ## 0.21.0
 
 ### `packagemanager.NewPackageManager` function change

docs/UPGRADING.md

@@ -27,18 +27,16 @@ func (s *HardwareLoader) Run(ctx *types.Context) error {
 		// This should happen only on legacy arduino-builder.
 		// Hopefully this piece will be removed once the legacy package will be cleanedup.
 		pm := packagemanager.NewPackageManager(nil, nil, nil, nil, "arduino-builder")
-		if errs := pm.LoadHardwareFromDirectories(ctx.HardwareDirs); len(errs) > 0 {
+		errs := pm.LoadHardwareFromDirectories(ctx.HardwareDirs)
+		if ctx.Verbose {
 			// With the refactoring of the initialization step of the CLI we changed how
 			// errors are returned when loading platforms and libraries, that meant returning a list of
 			// errors instead of a single one to enhance the experience for the user.
 			// I have no intention right now to start a refactoring of the legacy package too, so
 			// here's this shitty solution for now.
 			// When we're gonna refactor the legacy package this will be gone.
-
-			if ctx.Verbose {
-				for _, err := range errs {
-					ctx.Info(tr("Error loading hardware platform: %[1]s", err.Message()))
-				}
+			for _, err := range errs {
+				ctx.Info(tr("Error loading hardware platform: %[1]s", err.Error()))
 			}
 		}
 		ctx.PackageManager = pm

legacy/builder/hardware_loader.go

@@ -759,7 +759,7 @@ def test_core_with_wrong_custom_board_options_is_loaded(run_command, data_dir):
     assert "arduino-beta-dev:platform_with_wrong_custom_board_options:altra" in boards
     # Verify warning is shown to user
     assert (
-        "Error initializing instance: "
+        "Error initializing instance: Error loading hardware platform: "
         + "loading platform release arduino-beta-dev:[email protected]: "
         + "loading boards: "
         + "skipping loading of boards arduino-beta-dev:platform_with_wrong_custom_board_options:nessuno: "
@@ -797,7 +797,7 @@ def test_core_with_missing_custom_board_options_is_loaded(run_command, data_dir)
     assert "arduino-beta-dev:platform_with_missing_custom_board_options:altra" in boards
     # Verify warning is shown to user
     assert (
-        "Error initializing instance: "
+        "Error initializing instance: Error loading hardware platform: "
         + "loading platform release arduino-beta-dev:[email protected]: "
         + "loading boards: "
         + "skipping loading of boards arduino-beta-dev:platform_with_missing_custom_board_options:nessuno: "