[breaking] Refactor DownloadProgress gRPC message: more clear field meaning. (#1875)

* Refactored DownloadProgress protocol

* Updated integration tests

* Do not create overly verbose errors

* Updated docs

* Update rpc/cc/arduino/cli/commands/v1/commands.proto

Co-authored-by: per1234 <accounts@perglass.com>

Co-authored-by: per1234 <accounts@perglass.com>

Author: cmaglie

arduino/cores/packagemanager/download.go

@@ -127,12 +127,7 @@ func (pme *Explorer) DownloadToolRelease(tool *cores.ToolRelease, config *downlo
 			Message: tr("Error downloading tool %s", tool),
 			Cause:   errors.New(tr("no versions available for the current OS"))}
 	}
-	if err := resource.Download(pme.DownloadDir, config, tool.String(), progressCB); err != nil {
-		return &arduino.FailedDownloadError{
-			Message: tr("Error downloading tool %s", tool),
-			Cause:   err}
-	}
-	return nil
+	return resource.Download(pme.DownloadDir, config, tool.String(), progressCB)
 }
 
 // DownloadPlatformRelease downloads a PlatformRelease. If the platform is already downloaded a

arduino/httpclient/httpclient.go

@@ -32,7 +32,16 @@ var tr = i18n.Tr
 
 // DownloadFile downloads a file from a URL into the specified path. An optional config and options may be passed (or nil to use the defaults).
 // A DownloadProgressCB callback function must be passed to monitor download progress.
-func DownloadFile(path *paths.Path, URL string, label string, downloadCB rpc.DownloadProgressCB, config *downloader.Config, options ...downloader.DownloadOptions) error {
+func DownloadFile(path *paths.Path, URL string, label string, downloadCB rpc.DownloadProgressCB, config *downloader.Config, options ...downloader.DownloadOptions) (returnedError error) {
+	downloadCB.Start(URL, label)
+	defer func() {
+		if returnedError == nil {
+			downloadCB.End(true, "")
+		} else {
+			downloadCB.End(false, returnedError.Error())
+		}
+	}()
+
 	if config == nil {
 		c, err := GetDownloaderConfig()
 		if err != nil {
@@ -45,23 +54,18 @@ func DownloadFile(path *paths.Path, URL string, label string, downloadCB rpc.Dow
 	if err != nil {
 		return err
 	}
-	downloadCB(&rpc.DownloadProgress{
-		File:      label,
-		Url:       d.URL,
-		TotalSize: d.Size(),
-	})
-	defer downloadCB(&rpc.DownloadProgress{Completed: true})
 
 	err = d.RunAndPoll(func(downloaded int64) {
-		downloadCB(&rpc.DownloadProgress{Downloaded: downloaded})
+		downloadCB.Update(downloaded, d.Size())
 	}, 250*time.Millisecond)
 	if err != nil {
 		return err
 	}
 
 	// The URL is not reachable for some reason
 	if d.Resp.StatusCode >= 400 && d.Resp.StatusCode <= 599 {
-		return &arduino.FailedDownloadError{Message: tr("Server responded with: %s", d.Resp.Status)}
+		msg := tr("Server responded with: %s", d.Resp.Status)
+		return &arduino.FailedDownloadError{Message: msg}
 	}
 
 	return nil

arduino/resources/download.go

@@ -44,12 +44,8 @@ func (r *DownloadResource) Download(downloadDir *paths.Path, config *downloader.
 			}
 		} else {
 			// File is cached, nothing to do here
-
-			// This signal means that the file is already downloaded
-			downloadCB(&rpc.DownloadProgress{
-				File:      label,
-				Completed: true,
-			})
+			downloadCB.Start(r.URL, label)
+			downloadCB.End(true, tr("%s already downloaded", label))
 			return nil
 		}
 	} else {

cli/core/search.go

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

cli/core/update_index.go

@@ -20,6 +20,7 @@ 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"
@@ -44,10 +45,9 @@ 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(),
-		output.PrintErrorFromDownloadResult(tr("Error updating index")))
+	err := commands.UpdateIndex(context.Background(), &rpc.UpdateIndexRequest{Instance: inst}, output.ProgressBar())
 	if err != nil {
+		feedback.Error(err)
 		os.Exit(errorcodes.ErrGeneric)
 	}
 }

cli/instance/instance.go

@@ -157,8 +157,7 @@ func FirstUpdate(instance *rpc.Instance) error {
 				Instance:                   instance,
 				IgnoreCustomPackageIndexes: true,
 			},
-			output.ProgressBar(),
-			output.PrintErrorFromDownloadResult(tr("Error updating index")))
+			output.ProgressBar())
 		if err != nil {
 			return err
 		}

