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

Fix resolution of library names when checking for upgrades #1725

Merged
merged 1 commit into from
May 11, 2022
Merged

Fix resolution of library names when checking for upgrades #1725

merged 1 commit into from
May 11, 2022

Conversation

stonehippo
Copy link
Contributor

@stonehippo stonehippo commented May 7, 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 kind of change does this PR introduce?

This change resolves #1041, where packages that have upgrades based on library_index.json do not show up as upgradable with using the arduino-cli lib commands.

  • What is the current behavior?

When using the arduino-cli lib commands to check for or apply updates, packages that have spaces in the name in the index are not shown as upgradable.

  • What is the new behavior?

The overall behavior is unchanged and commands remain as currently implemented and documented. However, the resolution of packages that have spaces in the names now works, so commands such as arduino-cli lib list --updatable will now show all upgradable packages.

No

  • Other information:

See how to contribute

@CLAassistant
Copy link

CLAassistant commented May 7, 2022

CLA assistant check
All committers have signed the CLA.

arduino/libraries/librariesindex/index.go Outdated Show resolved Hide resolved
@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels May 8, 2022
@per1234 per1234 self-assigned this May 8, 2022
The real name for a lib (the name stored in the library.properties)
is required to find a library in the index. The installed name, which
matches the diretory name, is not valid for looking up libraries in
the index.

Libraries that do not have a library.properties file, or where the
name value in that file does not match the value in library_index.json,
cannot be upgrade by the CLI.

Also Fix tests for outdated libs

The tests for update and outdated changed the library.properties
file for an installed lib, but also wiped out the name field.
Since we rely on that field to determine the library name when
using the RealName property, the test's change was causing a failure
outside the scope of the test itself.
@per1234
Copy link
Contributor

per1234 commented May 10, 2022

It might make sense to mark those libraries as "legacy" to declare that they have to be upgraded manually

They are already marked as such in the [*].library.is_legacy field of the machine readable output:

$ arduino-cli version
arduino-cli.exe  Version: git-snapshot Commit: d4d1fa91 Date: 2022-05-10T02:50:00Z

$ export ARDUINO_LIBRARY_ENABLE_UNSAFE_INSTALL="true"

$ arduino-cli lib install --git-url https://github.com/arduino/HMC5983
--git-url and --zip-path flags allow installing untrusted files, use it at your own risk.
Enumerating objects: 9, done.
Counting objects: 100% (9/9), done.
Compressing objects: 100% (7/7), done.
Total 9 (delta 0), reused 6 (delta 0), pack-reused 0
Library installed

$ arduino-cli lib list --format json
[
  {
    "library": {
      "name": "HMC5983",
      "architectures": [
        "*"
      ],
      "install_dir": "E:\\deleteme\\arduino-cli\\directories\\user\\libraries\\HMC5983",
      "source_dir": "e:\\deleteme\\arduino-cli\\directories\\user\\libraries\\HMC5983",
      "is_legacy": true,
      "location": 1,
      "examples": [
        "E:\\deleteme\\arduino-cli\\directories\\user\\libraries\\HMC5983\\examples\\single_read"
      ],
      "provides_includes": [
        "HMC5983.h"
      ]
    }
  }
]

Of course the average user will be using the text format output, but I'm not sure how much of a concern it is in reality since the libraries using that ancient format are not very common these days and when they are used it is likely no update will available.

if they're in the index but still not conforming to the 1.5 spec

1.5 format is a requirement for inclusion in the index, so there are no "legacy" format libraries in the index.

Note that the uninstall command uses a different resolving mechanism and will uninstall libraries based on their real names or directory names. It may also make sense to use this resolving mechanism in place of what I have here, but I'll leave that to reviewers to weigh in on.

Thanks for pointing that out. I am against it. In my opinion, there should be one and only one name for each library, which is always used when referring to the library in the Arduino CLI output and in the commands. So I think the way you chose to make lib upgrade work here is correct.

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.

Thanks @stonehippo!

This PR fixes only one of several of the symptoms of an unintuitive and flawed approach to handling names of installed libraries. However, it does indeed fix that symptom and thus is a valuable contribution by making the behavior of Arduino CLI less buggy in this respect.

So I am in support of merging this, even though the big picture is that the entire local library name system needs to be reworked from the root of the problem in the design of github.com/arduino/arduino-cli/arduino/libraries.Library on up.

I'll give a little time in case other maintainers want to review before merging.

@stonehippo
Copy link
Contributor Author

Thanks, @per1234. I agree re the current system. It's really hard to make definitive calls about the state of a user's installed libraries vs the index. You're right that this is only a patch over the symptoms, but it's going to take some collective work to reconcile the deeper issue.

@per1234 per1234 merged commit ca8c48a into arduino:master May 11, 2022
@stonehippo stonehippo deleted the lib-upgrade-resolution-fix branch May 11, 2022 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Newer library is not listed as available
3 participants