Added support for external libraries #648

vinay-lanka · 2020-04-09T15:41:47Z

Please check if the PR fulfills these requirements

The PR follows our contributing guidelines
The PR has no duplicates (please search among the Pull Requests before creating one)
Tests for the changes have been added (for bug fixes / features)

What kind of change does this PR introduce?

Feature -

Added support for external libraries -
- added --git-url flag to add libraries via git url
  Usage : arduino-cli lib install <Library Name> --git-url <url>
- added --zip-path flag to add libraries by pointing path to zip file
  Usage : arduino-cli lib install <Library Name> --zip-path <absolute path>
What is the new behavior?
One can add external libraries and existing libraries right from the git repositories.
Can also include libraries from other vendors distributed as a zip file.
Other information:
Have not added tests for zip-path flag. (Suggestions on how to download and locate a zip library with pytest)

vinay-lanka · 2020-04-23T21:12:00Z

Is there anything I could do to improve this change?
I feel this could be a slight improvement to the whole user experience as discussed in #128

altjz · 2020-10-16T02:43:17Z

Any progress on this feature?

silvanocerza · 2020-10-21T08:48:04Z

Hey @vinay-lanka, sorry for the late response on this PR!
It would be great if you have the time to rebase it on top of master to resolve the current conflicts.
Don't worry if you don't have the time I'll find another way.

vinay-lanka · 2020-10-23T09:34:18Z

Hey,
Sure, could I have a day or two to rebase it to the master?
I'll try to resolve all the current conflicts ASAP.

silvanocerza · 2020-10-23T09:35:12Z

That's perfect, thanks!

vinay-lanka · 2020-10-26T02:40:31Z

Hey, @silvanocerza, I'm done with resolving all conflicts.

silvanocerza · 2020-10-26T16:46:19Z

@vinay-lanka thanks! I'll do a review as soon as I can, probably tomorrow.

arduino/libraries/librariesmanager/install.go

cli/lib/install.go

silvanocerza · 2020-10-27T09:57:39Z

cli/lib/install.go

