Skip to content

[skip-changelog] Add query parameters to urls generated by lib commands #2034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions arduino/cores/packagemanager/download.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +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"))}
}
return resource.Download(pme.DownloadDir, config, tool.String(), progressCB)
return resource.Download(pme.DownloadDir, config, tool.String(), progressCB, "")
}

// DownloadPlatformRelease downloads a PlatformRelease. If the platform is already downloaded a
Expand All @@ -136,5 +136,5 @@ func (pme *Explorer) DownloadPlatformRelease(platform *cores.PlatformRelease, co
if platform.Resource == nil {
return &arduino.PlatformNotFoundError{Platform: platform.String()}
}
return platform.Resource.Download(pme.DownloadDir, config, platform.String(), progressCB)
return platform.Resource.Download(pme.DownloadDir, config, platform.String(), progressCB, "")
}
2 changes: 1 addition & 1 deletion arduino/cores/packagemanager/profiles.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ func (pmb *Builder) installMissingProfileTool(toolRelease *cores.ToolRelease, de
return &arduino.InvalidVersionError{Cause: fmt.Errorf(tr("version %s not available for this operating system", toolRelease))}
}
taskCB(&rpc.TaskProgress{Name: tr("Downloading tool %s", toolRelease)})
if err := toolResource.Download(pmb.DownloadDir, nil, toolRelease.String(), downloadCB); err != nil {
if err := toolResource.Download(pmb.DownloadDir, nil, toolRelease.String(), downloadCB, ""); err != nil {
taskCB(&rpc.TaskProgress{Name: tr("Error downloading tool %s", toolRelease)})
return &arduino.FailedInstallError{Message: tr("Error installing tool %s", toolRelease), Cause: err}
}
Expand Down
8 changes: 7 additions & 1 deletion arduino/httpclient/httpclient.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,20 @@ import (
"github.com/arduino/arduino-cli/i18n"
rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
"github.com/arduino/go-paths-helper"
"github.com/sirupsen/logrus"
"go.bug.st/downloader/v2"
)

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) (returnedError error) {
// If a not empty queryParameter is passed, it is appended to the URL for analysis purposes.
func DownloadFile(path *paths.Path, URL string, queryParameter string, label string, downloadCB rpc.DownloadProgressCB, config *downloader.Config, options ...downloader.DownloadOptions) (returnedError error) {
if queryParameter != "" {
URL = URL + "?query=" + queryParameter
}
logrus.WithField("url", URL).Info("Starting download")
downloadCB.Start(URL, label)
defer func() {
if returnedError == nil {
Expand Down
5 changes: 3 additions & 2 deletions arduino/resources/download.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ import (

// Download performs a download loop using the provided downloader.Config.
// Messages are passed back to the DownloadProgressCB using label as text for the File field.
func (r *DownloadResource) Download(downloadDir *paths.Path, config *downloader.Config, label string, downloadCB rpc.DownloadProgressCB) error {
// queryParameter is passed for analysis purposes.
func (r *DownloadResource) Download(downloadDir *paths.Path, config *downloader.Config, label string, downloadCB rpc.DownloadProgressCB, queryParameter string) error {
path, err := r.ArchivePath(downloadDir)
if err != nil {
return fmt.Errorf(tr("getting archive path: %s"), err)
Expand All @@ -51,5 +52,5 @@ func (r *DownloadResource) Download(downloadDir *paths.Path, config *downloader.
} else {
return fmt.Errorf(tr("getting archive file info: %s"), err)
}
return httpclient.DownloadFile(path, r.URL, label, downloadCB, config)
return httpclient.DownloadFile(path, r.URL, queryParameter, label, downloadCB, config)
}
2 changes: 1 addition & 1 deletion arduino/resources/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func TestDownloadApplyUserAgentHeaderUsingConfig(t *testing.T) {

httpClient := httpclient.NewWithConfig(&httpclient.Config{UserAgent: goldUserAgentValue})

err = r.Download(tmp, &downloader.Config{HttpClient: *httpClient}, "", func(progress *rpc.DownloadProgress) {})
err = r.Download(tmp, &downloader.Config{HttpClient: *httpClient}, "", func(progress *rpc.DownloadProgress) {}, "")
require.NoError(t, err)

// leverage the download helper to download the echo for the request made by the downloader itself
Expand Down
4 changes: 2 additions & 2 deletions arduino/resources/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func (res *IndexResource) Download(destDir *paths.Path, downloadCB rpc.DownloadP
// Download index file
indexFileName := path.Base(res.URL.Path) // == package_index.json[.gz]
tmpIndexPath := tmp.Join(indexFileName)
if err := httpclient.DownloadFile(tmpIndexPath, res.URL.String(), tr("Downloading index: %s", indexFileName), downloadCB, nil, downloader.NoResume); err != nil {
if err := httpclient.DownloadFile(tmpIndexPath, res.URL.String(), "", tr("Downloading index: %s", indexFileName), downloadCB, nil, downloader.NoResume); err != nil {
return &arduino.FailedDownloadError{Message: tr("Error downloading index '%s'", res.URL), Cause: err}
}

Expand Down Expand Up @@ -112,7 +112,7 @@ func (res *IndexResource) Download(destDir *paths.Path, downloadCB rpc.DownloadP
// Download signature
signaturePath = destDir.Join(signatureFileName)
tmpSignaturePath = tmp.Join(signatureFileName)
if err := httpclient.DownloadFile(tmpSignaturePath, res.SignatureURL.String(), tr("Downloading index signature: %s", signatureFileName), downloadCB, nil, downloader.NoResume); err != nil {
if err := httpclient.DownloadFile(tmpSignaturePath, res.SignatureURL.String(), "", tr("Downloading index signature: %s", signatureFileName), downloadCB, nil, downloader.NoResume); err != nil {
return &arduino.FailedDownloadError{Message: tr("Error downloading index signature '%s'", res.SignatureURL), Cause: err}
}

Expand Down
4 changes: 2 additions & 2 deletions arduino/resources/resources_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func TestDownloadAndChecksums(t *testing.T) {
require.NoError(t, err)

downloadAndTestChecksum := func() {
err := r.Download(tmp, &downloader.Config{}, "", func(*rpc.DownloadProgress) {})
err := r.Download(tmp, &downloader.Config{}, "", func(*rpc.DownloadProgress) {}, "")
require.NoError(t, err)

data, err := testFile.ReadFile()
Expand All @@ -62,7 +62,7 @@ func TestDownloadAndChecksums(t *testing.T) {
downloadAndTestChecksum()

// Download with cached file
err = r.Download(tmp, &downloader.Config{}, "", func(*rpc.DownloadProgress) {})
err = r.Download(tmp, &downloader.Config{}, "", func(*rpc.DownloadProgress) {}, "")
require.NoError(t, err)

// Download if cached file has data in excess (redownload)
Expand Down
2 changes: 2 additions & 0 deletions commands/daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,7 @@ func (s *ArduinoCoreServerImpl) LibraryDownload(req *rpc.LibraryDownloadRequest,
resp, err := lib.LibraryDownload(
stream.Context(), req,
func(p *rpc.DownloadProgress) { stream.Send(&rpc.LibraryDownloadResponse{Progress: p}) },
"",
)
if err != nil {
return convertErrorToRPCStatus(err)
Expand All @@ -356,6 +357,7 @@ func (s *ArduinoCoreServerImpl) LibraryInstall(req *rpc.LibraryInstallRequest, s
stream.Context(), req,
func(p *rpc.DownloadProgress) { stream.Send(&rpc.LibraryInstallResponse{Progress: p}) },
func(p *rpc.TaskProgress) { stream.Send(&rpc.LibraryInstallResponse{TaskProgress: p}) },
"",
)
if err != nil {
return convertErrorToRPCStatus(err)
Expand Down
2 changes: 1 addition & 1 deletion commands/instances.go
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro
responseError(err.ToRPCStatus())
continue
}
if err := libRelease.Resource.Download(lm.DownloadsDir, nil, libRelease.String(), downloadCallback); err != nil {
if err := libRelease.Resource.Download(lm.DownloadsDir, nil, libRelease.String(), downloadCallback, ""); err != nil {
taskCallback(&rpc.TaskProgress{Name: tr("Error downloading library %s", libraryRef)})
e := &arduino.FailedLibraryInstallError{Cause: err}
responseError(e.ToRPCStatus())
Expand Down
12 changes: 7 additions & 5 deletions commands/lib/download.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ import (

var tr = i18n.Tr

// LibraryDownload FIXMEDOC
func LibraryDownload(ctx context.Context, req *rpc.LibraryDownloadRequest, downloadCB rpc.DownloadProgressCB) (*rpc.LibraryDownloadResponse, error) {
// LibraryDownload executes the download of the library.
// A DownloadProgressCB callback function must be passed to monitor download progress.
// queryParameter is passed for analysis purposes.
func LibraryDownload(ctx context.Context, req *rpc.LibraryDownloadRequest, downloadCB rpc.DownloadProgressCB, queryParameter string) (*rpc.LibraryDownloadResponse, error) {
logrus.Info("Executing `arduino-cli lib download`")

lm := commands.GetLibraryManager(req)
Expand All @@ -46,22 +48,22 @@ func LibraryDownload(ctx context.Context, req *rpc.LibraryDownloadRequest, downl
return nil, err
}

if err := downloadLibrary(lm, lib, downloadCB, func(*rpc.TaskProgress) {}); err != nil {
if err := downloadLibrary(lm, lib, downloadCB, func(*rpc.TaskProgress) {}, queryParameter); err != nil {
return nil, err
}

return &rpc.LibraryDownloadResponse{}, nil
}

func downloadLibrary(lm *librariesmanager.LibrariesManager, libRelease *librariesindex.Release,
downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB) error {
downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB, queryParameter string) error {

taskCB(&rpc.TaskProgress{Name: tr("Downloading %s", libRelease)})
config, err := httpclient.GetDownloaderConfig()
if err != nil {
return &arduino.FailedDownloadError{Message: tr("Can't download library"), Cause: err}
}
if err := libRelease.Resource.Download(lm.DownloadsDir, config, libRelease.String(), downloadCB); err != nil {
if err := libRelease.Resource.Download(lm.DownloadsDir, config, libRelease.String(), downloadCB, queryParameter); err != nil {
return &arduino.FailedDownloadError{Message: tr("Can't download library"), Cause: err}
}
taskCB(&rpc.TaskProgress{Completed: true})
Expand Down
25 changes: 18 additions & 7 deletions commands/lib/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ import (
"github.com/sirupsen/logrus"
)

// LibraryInstall FIXMEDOC
func LibraryInstall(ctx context.Context, req *rpc.LibraryInstallRequest, downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB) error {
// LibraryInstall resolves the library dependencies, then downloads and installs the libraries into the install location.
// queryParameter is passed for analysis purposes and forwarded to the called functions. It is set to "depends" when a dependency is installed
func LibraryInstall(ctx context.Context, req *rpc.LibraryInstallRequest, downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB, queryParameter string) error {
lm := commands.GetLibraryManager(req)
if lm == nil {
return &arduino.InvalidInstanceError{}
Expand Down Expand Up @@ -96,11 +97,21 @@ func LibraryInstall(ctx context.Context, req *rpc.LibraryInstallRequest, downloa
}

for libRelease, installTask := range libReleasesToInstall {
if err := downloadLibrary(lm, libRelease, downloadCB, taskCB); err != nil {
return err
}
if err := installLibrary(lm, libRelease, installTask, taskCB); err != nil {
return err
// Checks if libRelease is the requested library and not a dependency
if libRelease.GetName() == req.Name {
if err := downloadLibrary(lm, libRelease, downloadCB, taskCB, queryParameter); err != nil {
return err
}
if err := installLibrary(lm, libRelease, installTask, taskCB); err != nil {
return err
}
} else {
if err := downloadLibrary(lm, libRelease, downloadCB, taskCB, "depends"); err != nil {
return err
}
if err := installLibrary(lm, libRelease, installTask, taskCB); err != nil {
return err
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion commands/lib/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func upgrade(instance *rpc.Instance, libs []*installedLib, downloadCB rpc.Downlo
NoDeps: false,
NoOverwrite: false,
}
err := LibraryInstall(context.Background(), libInstallReq, downloadCB, taskCB)
err := LibraryInstall(context.Background(), libInstallReq, downloadCB, taskCB, "upgrade")
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion internal/cli/lib/download.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func runDownloadCommand(cmd *cobra.Command, args []string) {
Name: library.Name,
Version: library.Version,
}
_, err := lib.LibraryDownload(context.Background(), libraryDownloadRequest, feedback.ProgressBar())
_, err := lib.LibraryDownload(context.Background(), libraryDownloadRequest, feedback.ProgressBar(), "download")
if err != nil {
feedback.Fatal(tr("Error downloading %[1]s: %[2]v", library, err), feedback.ErrNetwork)
}
Expand Down
2 changes: 1 addition & 1 deletion internal/cli/lib/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ func runInstallCommand(cmd *cobra.Command, args []string) {
NoDeps: noDeps,
NoOverwrite: noOverwrite,
}
err := lib.LibraryInstall(context.Background(), libraryInstallRequest, feedback.ProgressBar(), feedback.TaskProgress())
err := lib.LibraryInstall(context.Background(), libraryInstallRequest, feedback.ProgressBar(), feedback.TaskProgress(), "install")
if err != nil {
feedback.Fatal(tr("Error installing %s: %v", libRef.Name, err), feedback.ErrGeneric)
}
Expand Down
40 changes: 40 additions & 0 deletions internal/integrationtest/lib/lib_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1480,3 +1480,43 @@ func TestInstallWithGitUrlLocalFileUri(t *testing.T) {
// Verifies library is installed
require.DirExists(t, libInstallDir.String())
}

func TestLibQueryParameters(t *testing.T) {
// This test should not use shared download directory because it needs to download the libraries from scratch
env := integrationtest.NewEnvironment(t)
cli := integrationtest.NewArduinoCliWithinEnvironment(env, &integrationtest.ArduinoCLIConfig{
ArduinoCLIPath: integrationtest.FindArduinoCLIPath(t),
})
defer env.CleanUp()

// Updates index for cores and libraries
_, _, err := cli.Run("core", "update-index")
require.NoError(t, err)
_, _, err = cli.Run("lib", "update-index")
require.NoError(t, err)

// Check query=install when a library is installed
stdout, _, err := cli.Run("lib", "install", "[email protected]", "-v", "--log-level", "debug")
require.NoError(t, err)
require.Contains(t, string(stdout),
"Starting download \x1b[36murl\x1b[0m=\"https://downloads.arduino.cc/libraries/github.com/arduino-libraries/USBHost-1.0.0.zip?query=install\"\n")

// Check query=upgrade when a library is upgraded
stdout, _, err = cli.Run("lib", "upgrade", "USBHost", "-v", "--log-level", "debug")
require.NoError(t, err)
require.Contains(t, string(stdout),
"Starting download \x1b[36murl\x1b[0m=\"https://downloads.arduino.cc/libraries/github.com/arduino-libraries/USBHost-1.0.5.zip?query=upgrade\"\n")

// Check query=depends when a library dependency is installed
stdout, _, err = cli.Run("lib", "install", "[email protected]", "-v", "--log-level", "debug")
require.NoError(t, err)
require.Contains(t, string(stdout),
"Starting download \x1b[36murl\x1b[0m=\"https://downloads.arduino.cc/libraries/github.com/MajicDesigns/MD_MAX72XX-3.3.1.zip?query=depends\"\n")

// Check query=download when a library is downloaded
stdout, _, err = cli.Run("lib", "download", "[email protected]", "-v", "--log-level", "debug")
require.NoError(t, err)
require.Contains(t, string(stdout),
"Starting download \x1b[36murl\x1b[0m=\"https://downloads.arduino.cc/libraries/github.com/arduino-libraries/WiFi101-0.16.1.zip?query=download\"\n")

}