cli/output/rpc_progress.go

@@ -17,6 +17,7 @@ package output
 
 import (
 	"fmt"
+	"sync"
 
 	"github.com/arduino/arduino-cli/cli/feedback"
 	"github.com/arduino/arduino-cli/i18n"
@@ -41,21 +42,6 @@ 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 {
@@ -70,25 +56,39 @@ func TaskProgress() rpc.TaskProgressCB {
 // NewDownloadProgressBarCB creates a progress bar callback that outputs a progress
 // bar on the terminal
 func NewDownloadProgressBarCB() func(*rpc.DownloadProgress) {
+	var mux sync.Mutex
 	var bar *pb.ProgressBar
-	var prefix string
+	var label string
+	started := false
 	return func(curr *rpc.DownloadProgress) {
+		mux.Lock()
+		defer mux.Unlock()
+
 		// fmt.Printf(">>> %v\n", curr)
-		if filename := curr.GetFile(); filename != "" {
-			if curr.GetCompleted() {
-				fmt.Println(tr("%s already downloaded", filename))
-				return
-			}
-			prefix = filename
-			bar = pb.StartNew(int(curr.GetTotalSize()))
-			bar.Prefix(prefix)
+		if start := curr.GetStart(); start != nil {
+			label = start.GetLabel()
+			bar = pb.New(0)
+			bar.Prefix(label)
 			bar.SetUnits(pb.U_BYTES)
 		}
-		if curr.GetDownloaded() != 0 {
-			bar.Set(int(curr.GetDownloaded()))
+		if update := curr.GetUpdate(); update != nil {
+			bar.SetTotal64(update.GetTotalSize())
+			if !started {
+				bar.Start()
+				started = true
+			}
+			bar.Set64(update.GetDownloaded())
 		}
-		if curr.GetCompleted() {
-			bar.FinishPrintOver(tr("%s downloaded", prefix))
+		if end := curr.GetEnd(); end != nil {
+			msg := end.GetMessage()
+			if end.GetSuccess() && msg == "" {
+				msg = tr("downloaded")
+			}
+			if started {
+				bar.FinishPrintOver(label + " " + msg)
+			} else {
+				feedback.Print(label + " " + msg)
+			}
 		}
 	}
 }

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(),
-		output.PrintErrorFromDownloadResult(tr("Error updating index")))
+	err := commands.UpdateCoreLibrariesIndex(context.Background(),
+		&rpc.UpdateCoreLibrariesIndexRequest{Instance: inst},
+		output.ProgressBar())
 	if err != nil {
 		feedback.Errorf(tr("Error updating core and libraries index: %v"), err)
 		os.Exit(errorcodes.ErrGeneric)

commands/daemon/daemon.go

@@ -162,12 +162,7 @@ func (s *ArduinoCoreServerImpl) Destroy(ctx context.Context, req *rpc.DestroyReq
 // UpdateIndex FIXMEDOC
 func (s *ArduinoCoreServerImpl) UpdateIndex(req *rpc.UpdateIndexRequest, stream rpc.ArduinoCoreService_UpdateIndexServer) error {
 	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}})
-		},
+		func(p *rpc.DownloadProgress) { stream.Send(&rpc.UpdateIndexResponse{DownloadProgress: p}) },
 	)
 	return convertErrorToRPCStatus(err)
 }
@@ -187,7 +182,6 @@ 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

@@ -19,7 +19,7 @@ import (
 	"context"
 	"fmt"
 	"net/url"
-	"os"
+	"path/filepath"
 	"strings"
 	"sync"
 
@@ -490,7 +490,7 @@ func UpdateLibrariesIndex(ctx context.Context, req *rpc.UpdateLibrariesIndexRequ
 }
 
 // UpdateIndex FIXMEDOC
-func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rpc.DownloadProgressCB, downloadResultCB rpc.DownloadResultCB) error {
+func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rpc.DownloadProgressCB) error {
 	if instances.GetInstance(req.GetInstance().GetId()) == nil {
 		return &arduino.InvalidInstanceError{}
 	}
@@ -504,64 +504,39 @@ func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rp
 
 	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),
-			})
+			msg := fmt.Sprintf("%s: %v", tr("Unable to parse URL"), err)
+			downloadCB.Start(u, tr("Downloading index: %s", u))
+			downloadCB.End(false, msg)
 			failed = true
 			continue
 		}
 
 		logrus.WithField("url", URL).Print("Updating index")
 
 		if URL.Scheme == "file" {
+			downloadCB.Start(u, tr("Downloading index: %s", filepath.Base(URL.Path)))
 			path := paths.New(URL.Path)
 			if _, err := packageindex.LoadIndexNoSign(path); err != nil {
-				downloadResultCB(&rpc.DownloadResult{
-					Url:   u,
-					Error: fmt.Sprintf("%s: %v", tr("Invalid package index in %s", path), err),
-				})
+				msg := fmt.Sprintf("%s: %v", tr("Invalid package index in %s", path), err)
+				downloadCB.End(false, msg)
 				failed = true
-				continue
+			} else {
+				downloadCB.End(true, "")
 			}
-
-			fi, _ := os.Stat(path.String())
-			downloadCB(&rpc.DownloadProgress{
-				File:      tr("Downloading index: %s", path.Base()),
-				TotalSize: fi.Size(),
-			})
-			downloadCB(&rpc.DownloadProgress{Completed: true})
-			downloadResultCB(&rpc.DownloadResult{
-				Url:        u,
-				Successful: true,
-			})
 			continue
 		}
 
