Skip to content

Commit 7ab07f3

Browse files
cmagliecmaglie
committed
Refactored DownloadProgress
1 parent 736608f commit 7ab07f3

File tree

15 files changed

+1173
-1067
lines changed

15 files changed

+1173
-1067
lines changed

Diff for: ‎arduino/httpclient/httpclient.go

+8-9
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ 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) (downloadError error) {
3636
if config == nil {
3737
c, err := GetDownloaderConfig()
3838
if err != nil {
@@ -45,25 +45,24 @@ func DownloadFile(path *paths.Path, URL string, label string, downloadCB rpc.Dow
4545
if err != nil {
4646
return err
4747
}
48-
downloadCB(&rpc.DownloadProgress{
49-
File: label,
50-
Url: d.URL,
51-
TotalSize: d.Size(),
52-
})
53-
defer downloadCB(&rpc.DownloadProgress{Completed: true})
48+
downloadCB(rpc.NewDownloadProgressStart(d.URL, label))
5449

5550
err = d.RunAndPoll(func(downloaded int64) {
56-
downloadCB(&rpc.DownloadProgress{Downloaded: downloaded})
51+
downloadCB(rpc.NewDownloadProgressUpdate(downloaded, d.Size()))
5752
}, 250*time.Millisecond)
5853
if err != nil {
54+
downloadCB(rpc.NewDownloadProgressEnd(false, err.Error()))
5955
return err
6056
}
6157

6258
// The URL is not reachable for some reason
6359
if d.Resp.StatusCode >= 400 && d.Resp.StatusCode <= 599 {
64-
return &arduino.FailedDownloadError{Message: tr("Server responded with: %s", d.Resp.Status)}
60+
msg := tr("Server responded with: %s", d.Resp.Status)
61+
downloadCB(rpc.NewDownloadProgressEnd(false, msg))
62+
return &arduino.FailedDownloadError{Message: msg}
6563
}
6664

65+
downloadCB(rpc.NewDownloadProgressEnd(true, ""))
6766
return nil
6867
}
6968

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(rpc.NewDownloadProgressStart(r.URL, label))
48+
downloadCB(rpc.NewDownloadProgressEnd(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

+1-3
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,7 @@ func runUpdateIndexCommand(cmd *cobra.Command, args []string) {
4444
inst := instance.CreateInstanceAndRunFirstUpdate()
4545
logrus.Info("Executing `arduino-cli core update-index`")
4646

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

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

+24-29
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@ package output
1717

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

21-
"github.com/arduino/arduino-cli/cli/feedback"
2222
"github.com/arduino/arduino-cli/i18n"
2323
rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
2424
"github.com/cmaglie/pb"
@@ -41,21 +41,6 @@ func ProgressBar() rpc.DownloadProgressCB {
4141
}
4242
}
4343

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-
5944
// TaskProgress returns a TaskProgressCB that prints the task progress.
6045
// If JSON output format has been selected, the callback outputs nothing.
6146
func TaskProgress() rpc.TaskProgressCB {
@@ -70,25 +55,35 @@ func TaskProgress() rpc.TaskProgressCB {
7055
// NewDownloadProgressBarCB creates a progress bar callback that outputs a progress
7156
// bar on the terminal
7257
func NewDownloadProgressBarCB() func(*rpc.DownloadProgress) {
58+
var mux sync.Mutex
7359
var bar *pb.ProgressBar
74-
var prefix string
60+
var label string
61+
started := false
7562
return func(curr *rpc.DownloadProgress) {
63+
mux.Lock()
64+
defer mux.Unlock()
65+
7666
// 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)
67+
if start := curr.GetStart(); start != nil {
68+
label = start.GetLabel()
69+
bar = pb.New(0)
70+
bar.Prefix(label)
8571
bar.SetUnits(pb.U_BYTES)
8672
}
87-
if curr.GetDownloaded() != 0 {
88-
bar.Set(int(curr.GetDownloaded()))
73+
if update := curr.GetUpdate(); update != nil {
74+
bar.SetTotal64(update.GetTotalSize())
75+
if !started {
76+
bar.Start()
77+
started = true
78+
}
79+
bar.Set64(update.GetDownloaded())
8980
}
90-
if curr.GetCompleted() {
91-
bar.FinishPrintOver(tr("%s downloaded", prefix))
81+
if end := curr.GetEnd(); end != nil {
82+
if end.GetSuccess() && end.GetMessage() == "" {
83+
bar.FinishPrintOver(tr("%s downloaded", label))
84+
} else {
85+
bar.FinishPrintOver(end.GetMessage())
86+
}
9287
}
9388
}
9489
}

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

+16-43
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import (
1919
"context"
2020
"fmt"
2121
"net/url"
22-
"os"
2322
"strings"
2423
"sync"
2524

@@ -490,7 +489,7 @@ func UpdateLibrariesIndex(ctx context.Context, req *rpc.UpdateLibrariesIndexRequ
490489
}
491490

492491
// UpdateIndex FIXMEDOC
493-
func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rpc.DownloadProgressCB, downloadResultCB rpc.DownloadResultCB) error {
492+
func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rpc.DownloadProgressCB) error {
494493
if instances.GetInstance(req.GetInstance().GetId()) == nil {
495494
return &arduino.InvalidInstanceError{}
496495
}
@@ -504,14 +503,12 @@ func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rp
504503

505504
failed := false
506505
for _, u := range urls {
507-
logrus.Info("URL: ", u)
506+
downloadCB(rpc.NewDownloadProgressStart(u, tr("Downloading index: %s", 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(rpc.NewDownloadProgressEnd(false, msg))
515512
failed = true
516513
continue
517514
}
@@ -521,47 +518,27 @@ func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rp
521518
if URL.Scheme == "file" {
522519
path := paths.New(URL.Path)
523520
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-
})
521+
msg := fmt.Sprintf("%s: %v", tr("Invalid package index in %s", path), err)
522+
downloadCB(rpc.NewDownloadProgressEnd(false, msg))
528523
failed = true
529-
continue
524+
} else {
525+
downloadCB(rpc.NewDownloadProgressEnd(true, ""))
530526
}
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-
})
542527
continue
543528
}
544529

545-
indexResource := resources.IndexResource{
546-
URL: URL,
547-
}
530+
indexResource := resources.IndexResource{URL: URL}
548531
if strings.HasSuffix(URL.Host, "arduino.cc") && strings.HasSuffix(URL.Path, ".json") {
549532
indexResource.SignatureURL, _ = url.Parse(u) // should not fail because we already parsed it
550533
indexResource.SignatureURL.Path += ".sig"
551534
}
552535
if err := indexResource.Download(indexpath, downloadCB); err != nil {
553-
downloadResultCB(&rpc.DownloadResult{
554-
Url: u,
555-
Error: err.Error(),
556-
})
536+
downloadCB(rpc.NewDownloadProgressEnd(false, err.Error()))
557537
failed = true
558-
continue
538+
} else {
539+
downloadCB(rpc.NewDownloadProgressEnd(true, ""))
559540
}
560-
561-
downloadResultCB(&rpc.DownloadResult{
562-
Url: u,
563-
Successful: true,
564-
})
541+
continue
565542
}
566543

567544
if failed {
@@ -571,17 +548,13 @@ func UpdateIndex(ctx context.Context, req *rpc.UpdateIndexRequest, downloadCB rp
571548
}
572549

573550
// 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)
551+
func UpdateCoreLibrariesIndex(ctx context.Context, req *rpc.UpdateCoreLibrariesIndexRequest, downloadCB rpc.DownloadProgressCB) error {
552+
err := UpdateIndex(ctx, &rpc.UpdateIndexRequest{Instance: req.Instance}, downloadCB)
578553
if err != nil {
579554
return err
580555
}
581556

582-
err = UpdateLibrariesIndex(ctx, &rpc.UpdateLibrariesIndexRequest{
583-
Instance: req.Instance,
584-
}, downloadCB)
557+
err = UpdateLibrariesIndex(ctx, &rpc.UpdateLibrariesIndexRequest{Instance: req.Instance}, downloadCB)
585558
if err != nil {
586559
return err
587560
}

Diff for: ‎internal/integrationtest/daemon/daemon_core_test.go

+16-18
Original file line numberDiff line numberDiff line change
@@ -47,26 +47,26 @@ func TestDaemonCoreUpdateIndex(t *testing.T) {
4747
res, err := analyzeUpdateIndexStream(t, cl)
4848
require.NoError(t, err)
4949
require.Len(t, res, 1)
50-
require.True(t, res["https://downloads.arduino.cc/packages/package_index.tar.bz2"].Successful)
50+
require.True(t, res["https://downloads.arduino.cc/packages/package_index.tar.bz2"].Success)
5151
}
5252
{
5353
cl, err := grpcInst.UpdateIndex(context.Background(), false)
5454
require.NoError(t, err)
5555
res, err := analyzeUpdateIndexStream(t, cl)
5656
require.Error(t, err)
5757
require.Len(t, res, 3)
58-
require.True(t, res["https://downloads.arduino.cc/packages/package_index.tar.bz2"].Successful)
59-
require.True(t, res["http://arduino.esp8266.com/stable/package_esp8266com_index.json"].Successful)
60-
require.False(t, res["http://downloads.arduino.cc/package_inexistent_index.json"].Successful)
58+
require.True(t, res["https://downloads.arduino.cc/packages/package_index.tar.bz2"].Success)
59+
require.True(t, res["http://arduino.esp8266.com/stable/package_esp8266com_index.json"].Success)
60+
require.False(t, res["http://downloads.arduino.cc/package_inexistent_index.json"].Success)
6161
}
6262
}
6363

6464
// analyzeUpdateIndexStream runs an update index checking if the sequence of DownloadProgress and
6565
// DownloadResult messages is correct. It returns a map reporting all the DownloadResults messages
6666
// received (it maps urls to DownloadResults).
67-
func analyzeUpdateIndexStream(t *testing.T, cl commands.ArduinoCoreService_UpdateIndexClient) (map[string]*commands.DownloadResult, error) {
67+
func analyzeUpdateIndexStream(t *testing.T, cl commands.ArduinoCoreService_UpdateIndexClient) (map[string]*commands.DownloadProgressEnd, error) {
6868
ongoingDownload := ""
69-
results := map[string]*commands.DownloadResult{}
69+
results := map[string]*commands.DownloadProgressEnd{}
7070
for {
7171
msg, err := cl.Recv()
7272
if err == io.EOF {
@@ -78,22 +78,20 @@ func analyzeUpdateIndexStream(t *testing.T, cl commands.ArduinoCoreService_Updat
7878
require.NoError(t, err)
7979
fmt.Printf("UPDATE> %+v\n", msg)
8080
if progress := msg.GetDownloadProgress(); progress != nil {
81-
if progress.Url != "" && progress.Url != ongoingDownload {
82-
require.Empty(t, ongoingDownload, "DownloadProgress: started a download without 'completing' the previous one")
83-
ongoingDownload = progress.Url
84-
}
85-
if progress.Completed {
81+
if start := progress.GetStart(); start != nil {
82+
require.Empty(t, ongoingDownload, "DownloadProgressStart: started a download without 'completing' the previous one")
83+
ongoingDownload = start.Url
84+
} else if update := progress.GetUpdate(); update != nil {
85+
require.NotEmpty(t, ongoingDownload, "DownloadProgressUpdate: received update, but the download is not yet started...")
86+
} else if end := progress.GetEnd(); end != nil {
8687
require.NotEmpty(t, ongoingDownload, "DownloadProgress: received a 'completed' notification but never initiated a download")
88+
results[ongoingDownload] = end
8789
ongoingDownload = ""
90+
} else {
91+
require.FailNow(t, "DownloadProgress: received an empty DownloadProgress (without Start, Update or End)")
8892
}
89-
if progress.Downloaded > 0 {
90-
require.NotEmpty(t, ongoingDownload, "DownloadProgress: received a download update but never initiated a download")
91-
}
92-
} else if result := msg.GetDownloadResult(); result != nil {
93-
require.Empty(t, ongoingDownload, "DownloadResult: got a download result without completing the current download first")
94-
results[result.Url] = result
9593
} else {
96-
require.FailNow(t, "DownloadProgress: received an empty message (without a Progress or a Result)")
94+
require.FailNow(t, "DownloadProgress: received an empty message (without a DownloadProgress)")
9795
}
9896
}
9997
}

0 commit comments

Comments
 (0)