Skip to content

Add --library flag to compile command #1258

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 13, 2021
Merged

Add --library flag to compile command #1258

merged 5 commits into from
Apr 13, 2021

Conversation

silvanocerza
Copy link
Contributor

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?

Adds a new flags to and existing command and related gRPC function.

  • What is the current behavior?

There is currently no way of specifying a single custom library path when calling the compile command.

  • What is the new behavior?

The user can now set the --library flag to specify one or more libraries root folder when compiling a sketch.

arduino-cli compile -b arduino:avr:uno --library=/path/to/lib /path/to/MySketch
arduino-cli compile -b arduino:avr:uno --library /path/to/lib1 --library /path/to/lib2 /path/to/MySketch
arduino-cli compile -b arduino:avr:uno --library=/path/to/lib1,/path/to/lib2 /path/to/MySketch

Libraries specified this way have top priority in case of conflict with identical libraries.

Nope.

  • Other information:

The docstring for the --libraries flag has also been modified to make it clearer and to differentiate it from this new --library flag.

The --libraries flag expects a path to a folder containing multiple libraries, much like the <sketchbook>/libraries folder does.
Assume we have two libraries in these paths:

/home/user/workspace/libraries/WiFi101
/home/user/workspace/libraries/AudioZero

The --libraries flag must be used like so:

arduino-cli compile -b arduino:avr:uno --libraries /home/user/workspace/libraries /path/to/MySketch

Instead the --library flag expects a path to the root folder of a single library, so it must be used like so:

arduino-cli compile -b arduino:avr:uno --library /home/user/workspace/libraries/WiFi101 --library /home/user/workspace/libraries/AudioZero /path/to/MySketch

See how to contribute

@silvanocerza silvanocerza requested a review from a team April 8, 2021 10:38
@silvanocerza silvanocerza self-assigned this Apr 8, 2021
per1234
per1234 requested changes Apr 9, 2021
View reviewed changes
cli/compile/compile.go
Comment on lines +100 to +103
command.Flags().StringSliceVar(&library, "library", []string{},
"List of paths to libraries root folders. Libraries set this way have top priority in case of conflicts. Can be used multiple times for different libraries.")
command.Flags().StringSliceVar(&libraries, "libraries", []string{},
"List of custom libraries paths separated by commas. Or can be used multiple times for multiple libraries paths.")
"List of custom libraries dir paths separated by commas. Or can be used multiple times for multiple libraries dir paths.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering about the mixture of "folder" and "dir" ("directory") terms in the descriptions of these two closely related flags. Is that intentional?

Even though I like the term "directory" better (probably due to it being the term used when I started with computers), I have made a decision to prefer "folder" in my own communications because all directories are folders, but not all folders are directories and it's too confusing to me to try to figure out whether "directory" will always be applicable for the specific situation. But "directory" is used more often in the Arduino CLI documentation, and is part of the configuration keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use them interchangeably, with this change I wanted to make it clear that --libraries must point to a single directory containing multiple libraries folders but am not sure I made it clearer.

I'm open to suggestions.

@silvanocerza silvanocerza force-pushed the scerza/compile-library-flag branch from ad14acf to 78f4db0 Compare April 9, 2021 10:55
@silvanocerza silvanocerza force-pushed the scerza/compile-library-flag branch from 78f4db0 to 3d1de92 Compare April 9, 2021 13:08
ubidefeo
ubidefeo approved these changes Apr 13, 2021
View reviewed changes
Copy link

@ubidefeo ubidefeo left a comment

Choose a reason for hiding this comment

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

Tested and working

@silvanocerza
Copy link
Contributor Author

@per1234 if it's ok for you I'll merge.

per1234
per1234 approved these changes Apr 13, 2021
View reviewed changes
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Great work! I apologize for being so slow on the re-review.

@silvanocerza
Copy link
Contributor Author

@per1234 no worries. <3

@silvanocerza silvanocerza merged commit 088d427 into master Apr 13, 2021
@silvanocerza silvanocerza deleted the scerza/compile-library-flag branch April 13, 2021 14:48
silvanocerza added a commit that referenced this pull request May 10, 2021
@silvanocerza
Add --library flag to compile command (#1258)
770b363 
* Add --library flag to compile command

* Fix library prioritization

* [skip changelog] Fix receiver name in LibraryManager functions

* [skip changelog] Fix variables names and some docstrings

* Fix libraries not being recompiled when path to source file changes
@ubergesundheit ubergesundheit mentioned this pull request May 11, 2021
Closed
@silvanocerza silvanocerza mentioned this pull request Jun 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ubidefeo ubidefeo ubidefeo approved these changes

@per1234 per1234 per1234 approved these changes

Assignees

@silvanocerza silvanocerza

Labels
type: enhancement Proposed improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@silvanocerza @ubidefeo @per1234