-		indexResource := resources.IndexResource{
-			URL: URL,
-		}
+		indexResource := resources.IndexResource{URL: URL}
 		if strings.HasSuffix(URL.Host, "arduino.cc") && strings.HasSuffix(URL.Path, ".json") {
 			indexResource.SignatureURL, _ = url.Parse(u) // should not fail because we already parsed it
 			indexResource.SignatureURL.Path += ".sig"
 		}
 		if err := indexResource.Download(indexpath, downloadCB); err != nil {
-			downloadResultCB(&rpc.DownloadResult{
-				Url:   u,
-				Error: err.Error(),
-			})
 			failed = true
-			continue
 		}
-
-		downloadResultCB(&rpc.DownloadResult{
-			Url:        u,
-			Successful: true,
-		})
 	}
 
 	if failed {
@@ -571,17 +546,13 @@ func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rp
 }
 
 // UpdateCoreLibrariesIndex updates both Cores and Libraries indexes
-func UpdateCoreLibrariesIndex(ctx context.Context, req *rpc.UpdateCoreLibrariesIndexRequest, downloadCB rpc.DownloadProgressCB, downloadResultCB rpc.DownloadResultCB) error {
-	err := UpdateIndex(ctx, &rpc.UpdateIndexRequest{
-		Instance: req.Instance,
-	}, downloadCB, downloadResultCB)
+func UpdateCoreLibrariesIndex(ctx context.Context, req *rpc.UpdateCoreLibrariesIndexRequest, downloadCB rpc.DownloadProgressCB) error {
+	err := UpdateIndex(ctx, &rpc.UpdateIndexRequest{Instance: req.Instance}, downloadCB)
 	if err != nil {
 		return err
 	}
 
-	err = UpdateLibrariesIndex(ctx, &rpc.UpdateLibrariesIndexRequest{
-		Instance: req.Instance,
-	}, downloadCB)
+	err = UpdateLibrariesIndex(ctx, &rpc.UpdateLibrariesIndexRequest{Instance: req.Instance}, downloadCB)
 	if err != nil {
 		return err
 	}