+	} else if installFlags.gitURL != "" {
+		GitlibraryInstallReq := &rpc.GitLibraryInstallReq{
+			Instance: instance,
+			Name:     args[0],


Why args[0] as Name? Can extract it directly from the gitURL inside the GitLibraryInstall function?

So I thought it'd be better if we followed the same syntax of arduino-cli lib install LIBRARY[@VERSION_NUMBER](S) [flags]. Even though it seems redundant, I thought it would minimize confusion as the same command would have different arguments for different flags.

Ah! I see why you did it but I think it would be better to avoid the redundancy, also if we go that way we'd have to do it for the installation suing the zip file too.

commands/lib/install.go

silvanocerza · 2020-10-27T10:01:18Z

commands/lib/install.go

+	res := &rpc.ZipLibraryInstallResp{}
+	lm := commands.GetLibraryManager(req.GetInstance().GetId())
+	Path := req.GetPath()
+	if err := installZipLibrary(lm, Path); err != nil {


We don't need the installZipLibrary, call directly LibrariesManager.InstallZipLib.

The normal Library install procedure is divided into two functions LibraryInstall() and installLibrary() so I thought I'd follow the same style. As it does not actually have a unique function, I'll remove it asap.

Interesting, the normal procedure will probably need to be changed to if it's like that, not in this PR though.

commands/lib/install.go

rpc/commands/lib.proto

test/test_lib.py

silvanocerza · 2020-11-02T10:41:55Z

@vinay-lanka please let me know when the PR is ready for review again, or if there are other issues so we can see what to do.

vinay-lanka · 2020-11-02T13:56:15Z

Hey, I was waiting for #1049 to get merged because I had a problem with the integration tests. I rebased after that but I'm still getting an assertion error at test/test_core.py::test_core_search - assert 2 == 0 . I'm trying to figure it out but the rest is ready for a review.

silvanocerza · 2020-11-02T13:58:25Z

Don't worry about that test failing for now, I'm aware of it and trying to understand what's going on exactly.

silvanocerza · 2020-11-02T16:08:38Z

Those tests have been fixed, should I review it now?

vinay-lanka · 2020-11-02T18:18:35Z

I've cleaned up the code (Sorry for the numerous errors!)
I've added tests for the zip path based install and added a test for lib download as well

silvanocerza · 2020-11-03T15:07:57Z

arduino/libraries/librariesmanager/install.go

+	r, err := zip.OpenReader(libPath)
+	if err != nil {
+		return err
+	}
+	defer r.Close()
+
+	for _, f := range r.File {
+
+		// Store filename/path for returning and using later on
+		fpath := filepath.Join(libsDir.String(), f.Name)
+
+		// Check for ZipSlip.
+		if !strings.HasPrefix(fpath, filepath.Clean(libsDir.String())+string(os.PathSeparator)) {
+			return fmt.Errorf("%s: illegal file path", fpath)
+		}
+
+		filenames = append(filenames, fpath)
+
+		if f.FileInfo().IsDir() {
+			// Make Folder
+			os.MkdirAll(fpath, os.ModePerm)
+			continue
+		}
+
+		// Make File
+		if err = os.MkdirAll(filepath.Dir(fpath), os.ModePerm); err != nil {
+			return err
+		}
+
+		outFile, err := os.OpenFile(fpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())
+		if err != nil {
+			return err
+		}
+
+		rc, err := f.Open()
+		if err != nil {
+			return err
+		}
+
+		_, err = io.Copy(outFile, rc)
+
+		// Close the file without defer to close before next iteration of loop
+		outFile.Close()
+		rc.Close()
+
+		if err != nil {
+			return err
+		}


Would be great if you could use the codeclysm/extract library, it's already around the CLI, see arduino/resources/install.go for example.

silvanocerza · 2020-11-03T15:17:43Z

arduino/libraries/librariesmanager/install.go

+	}
+	i := strings.LastIndex(url, "/")
+	folder := strings.TrimRight(url[i+1:], ".git")
+	path := libsDir.String() + "/" + folder


Concatenate using Path.Join() like so libsDir.Join(folder), using the path separator explicitly like so might break on some OS.

silvanocerza · 2020-11-03T15:25:52Z

cli/lib/install.go

+		_, err := lib.ZipLibraryInstall(context.Background(), ziplibraryInstallReq)
+		if err != nil {
+			feedback.Errorf("Error installing Zip Library %v", err)
+			os.Exit(errorcodes.ErrGeneric)
+		}
+	} else if installFlags.gitURL != "" {
+		gitlibraryInstallReq := &rpc.GitLibraryInstallReq{
+			Instance: instance,
+			Url:      installFlags.gitURL,
+		}
+		_, err := lib.GitLibraryInstall(context.Background(), gitlibraryInstallReq)
+		if err != nil {
+			feedback.Errorf("Error installing Git Library %v", err)
+			os.Exit(errorcodes.ErrGeneric)
 		}
 	} else {
-		for _, libRef := range libRefs {
-			depsResp, err := lib.LibraryResolveDependencies(context.Background(), &rpc.LibraryResolveDependenciesReq{
-				Instance: instance,
-				Name:     libRef.Name,
-				Version:  libRef.Version,
-			})
-			if err != nil {
-				feedback.Errorf("Error resolving dependencies for %s: %s", libRef, err)
-				os.Exit(errorcodes.ErrGeneric)
+		if len(args) == 0 {
+			cmd.Help()
+			os.Exit(errorcodes.ErrGeneric)
+		}
+		libRefs, err := ParseLibraryReferenceArgsAndAdjustCase(instance, args)
+		if err != nil {
+			feedback.Errorf("Arguments error: %v", err)
+			os.Exit(errorcodes.ErrBadArgument)
+		}
+
+		toInstall := map[string]*rpc.LibraryDependencyStatus{}
+		if installFlags.noDeps {
+			for _, libRef := range libRefs {
+				toInstall[libRef.Name] = &rpc.LibraryDependencyStatus{
+					Name:            libRef.Name,
+					VersionRequired: libRef.Version,
+				}
 			}
-			for _, dep := range depsResp.GetDependencies() {
-				feedback.Printf("%s depends on %s@%s", libRef, dep.GetName(), dep.GetVersionRequired())
-				if existingDep, has := toInstall[dep.GetName()]; has {
-					if existingDep.GetVersionRequired() != dep.GetVersionRequired() {
-						// TODO: make a better error
-						feedback.Errorf("The library %s is required in two different versions: %s and %s",
-							dep.GetName(), dep.GetVersionRequired(), existingDep.GetVersionRequired())
-						os.Exit(errorcodes.ErrGeneric)
+		} else {
+			for _, libRef := range libRefs {
+				depsResp, err := lib.LibraryResolveDependencies(context.Background(), &rpc.LibraryResolveDependenciesReq{
+					Instance: instance,
+					Name:     libRef.Name,
+					Version:  libRef.Version,
+				})
+				if err != nil {
+					feedback.Errorf("Error resolving dependencies for %s: %s", libRef, err)
+					os.Exit(errorcodes.ErrGeneric)
+				}
+				for _, dep := range depsResp.GetDependencies() {
+					feedback.Printf("%s depends on %s@%s", libRef, dep.GetName(), dep.GetVersionRequired())
+					if existingDep, has := toInstall[dep.GetName()]; has {
+						if existingDep.GetVersionRequired() != dep.GetVersionRequired() {
+							// TODO: make a better error
+							feedback.Errorf("The library %s is required in two different versions: %s and %s",
+								dep.GetName(), dep.GetVersionRequired(), existingDep.GetVersionRequired())
+							os.Exit(errorcodes.ErrGeneric)
+						}
 					}
+					toInstall[dep.GetName()] = dep
 				}
-				toInstall[dep.GetName()] = dep
 			}
 		}
-	}

-	for _, library := range toInstall {
-		libraryInstallReq := &rpc.LibraryInstallReq{
-			Instance: instance,
-			Name:     library.Name,
-			Version:  library.VersionRequired,
-		}
-		err := lib.LibraryInstall(context.Background(), libraryInstallReq, output.ProgressBar(), output.TaskProgress())
-		if err != nil {
-			feedback.Errorf("Error installing %s: %v", library, err)
-			os.Exit(errorcodes.ErrGeneric)
+		for _, library := range toInstall {
+			libraryInstallReq := &rpc.LibraryInstallReq{
+				Instance: instance,
+				Name:     library.Name,
+				Version:  library.VersionRequired,
+			}
+			err := lib.LibraryInstall(context.Background(), libraryInstallReq, output.ProgressBar(), output.TaskProgress())
+			if err != nil {
+				feedback.Errorf("Error installing %s: %v", library, err)
+				os.Exit(errorcodes.ErrGeneric)
+			}


I'm not sure setting the minimum number of args to 0 is the best approach here.
Let's try this thing, change the minimum number of args back to 1, the flags git-url and zip-path to boolean flags. Doing so we can call the CLI like so:

arduino-cli lib install <my_lib_name> arduino-cli lib install --git-url <my_git_url> arduino-cli lib install --zip-path <my_zip_file>

You'll have to change a bit the logic in this file but I think it's a good improvement and will make the command clearer to use.

silvanocerza · 2020-11-03T15:29:12Z

commands/lib/install.go

+		res.Status = "Error installing Git Library"
+		return res, err


If there's an error don't return res, just the error. return nil, err is fine, same for ZipLibraryInstall.

have added a --zip-path flag to lib install command takes absolute path to zip file and extracts it to libraries directory

have added a --git-url flag to lib install takes the git url and extracts it to the libraries directory

silvanocerza · 2020-11-04T14:41:42Z

Approved, looks great now! 🥳
Don't worry about the verify-links failing, we're aware of it and is not an issue.
Thank you very much for the contribution, we greatly appreciate the help of the community and look forward to future PRs. :)
I'll merge this as soon all checks finish.

silvanocerza · 2020-11-04T14:46:30Z

Merged! Thanks again @vinay-lanka.

vinay-lanka · 2020-11-04T15:01:55Z

Thank you so much for the opportunity!
I learnt a lot while contributing so would love to open PRs in the future! :)

ubidefeo · 2020-11-04T18:03:53Z

@vinay-lanka PRs like these are always welcome 👍

vinay-lanka force-pushed the libfeature branch from 4917a48 to 0b0b968 Compare April 23, 2020 20:51

vinay-lanka force-pushed the libfeature branch 2 times, most recently from d2a6e0e to f021cd0 Compare October 25, 2020 18:10