Skip to content

[breaking] Fix behaviour of lib commands when a library is installed multiple times #1878

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 12 commits into from
Sep 21, 2022
Merged

[breaking] Fix behaviour of lib commands when a library is installed multiple times #1878

merged 12 commits into from
Sep 21, 2022

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Sep 16, 2022 •
edited by umbynos
Loading

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 is the current behavior?
    1. lib install and lib upgrade may cause multiple installations of the same library
    2. if you use lib install/upgrade/uninstall on a library installed multiple times, the command may act only on one of the multiple copies (chosen pseudo-randomly).
    3. some commands do not use the real name of the library but the installed directory name, i.e. lib example "Robot_Control" instead of lib example "Robot Control".
  • What is the new behavior?
    1. lib install and lib upgrade will not cause multiple installations anymore
    2. if you use lib install/upgrade/uninstall on a library installed multiple times, the command will fail, showing the path of every copy of the library and asking to manually fix the multiple installations.
    3. all commands now use the real name of the library (i.e. the one written inside the library.properties file)
  • Does this PR introduce a breaking change, and is titled accordingly?
    Yes. The biggest change is the renaming of the fields in the Library structure:
    • Name -> CanonicalName
    • RealName -> Name
      This change is quite involved in the go-lang codebase but results in a backward compatible change in the user-facing API. It should make the overall API usage more straightforward since it factors out the legacy (IDE <1.5) way to name the libraries as the containing directory.

@codecov
Copy link

codecov bot commented Sep 16, 2022 •
edited
Loading

Codecov Report

Base: 36.68% // Head: 36.62% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (ac3018b) compared to base (7d1916d).
Patch coverage: 16.26% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1878      +/-   ##
==========================================
- Coverage   36.68%   36.62%   -0.06%     
==========================================
  Files         231      231              
  Lines       19680    19708      +28     
==========================================
- Hits         7219     7218       -1     
- Misses      11633    11662      +29     
  Partials      828      828
Flag Coverage Δ
unit 36.62% <16.26%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
arduino/errors.go 3.70% <0.00%> (-0.13%) ⬇️
arduino/libraries/libraries.go 43.47% <0.00%> (ø)
arduino/libraries/librarieslist.go 41.66% <0.00%> (-29.77%) ⬇️
arduino/libraries/librariesmanager/install.go 14.46% <0.00%> (-0.51%) ⬇️
commands/lib/install.go 0.00% <0.00%> (ø)
commands/lib/list.go 0.00% <0.00%> (ø)
commands/lib/uninstall.go 0.00% <0.00%> (ø)
commands/lib/upgrade.go 0.00% <0.00%> (ø)
legacy/builder/create_cmake_rule.go 8.80% <0.00%> (ø)
...ino/libraries/librariesmanager/librariesmanager.go 54.05% <33.33%> (+7.95%) ⬆️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cmaglie cmaglie self-assigned this Sep 16, 2022
@cmaglie cmaglie added priority: high Resolution is a high priority topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project criticality: medium Of moderate impact labels Sep 16, 2022
@cmaglie cmaglie marked this pull request as ready for review September 16, 2022 15:11
@cmaglie cmaglie requested review from per1234 and umbynos September 16, 2022 15:11
umbynos
umbynos approved these changes Sep 19, 2022
View reviewed changes
@cmaglie cmaglie merged commit b9599e1 into arduino:master Sep 21, 2022
@cmaglie cmaglie deleted the multilib-installs branch September 21, 2022 12:07
This was referenced Sep 22, 2022
Closed
Closed
Closed
@kittaakos
Copy link
Contributor

  • Name -> CanonicalName
  • RealName -> Name
    This change is quite involved in the go-lang codebase but results in a backward compatible change in the user-facing API.

Would you consider applying the same change on the gRPC API?

arduino-cli/rpc/cc/arduino/cli/commands/v1/lib.proto

Line 254 in 5730e2e

string name = 1;

arduino-cli/rpc/cc/arduino/cli/commands/v1/lib.proto

Line 283 in 5730e2e

string real_name = 16 [ deprecated = true ];

IDE2 still uses both. RealName is available on the UI, and Name is the unique ID of the lib.

Should IDE2 apply any changes after closing #932?

Thank you!

@cmaglie
Copy link
Member Author

cmaglie commented Sep 23, 2022

Now both Name and RealName gets the same value: the library unique name (as written in the index / library.properties).
I forget to change the description on the field Name, will do it in a new PR.
I kept RealName for backward compatibility, so there should be no changes required in the IDE.
BTW it's probably better to remove the RealName field and cleanup the API, WDYT?

This was referenced Sep 27, 2022
Closed
Merged
@per1234 per1234 mentioned this pull request Oct 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@umbynos umbynos umbynos approved these changes

@per1234 per1234 Awaiting requested review from per1234

Assignees

@cmaglie cmaglie

Labels
criticality: medium Of moderate impact priority: high Resolution is a high priority topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cmaglie @kittaakos @umbynos