Skip to content
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

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

Merged
merged 12 commits into from Sep 21, 2022

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Sep 16, 2022

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

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
arduino/libraries/librarieslist.go Outdated Show resolved Hide resolved
internal/integrationtest/lib/lib_test.go Outdated Show resolved Hide resolved
cmaglie and others added 2 commits September 19, 2022 16:53
Co-authored-by: Umberto Baldi <34278123+umbynos@users.noreply.github.com>
@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?

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants