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

Added no_overwrite flag in LibraryInstall #1793

Merged
merged 4 commits into from
Jul 11, 2022

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Jul 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?
Added a flag to lib install to skip library install if the library is already installed (no upgrades or downgrades)

What is the current behavior?
The library is installed, possibly replacing another version already installed.

What is the new behavior?
If the no_overwrite flag is set the installation is aborted if the installed library will replace another already installed library.

Does this PR introduce a breaking change, and is titled accordingly?
no

@cmaglie cmaglie requested a review from a team July 7, 2022 15:46
@cmaglie cmaglie self-assigned this Jul 7, 2022
per1234
per1234 previously requested changes Jul 7, 2022
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.

How about switching the flag to something like --overwrite and inverting the logic?

This type of interface where you have to use a double negative to get a true is not terribly intuitive.

@per1234
Copy link
Contributor

per1234 commented Jul 8, 2022

I'm reconsidering my previous suggestion because:

  • Since the "overwrite" behavior is the current default, the typical use case for the flag under the current Arduino CLI behavior would be --overwrite=false, which is kind of awkward.
  • As for changing the default behavior, I did a quick survey of dependencies management tools and found that Arduino CLI's current default is not so unusual (both go get <module> and npm install <package> will upgrade a previously installed dependency, while pip install <package> and poetry add <package> do not upgrade).

@per1234 per1234 dismissed their stale review July 8, 2022 05:32

See comment above

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.

It would be good to add test coverage of dependencies handling, including custom version constraints.

commands/lib/install.go Outdated Show resolved Hide resolved
rpc/cc/arduino/cli/commands/v1/lib.proto Outdated Show resolved Hide resolved
@per1234 per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Jul 8, 2022
@cmaglie cmaglie force-pushed the add_do_not_overwrite_on_lib_install branch from 0651e1d to 0ad48e0 Compare July 8, 2022 09:23
@cmaglie cmaglie requested review from umbynos and per1234 July 8, 2022 09:47
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.

The behavior when there are custom dependency version constraints is not as I expected.

The library [email protected] has the following depends field:

depends=Bounce2 (>=2.53)

My expectation is that if [email protected] is already installed, installation of [email protected] will:

  • Install the EncoderTool (because the currently installed version of the dependency matches its constraint)
  • Not update the Bounce2 library

But the installation fails:

$ arduino-cli version
arduino-cli.exe  Version: git-snapshot Commit: 0ad48e07 Date: 2022-07-08T10:52:47Z

$ arduino-cli lib install [email protected]
Downloading [email protected]...
[email protected] downloaded
Installing [email protected]...
Installed [email protected]

$ arduino-cli lib install --no-overwrite [email protected]
Error installing EncoderTool: Library [email protected] is already installed, but with a different version: [email protected]

@cmaglie
Copy link
Member Author

cmaglie commented Jul 8, 2022

The behavior when there are custom dependency version constraints is not as I expected.

I see and you're right with your expected output. As it is now the --no-overwrite flag is not affecting the library dependencies resolution, it just stops the install if the "solution" found by the resolver will change at least one of the libraries.

To achieve your request we need to change how the resolver behaves, BTW this change it's quite involved and requires more work on the library dependencies resolution part. We need to:

  • give the status of the currently installed libraries to the resolver
  • ask the resolver to keep the already installed libraries at their version this means that
    • if a dependency is mentioned in a constraint expression we must either:
      • synthesize another constraint that forces the mentioned library to stay at the current version, or
      • pick the mentioned library at the current version as part of the solution
  • merge some pending PR on the semver library:

So for now I think we can postpone this to a future release since it will not be a breaking change but an improvement to the currently available commands.

If you agree, I would ask you to move the example above to a new issue.

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: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants