Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Added support for external libraries #648

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
silvanocerza merged 3 commits into arduino:master from vinay-lanka:libfeature
Nov 4, 2020

Conversation

Copy link
Contributor

@vinay-lanka vinay-lanka commented Apr 9, 2020 •
edited
Loading

Please check if the PR fulfills these requirements

  • 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)

See how to contribute

SteffanoP, JDaniel41, Tim-ONT, umbynos, and phptuts reacted with heart emoji
Copy link
Contributor Author

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

Copy link

altjz commented Oct 16, 2020

Any progress on this feature?

Copy link
Contributor

silvanocerza commented Oct 21, 2020 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Contributor

That's perfect, thanks!

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

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

Copy link
Contributor

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

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

@silvanocerza silvanocerza Oct 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@vinay-lanka vinay-lanka Oct 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@silvanocerza silvanocerza Oct 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@silvanocerza silvanocerza Oct 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@vinay-lanka vinay-lanka Oct 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@silvanocerza silvanocerza Oct 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vinay-lanka vinay-lanka force-pushed the libfeature branch 4 times, most recently from b6f4022 to e60c16b Compare October 28, 2020 15:48
Copy link
Contributor

@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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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

Comment on lines 104 to 151
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
}
Copy link
Contributor

@silvanocerza silvanocerza Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vinay-lanka reacted with thumbs up emoji
}
i := strings.LastIndex(url, "/")
folder := strings.TrimRight(url[i+1:], ".git")
path := libsDir.String() + "/" + folder
Copy link
Contributor

@silvanocerza silvanocerza Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vinay-lanka reacted with thumbs up emoji
Comment on lines 39 to 127
Args: cobra.MinimumNArgs(1),
Args: cobra.MinimumNArgs(0),
Run: runInstallCommand,
}
installCommand.Flags().BoolVar(&installFlags.noDeps, "no-deps", false, "Do not install dependencies.")
installCommand.Flags().StringVarP(&installFlags.gitURL, "git-url", "g", "", "Enter git url for libraries hosted on repositories")
installCommand.Flags().StringVarP(&installFlags.zipPath, "zip-path", "z", "", "Enter a path to zip file")
return installCommand
}

var installFlags struct {
noDeps bool
noDeps bool
gitURL string
zipPath string
}

func runInstallCommand(cmd *cobra.Command, args []string) {
instance := instance.CreateInstanceIgnorePlatformIndexErrors()
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,
}
if installFlags.zipPath != "" {
ziplibraryInstallReq := &rpc.ZipLibraryInstallReq{
Instance: instance,
Path: installFlags.zipPath,
}
_, 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)
}
Copy link
Contributor

@silvanocerza silvanocerza Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

vinay-lanka reacted with thumbs up emoji
Comment on lines 98 to 99
res.Status = "Error installing Git Library"
return res, err
Copy link
Contributor

@silvanocerza silvanocerza Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vinay-lanka reacted with thumbs up emoji
vinay-lanka added 3 commits November 4, 2020 15:48 
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
Copy link
Contributor

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.
umbynos and altjz reacted with rocket emoji

@silvanocerza silvanocerza merged commit 8f5d38b into arduino:master Nov 4, 2020
Copy link
Contributor

Merged! Thanks again @vinay-lanka.

Copy link
Contributor Author

Thank you so much for the opportunity!
I learnt a lot while contributing so would love to open PRs in the future! :)
rsora, ubidefeo, per1234, and altjz reacted with thumbs up emoji

Copy link

ubidefeo commented Nov 4, 2020

@vinay-lanka PRs like these are always welcome 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@silvanocerza silvanocerza silvanocerza approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /