Skip to content

Commit c8ff042

Browse files
cmaglieper1234
cmaglie
and
per1234
authored
[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>
1 parent bcb69d9 commit c8ff042

File tree

20 files changed

+1320
-1133
lines changed

20 files changed

+1320
-1133
lines changed

Diff for: ‎arduino/cores/packagemanager/download.go

+1-6
Original file line numberDiff line numberDiff line change
@@ -127,12 +127,7 @@ func (pme *Explorer) DownloadToolRelease(tool *cores.ToolRelease, config *downlo
127127
Message: tr("Error downloading tool %s", tool),
128128
Cause: errors.New(tr("no versions available for the current OS"))}
129129
}
130-
if err := resource.Download(pme.DownloadDir, config, tool.String(), progressCB); err != nil {
131-
return &arduino.FailedDownloadError{
132-
Message: tr("Error downloading tool %s", tool),
133-
Cause: err}
134-
}
135-
return nil
130+
return resource.Download(pme.DownloadDir, config, tool.String(), progressCB)
136131
}
137132

138133
// DownloadPlatformRelease downloads a PlatformRelease. If the platform is already downloaded a

Diff for: ‎arduino/httpclient/httpclient.go

+13-9
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,16 @@ var tr = i18n.Tr
3232

3333
// 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).
3434
// A DownloadProgressCB callback function must be passed to monitor download progress.
35-
func DownloadFile(path *paths.Path, URL string, label string, downloadCB rpc.DownloadProgressCB, config *downloader.Config, options ...downloader.DownloadOptions) error {
35+
func DownloadFile(path *paths.Path, URL string, label string, downloadCB rpc.DownloadProgressCB, config *downloader.Config, options ...downloader.DownloadOptions) (returnedError error) {
36+
downloadCB.Start(URL, label)
37+
defer func() {
38+
if returnedError == nil {
39+
downloadCB.End(true, "")
40+
} else {
41+
downloadCB.End(false, returnedError.Error())
42+
}
43+
}()
44+
3645
if config == nil {
3746
c, err := GetDownloaderConfig()
3847
if err != nil {
@@ -45,23 +54,18 @@ func DownloadFile(path *paths.Path, URL string, label string, downloadCB rpc.Dow
4554
if err != nil {
4655
return err
4756
}
48-
downloadCB(&rpc.DownloadProgress{
49-
File: label,
50-
Url: d.URL,
51-
TotalSize: d.Size(),
52-
})
53-
defer downloadCB(&rpc.DownloadProgress{Completed: true})
5457

5558
err = d.RunAndPoll(func(downloaded int64) {
56-
downloadCB(&rpc.DownloadProgress{Downloaded: downloaded})
59+
downloadCB.Update(downloaded, d.Size())
5760
}, 250*time.Millisecond)
5861
if err != nil {
5962
return err
6063
}
6164

6265
// The URL is not reachable for some reason
6366
if d.Resp.StatusCode >= 400 && d.Resp.StatusCode <= 599 {
64-
return &arduino.FailedDownloadError{Message: tr("Server responded with: %s", d.Resp.Status)}
67+
msg := tr("Server responded with: %s", d.Resp.Status)
68+
return &arduino.FailedDownloadError{Message: msg}
6569
}
6670

6771
return nil

Diff for: ‎arduino/resources/download.go

+2-6
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,8 @@ func (r *DownloadResource) Download(downloadDir *paths.Path, config *downloader.
4444
}
4545
} else {
4646
// File is cached, nothing to do here
47-
48-
// This signal means that the file is already downloaded
49-
downloadCB(&rpc.DownloadProgress{
50-
File: label,
51-
Completed: true,
52-
})
47+
downloadCB.Start(r.URL, label)
48+
downloadCB.End(true, tr("%s already downloaded", label))
5349
return nil
5450
}
5551
} else {

Diff for: ‎cli/core/search.go

+1-3
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,7 @@ func runSearchCommand(cmd *cobra.Command, args []string) {
6767
}
6868

6969
if indexesNeedUpdating(indexUpdateInterval) {
70-
err := commands.UpdateIndex(context.Background(), &rpc.UpdateIndexRequest{Instance: inst},
71-
output.ProgressBar(),
72-
output.PrintErrorFromDownloadResult(tr("Error updating index")))
70+
err := commands.UpdateIndex(context.Background(), &rpc.UpdateIndexRequest{Instance: inst}, output.ProgressBar())
7371
if err != nil {
7472
os.Exit(errorcodes.ErrGeneric)
7573
}

Diff for: ‎cli/core/update_index.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"os"
2121

2222
"github.com/arduino/arduino-cli/cli/errorcodes"
23+
"github.com/arduino/arduino-cli/cli/feedback"
2324
"github.com/arduino/arduino-cli/cli/instance"
2425
"github.com/arduino/arduino-cli/cli/output"
2526
"github.com/arduino/arduino-cli/commands"
@@ -44,10 +45,9 @@ func runUpdateIndexCommand(cmd *cobra.Command, args []string) {
4445
inst := instance.CreateInstanceAndRunFirstUpdate()
4546
logrus.Info("Executing `arduino-cli core update-index`")
4647

47-
err := commands.UpdateIndex(context.Background(), &rpc.UpdateIndexRequest{Instance: inst},
48-
output.ProgressBar(),
49-
output.PrintErrorFromDownloadResult(tr("Error updating index")))
48+
err := commands.UpdateIndex(context.Background(), &rpc.UpdateIndexRequest{Instance: inst}, output.ProgressBar())
5049
if err != nil {
50+
feedback.Error(err)
5151
os.Exit(errorcodes.ErrGeneric)
5252
}
5353
}

Diff for: ‎cli/instance/instance.go

+1-2
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,7 @@ func FirstUpdate(instance *rpc.Instance) error {
157157
Instance: instance,
158158
IgnoreCustomPackageIndexes: true,
159159
},
160-
output.ProgressBar(),
161-
output.PrintErrorFromDownloadResult(tr("Error updating index")))
160+
output.ProgressBar())
162161
if err != nil {
163162
return err
164163
}

Diff for: ‎cli/output/rpc_progress.go

+28-28
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package output
1717

1818
import (
1919
"fmt"
20+
"sync"
2021

2122
"github.com/arduino/arduino-cli/cli/feedback"
2223
"github.com/arduino/arduino-cli/i18n"
@@ -41,21 +42,6 @@ func ProgressBar() rpc.DownloadProgressCB {
4142
}
4243
}
4344

44-
// PrintErrorFromDownloadResult returns a DownloadResultCB that only prints
45-
// the errors with the give message prefixed.
46-
func PrintErrorFromDownloadResult(msg string) rpc.DownloadResultCB {
47-
if OutputFormat != "json" {
48-
return func(res *rpc.DownloadResult) {
49-
if !res.GetSuccessful() {
50-
feedback.Errorf("%s: %s", msg, res.GetError())
51-
}
52-
}
53-
}
54-
return func(res *rpc.DownloadResult) {
55-
// XXX: Output result in JSON?
56-
}
57-
}
58-
5945
// TaskProgress returns a TaskProgressCB that prints the task progress.
6046
// If JSON output format has been selected, the callback outputs nothing.
6147
func TaskProgress() rpc.TaskProgressCB {
@@ -70,25 +56,39 @@ func TaskProgress() rpc.TaskProgressCB {
7056
// NewDownloadProgressBarCB creates a progress bar callback that outputs a progress
7157
// bar on the terminal
7258
func NewDownloadProgressBarCB() func(*rpc.DownloadProgress) {
59+
var mux sync.Mutex
7360
var bar *pb.ProgressBar
74-
var prefix string
61+
var label string
62+
started := false
7563
return func(curr *rpc.DownloadProgress) {
64+
mux.Lock()
65+
defer mux.Unlock()
66+
7667
// fmt.Printf(">>> %v\n", curr)
77-
if filename := curr.GetFile(); filename != "" {
78-
if curr.GetCompleted() {
79-
fmt.Println(tr("%s already downloaded", filename))
80-
return
81-
}
82-
prefix = filename
83-
bar = pb.StartNew(int(curr.GetTotalSize()))
84-
bar.Prefix(prefix)
68+
if start := curr.GetStart(); start != nil {
69+
label = start.GetLabel()
70+
bar = pb.New(0)
71+
bar.Prefix(label)
8572
bar.SetUnits(pb.U_BYTES)
8673
}
87-
if curr.GetDownloaded() != 0 {
88-
bar.Set(int(curr.GetDownloaded()))
74+
if update := curr.GetUpdate(); update != nil {
75+
bar.SetTotal64(update.GetTotalSize())
76+
if !started {
77+
bar.Start()
78+
started = true
79+
}
80+
bar.Set64(update.GetDownloaded())
8981
}
90-
if curr.GetCompleted() {
91-
bar.FinishPrintOver(tr("%s downloaded", prefix))
82+
if end := curr.GetEnd(); end != nil {
83+
msg := end.GetMessage()
84+
if end.GetSuccess() && msg == "" {
85+
msg = tr("downloaded")
86+
}
87+
if started {
88+
bar.FinishPrintOver(label + " " + msg)
89+
} else {
90+
feedback.Print(label + " " + msg)
91+
}
9292
}
9393
}
9494
}

Diff for: ‎cli/update/update.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,9 @@ func runUpdateCommand(cmd *cobra.Command, args []string) {
5656
inst := instance.CreateInstanceAndRunFirstUpdate()
5757
logrus.Info("Executing `arduino-cli update`")
5858

59-
err := commands.UpdateCoreLibrariesIndex(context.Background(), &rpc.UpdateCoreLibrariesIndexRequest{Instance: inst},
60-
output.ProgressBar(),
61-
output.PrintErrorFromDownloadResult(tr("Error updating index")))
59+
err := commands.UpdateCoreLibrariesIndex(context.Background(),
60+
&rpc.UpdateCoreLibrariesIndexRequest{Instance: inst},
61+
output.ProgressBar())
6262
if err != nil {
6363
feedback.Errorf(tr("Error updating core and libraries index: %v"), err)
6464
os.Exit(errorcodes.ErrGeneric)

Diff for: ‎commands/daemon/daemon.go

+1-7
Original file line numberDiff line numberDiff line change
@@ -162,12 +162,7 @@ func (s *ArduinoCoreServerImpl) Destroy(ctx context.Context, req *rpc.DestroyReq
162162
// UpdateIndex FIXMEDOC
163163
func (s *ArduinoCoreServerImpl) UpdateIndex(req *rpc.UpdateIndexRequest, stream rpc.ArduinoCoreService_UpdateIndexServer) error {
164164
err := commands.UpdateIndex(stream.Context(), req,
165-
func(p *rpc.DownloadProgress) {
166-
stream.Send(&rpc.UpdateIndexResponse{Message: &rpc.UpdateIndexResponse_DownloadProgress{DownloadProgress: p}})
167-
},
168-
func(r *rpc.DownloadResult) {
169-
stream.Send(&rpc.UpdateIndexResponse{Message: &rpc.UpdateIndexResponse_DownloadResult{DownloadResult: r}})
170-
},
165+
func(p *rpc.DownloadProgress) { stream.Send(&rpc.UpdateIndexResponse{DownloadProgress: p}) },
171166
)
172167
return convertErrorToRPCStatus(err)
173168
}
@@ -187,7 +182,6 @@ func (s *ArduinoCoreServerImpl) UpdateLibrariesIndex(req *rpc.UpdateLibrariesInd
187182
func (s *ArduinoCoreServerImpl) UpdateCoreLibrariesIndex(req *rpc.UpdateCoreLibrariesIndexRequest, stream rpc.ArduinoCoreService_UpdateCoreLibrariesIndexServer) error {
188183
err := commands.UpdateCoreLibrariesIndex(stream.Context(), req,
189184
func(p *rpc.DownloadProgress) { stream.Send(&rpc.UpdateCoreLibrariesIndexResponse{DownloadProgress: p}) },
190-
func(res *rpc.DownloadResult) { /* ignore */ },
191185
)
192186
if err != nil {
193187
return convertErrorToRPCStatus(err)

Diff for: ‎commands/instances.go

+14-43
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import (
1919
"context"
2020
"fmt"
2121
"net/url"
22-
"os"
22+
"path/filepath"
2323
"strings"
2424
"sync"
2525

@@ -490,7 +490,7 @@ func UpdateLibrariesIndex(ctx context.Context, req *rpc.UpdateLibrariesIndexRequ
490490
}
491491

492492
// UpdateIndex FIXMEDOC
493-
func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rpc.DownloadProgressCB, downloadResultCB rpc.DownloadResultCB) error {
493+
func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rpc.DownloadProgressCB) error {
494494
if instances.GetInstance(req.GetInstance().GetId()) == nil {
495495
return &arduino.InvalidInstanceError{}
496496
}
@@ -504,64 +504,39 @@ func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rp
504504

505505
failed := false
506506
for _, u := range urls {
507-
logrus.Info("URL: ", u)
508507
URL, err := utils.URLParse(u)
509508
if err != nil {
510509
logrus.Warnf("unable to parse additional URL: %s", u)
511-
downloadResultCB(&rpc.DownloadResult{
512-
Url: u,
513-
Error: fmt.Sprintf("%s: %v", tr("Unable to parse URL"), err),
514-
})
510+
msg := fmt.Sprintf("%s: %v", tr("Unable to parse URL"), err)
511+
downloadCB.Start(u, tr("Downloading index: %s", u))
512+
downloadCB.End(false, msg)
515513
failed = true
516514
continue
517515
}
518516

519517
logrus.WithField("url", URL).Print("Updating index")
520518

521519
if URL.Scheme == "file" {
520+
downloadCB.Start(u, tr("Downloading index: %s", filepath.Base(URL.Path)))
522521
path := paths.New(URL.Path)
523522
if _, err := packageindex.LoadIndexNoSign(path); err != nil {
524-
downloadResultCB(&rpc.DownloadResult{
525-
Url: u,
526-
Error: fmt.Sprintf("%s: %v", tr("Invalid package index in %s", path), err),
527-
})
523+
msg := fmt.Sprintf("%s: %v", tr("Invalid package index in %s", path), err)
524+
downloadCB.End(false, msg)
528525
failed = true
529-
continue
526+
} else {
527+
downloadCB.End(true, "")
530528
}
531-
532-
fi, _ := os.Stat(path.String())
533-
downloadCB(&rpc.DownloadProgress{
534-
File: tr("Downloading index: %s", path.Base()),
535-
TotalSize: fi.Size(),
536-
})
537-
downloadCB(&rpc.DownloadProgress{Completed: true})
538-
downloadResultCB(&rpc.DownloadResult{
539-
Url: u,
540-
Successful: true,
541-
})
542529
continue
543530
}
544531

545-
indexResource := resources.IndexResource{
546-
URL: URL,
547-
}
532+
indexResource := resources.IndexResource{URL: URL}
548533
if strings.HasSuffix(URL.Host, "arduino.cc") && strings.HasSuffix(URL.Path, ".json") {
549534
indexResource.SignatureURL, _ = url.Parse(u) // should not fail because we already parsed it
550535
indexResource.SignatureURL.Path += ".sig"
551536
}
552537
if err := indexResource.Download(indexpath, downloadCB); err != nil {
553-
downloadResultCB(&rpc.DownloadResult{
554-
Url: u,
555-
Error: err.Error(),
556-
})
557538
failed = true
558-
continue
559539
}
560-
561-
downloadResultCB(&rpc.DownloadResult{
562-
Url: u,
563-
Successful: true,
564-
})
565540
}
566541

567542
if failed {
@@ -571,17 +546,13 @@ func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rp
571546
}
572547

573548
// UpdateCoreLibrariesIndex updates both Cores and Libraries indexes
574-
func UpdateCoreLibrariesIndex(ctx context.Context, req *rpc.UpdateCoreLibrariesIndexRequest, downloadCB rpc.DownloadProgressCB, downloadResultCB rpc.DownloadResultCB) error {
575-
err := UpdateIndex(ctx, &rpc.UpdateIndexRequest{
576-
Instance: req.Instance,
577-
}, downloadCB, downloadResultCB)
549+
func UpdateCoreLibrariesIndex(ctx context.Context, req *rpc.UpdateCoreLibrariesIndexRequest, downloadCB rpc.DownloadProgressCB) error {
550+
err := UpdateIndex(ctx, &rpc.UpdateIndexRequest{Instance: req.Instance}, downloadCB)
578551
if err != nil {
579552
return err
580553
}
581554

582-
err = UpdateLibrariesIndex(ctx, &rpc.UpdateLibrariesIndexRequest{
583-
Instance: req.Instance,
584-
}, downloadCB)
555+
err = UpdateLibrariesIndex(ctx, &rpc.UpdateLibrariesIndexRequest{Instance: req.Instance}, downloadCB)
585556
if err != nil {
586557
return err
587558
}

0 commit comments

Comments
 (0)