[breaking] core update-index do not stop at the first download error (#1866)

* Added flag to skip 3rd party package_index in UpdateIndex gRPC API

Fix #1788

* UpdateIndex gRPC call does not stop at the first error

* Updated docs

* Fixed integration tests

* Ensure that a 'Completed' download progress is always sent

* UpdateIndex: Fixed a wrong success report and added test.

* Improved test error messages

Author: cmaglie

arduino/httpclient/httpclient.go

@@ -50,6 +50,7 @@ func DownloadFile(path *paths.Path, URL string, label string, downloadCB rpc.Dow
 		Url:       d.URL,
 		TotalSize: d.Size(),
 	})
+	defer downloadCB(&rpc.DownloadProgress{Completed: true})
 
 	err = d.RunAndPoll(func(downloaded int64) {
 		downloadCB(&rpc.DownloadProgress{Downloaded: downloaded})
@@ -63,7 +64,6 @@ func DownloadFile(path *paths.Path, URL string, label string, downloadCB rpc.Dow
 		return &arduino.FailedDownloadError{Message: tr("Server responded with: %s", d.Resp.Status)}
 	}
 
-	downloadCB(&rpc.DownloadProgress{Completed: true})
 	return nil
 }
 

cli/core/search.go

@@ -67,11 +67,10 @@ func runSearchCommand(cmd *cobra.Command, args []string) {
 	}
 
 	if indexesNeedUpdating(indexUpdateInterval) {
-		_, err := commands.UpdateIndex(context.Background(), &rpc.UpdateIndexRequest{
-			Instance: inst,
-		}, output.ProgressBar())
+		err := commands.UpdateIndex(context.Background(), &rpc.UpdateIndexRequest{Instance: inst},
+			output.ProgressBar(),
+			output.PrintErrorFromDownloadResult(tr("Error updating index")))
 		if err != nil {
-			feedback.Errorf(tr("Error updating index: %v"), err)
 			os.Exit(errorcodes.ErrGeneric)
 		}
 	}

cli/core/update_index.go

@@ -20,7 +20,6 @@ import (
 	"os"
 
 	"github.com/arduino/arduino-cli/cli/errorcodes"
-	"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"
@@ -45,11 +44,10 @@ func runUpdateIndexCommand(cmd *cobra.Command, args []string) {
 	inst := instance.CreateInstanceAndRunFirstUpdate()
 	logrus.Info("Executing `arduino-cli core update-index`")
 
-	_, err := commands.UpdateIndex(context.Background(), &rpc.UpdateIndexRequest{
-		Instance: inst,
-	}, output.ProgressBar())
+	err := commands.UpdateIndex(context.Background(), &rpc.UpdateIndexRequest{Instance: inst},
+		output.ProgressBar(),
+		output.PrintErrorFromDownloadResult(tr("Error updating index")))
 	if err != nil {
-		feedback.Errorf(tr("Error updating index: %v"), err)
 		os.Exit(errorcodes.ErrGeneric)
 	}
 }

cli/instance/instance.go

@@ -152,12 +152,13 @@ func FirstUpdate(instance *rpc.Instance) error {
 	// similarly to the library update we download that file and all the other package indexes
 	// from additional_urls
 	if packageIndex.NotExist() {
-		_, err := commands.UpdateIndex(context.Background(),
+		err := commands.UpdateIndex(context.Background(),
 			&rpc.UpdateIndexRequest{
-				Instance: instance,
+				Instance:                   instance,
+				IgnoreCustomPackageIndexes: true,
 			},
 			output.ProgressBar(),
-		)
+			output.PrintErrorFromDownloadResult(tr("Error updating index")))
 		if err != nil {
 			return err
 		}

cli/output/rpc_progress.go

@@ -18,6 +18,7 @@ package output
 import (
 	"fmt"
 
+	"github.com/arduino/arduino-cli/cli/feedback"
 	"github.com/arduino/arduino-cli/i18n"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
 	"github.com/cmaglie/pb"
@@ -40,6 +41,21 @@ func ProgressBar() rpc.DownloadProgressCB {
 	}
 }
 
+// PrintErrorFromDownloadResult returns a DownloadResultCB that only prints
+// the errors with the give message prefixed.
+func PrintErrorFromDownloadResult(msg string) rpc.DownloadResultCB {
+	if OutputFormat != "json" {
+		return func(res *rpc.DownloadResult) {
+			if !res.GetSuccessful() {
+				feedback.Errorf("%s: %s", msg, res.GetError())
+			}
+		}
+	}
+	return func(res *rpc.DownloadResult) {
+		// XXX: Output result in JSON?
+	}
+}
+
 // TaskProgress returns a TaskProgressCB that prints the task progress.
 // If JSON output format has been selected, the callback outputs nothing.
 func TaskProgress() rpc.TaskProgressCB {

cli/update/update.go

@@ -56,9 +56,9 @@ func runUpdateCommand(cmd *cobra.Command, args []string) {
 	inst := instance.CreateInstanceAndRunFirstUpdate()
 	logrus.Info("Executing `arduino-cli update`")
 
-	err := commands.UpdateCoreLibrariesIndex(context.Background(), &rpc.UpdateCoreLibrariesIndexRequest{
-		Instance: inst,
-	}, output.ProgressBar())
+	err := commands.UpdateCoreLibrariesIndex(context.Background(), &rpc.UpdateCoreLibrariesIndexRequest{Instance: inst},
+		output.ProgressBar(),
+		output.PrintErrorFromDownloadResult(tr("Error updating index")))
 	if err != nil {
 		feedback.Errorf(tr("Error updating core and libraries index: %v"), err)
 		os.Exit(errorcodes.ErrGeneric)

commands/daemon/daemon.go

@@ -161,13 +161,15 @@ func (s *ArduinoCoreServerImpl) Destroy(ctx context.Context, req *rpc.DestroyReq
 
 // UpdateIndex FIXMEDOC
 func (s *ArduinoCoreServerImpl) UpdateIndex(req *rpc.UpdateIndexRequest, stream rpc.ArduinoCoreService_UpdateIndexServer) error {
-	resp, err := commands.UpdateIndex(stream.Context(), req,
-		func(p *rpc.DownloadProgress) { stream.Send(&rpc.UpdateIndexResponse{DownloadProgress: p}) },
+	err := commands.UpdateIndex(stream.Context(), req,
+		func(p *rpc.DownloadProgress) {
+			stream.Send(&rpc.UpdateIndexResponse{Message: &rpc.UpdateIndexResponse_DownloadProgress{DownloadProgress: p}})
+		},
+		func(r *rpc.DownloadResult) {
+			stream.Send(&rpc.UpdateIndexResponse{Message: &rpc.UpdateIndexResponse_DownloadResult{DownloadResult: r}})
+		},
 	)
-	if err != nil {
-		return convertErrorToRPCStatus(err)
-	}
-	return stream.Send(resp)
+	return convertErrorToRPCStatus(err)
 }
 
 // UpdateLibrariesIndex FIXMEDOC
@@ -185,6 +187,7 @@ func (s *ArduinoCoreServerImpl) UpdateLibrariesIndex(req *rpc.UpdateLibrariesInd
 func (s *ArduinoCoreServerImpl) UpdateCoreLibrariesIndex(req *rpc.UpdateCoreLibrariesIndexRequest, stream rpc.ArduinoCoreService_UpdateCoreLibrariesIndexServer) error {
 	err := commands.UpdateCoreLibrariesIndex(stream.Context(), req,
 		func(p *rpc.DownloadProgress) { stream.Send(&rpc.UpdateCoreLibrariesIndexResponse{DownloadProgress: p}) },
+		func(res *rpc.DownloadResult) { /* ignore */ },
 	)
 	if err != nil {
 		return convertErrorToRPCStatus(err)

commands/instances.go

@@ -490,20 +490,29 @@ func UpdateLibrariesIndex(ctx context.Context, req *rpc.UpdateLibrariesIndexRequ
 }
 
 // UpdateIndex FIXMEDOC
-func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rpc.DownloadProgressCB) (*rpc.UpdateIndexResponse, error) {
+func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rpc.DownloadProgressCB, downloadResultCB rpc.DownloadResultCB) error {
 	if instances.GetInstance(req.GetInstance().GetId()) == nil {
-		return nil, &arduino.InvalidInstanceError{}
+		return &arduino.InvalidInstanceError{}
 	}
 
 	indexpath := configuration.DataDir(configuration.Settings)
 
 	urls := []string{globals.DefaultIndexURL}
-	urls = append(urls, configuration.Settings.GetStringSlice("board_manager.additional_urls")...)
+	if !req.GetIgnoreCustomPackageIndexes() {
+		urls = append(urls, configuration.Settings.GetStringSlice("board_manager.additional_urls")...)
+	}
+
+	failed := false
 	for _, u := range urls {
 		logrus.Info("URL: ", u)
 		URL, err := utils.URLParse(u)
 		if err != nil {
 			logrus.Warnf("unable to parse additional URL: %s", u)
+			downloadResultCB(&rpc.DownloadResult{
+				Url:   u,
+				Error: fmt.Sprintf("%s: %v", tr("Unable to parse URL"), err),
+			})
+			failed = true
 			continue
 		}
 
@@ -512,7 +521,12 @@ func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rp
 		if URL.Scheme == "file" {
 			path := paths.New(URL.Path)
 			if _, err := packageindex.LoadIndexNoSign(path); err != nil {
-				return nil, &arduino.InvalidArgumentError{Message: tr("Invalid package index in %s", path), Cause: err}
+				downloadResultCB(&rpc.DownloadResult{
+					Url:   u,
+					Error: fmt.Sprintf("%s: %v", tr("Invalid package index in %s", path), err),
+				})
+				failed = true
+				continue
 			}
 
 			fi, _ := os.Stat(path.String())
@@ -521,6 +535,10 @@ func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rp
 				TotalSize: fi.Size(),
 			})
 			downloadCB(&rpc.DownloadProgress{Completed: true})
+			downloadResultCB(&rpc.DownloadResult{
+				Url:        u,
+				Successful: true,
+			})
 			continue
 		}
 
@@ -532,18 +550,31 @@ func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rp
 			indexResource.SignatureURL.Path += ".sig"
 		}
 		if err := indexResource.Download(indexpath, downloadCB); err != nil {
-			return nil, err
+			downloadResultCB(&rpc.DownloadResult{
+				Url:   u,
+				Error: err.Error(),
+			})
+			failed = true
+			continue
 		}
+
+		downloadResultCB(&rpc.DownloadResult{
+			Url:        u,
+			Successful: true,
+		})
 	}
 
-	return &rpc.UpdateIndexResponse{}, nil
+	if failed {
+		return &arduino.FailedDownloadError{Message: tr("Some indexes could not be updated.")}
+	}
+	return nil
 }
 
 // UpdateCoreLibrariesIndex updates both Cores and Libraries indexes
-func UpdateCoreLibrariesIndex(ctx context.Context, req *rpc.UpdateCoreLibrariesIndexRequest, downloadCB rpc.DownloadProgressCB) error {
-	_, err := UpdateIndex(ctx, &rpc.UpdateIndexRequest{
+func UpdateCoreLibrariesIndex(ctx context.Context, req *rpc.UpdateCoreLibrariesIndexRequest, downloadCB rpc.DownloadProgressCB, downloadResultCB rpc.DownloadResultCB) error {
+	err := UpdateIndex(ctx, &rpc.UpdateIndexRequest{
 		Instance: req.Instance,
-	}, downloadCB)
+	}, downloadCB, downloadResultCB)
 	if err != nil {
 		return err
 	}

docs/UPGRADING.md

@@ -2,6 +2,60 @@
 
 Here you can find a list of migration guides to handle breaking changes between releases of the CLI.
 
+## 0.28.0
+
+### Breaking changes in UpdateIndex API (both gRPC and go-lang)
+
+The gRPC message `cc.arduino.cli.commands.v1.UpdateIndexResponse` has been changed from:
+
+```
+message UpdateIndexResponse {
+  // Progress of the platforms index download.
+  DownloadProgress download_progress = 1;
+}
+```
+
+to
+
+```
+message UpdateIndexResponse {
+  oneof message {
+    // Progress of the platforms index download.
+    DownloadProgress download_progress = 1;
+    // Report of the index update downloads.
+    DownloadResult download_result = 2;
+  }
+}
+
+message DownloadResult {
+  // Index URL.
+  string url = 1;
+  // Download result: true if successful, false if an error occurred.
+  bool successful = 2;
+  // Download error details.
+  string error = 3;
+}
+```
+
+even if not strictly a breaking change it's worth noting that the detailed error message is now streamed in the
+response. About the go-lang API the following functions in `github.com/arduino/arduino-cli/commands`:
+
+```go
+func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rpc.DownloadProgressCB) (*rpc.UpdateIndexResponse, error) { ... }
+func UpdateCoreLibrariesIndex(ctx context.Context, req *rpc.UpdateCoreLibrariesIndexRequest, downloadCB rpc.DownloadProgressCB) error { ... }
+```
+
+have changed their signature to:
+
+```go
+func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rpc.DownloadProgressCB, downloadResultCB rpc.DownloadResultCB) error { ... }
+func UpdateCoreLibrariesIndex(ctx context.Context, req *rpc.UpdateCoreLibrariesIndexRequest, downloadCB rpc.DownloadProgressCB, downloadResultCB rpc.DownloadResultCB) error { ... }
+```
+
+`UpdateIndex` do not return anymore the latest `UpdateIndexResponse` (it was always empty). Both `UpdateIndex` and
+`UpdateCoreLibrariesIndex` now accepts an `rpc.DownloadResultCB` to get download results, you can pass an empty callback
+if you're not interested in the error details.
+
 ## 0.27.0
 
 ### Breaking changes in golang API `github.com/arduino/arduino-cli/arduino/cores/packagemanager.PackageManager`

internal/integrationtest/arduino-cli.go

@@ -367,3 +367,14 @@ func (inst *ArduinoCLIInstance) LibraryUninstall(ctx context.Context, name, vers
 	logCallf(">>> LibraryUninstall(%+v)\n", req)
 	return installCl, err
 }
+
+// UpdateIndex calls the "UpdateIndex" gRPC method.
+func (inst *ArduinoCLIInstance) UpdateIndex(ctx context.Context, ignoreCustomPackages bool) (commands.ArduinoCoreService_UpdateIndexClient, error) {
+	req := &commands.UpdateIndexRequest{
+		Instance:                   inst.instance,
+		IgnoreCustomPackageIndexes: ignoreCustomPackages,
+	}
+	updCl, err := inst.cli.daemonClient.UpdateIndex(ctx, req)
+	logCallf(">>> UpdateIndex(%+v)\n", req)
+	return updCl, err
+}