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

Add install-upgrade. #6798

Merged
merged 3 commits into from
Apr 5, 2019
Merged

Add install-upgrade. #6798

merged 3 commits into from
Apr 5, 2019

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Mar 31, 2019

This implements the feature described in #6667. Instead of failing when cargo install detects a package is already installed, it will upgrade if the versions don't match, or do nothing (exit 0) if it is considered "up-to-date".

Closes #6667.

Notes:

  • This feature rejects ambiguous --version input (such as 1.0). This is not required, but seemed like a reasonable time to make the change.
  • There is a slight change to the output on stable which reports what was installed/replaced.
  • Added better support for * in --version (don't show warning).

@rust-highfive
Copy link

r? @dwijnand

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 31, 2019
@bors
Copy link
Contributor

bors commented Apr 1, 2019

☔ The latest upstream changes (presumably #6804) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss ehuss force-pushed the install-upgrade branch 3 times, most recently from f95f952 to 3929431 Compare April 1, 2019 20:59
@bors
Copy link
Contributor

bors commented Apr 2, 2019

☔ The latest upstream changes (presumably #6812) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss ehuss force-pushed the install-upgrade branch from 3929431 to 7af4577 Compare April 2, 2019 14:55
@bors
Copy link
Contributor

bors commented Apr 2, 2019

☔ The latest upstream changes (presumably #6811) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss ehuss force-pushed the install-upgrade branch from 7af4577 to e023a66 Compare April 2, 2019 15:40
@dwijnand
Copy link
Member

dwijnand commented Apr 2, 2019

I'm just getting home. Sorry for this sitting in the PR queue unreviewed. I won't have time to review it in the near future so I'll reassign it.

r? @alexcrichton

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Sorry it took me awhile to get to this, but this looks fantastic! The biggest thing I'd like to learn more about is the state of the v1/v2 manifests and how they're kept in sync and that logic, but otherwise this looks fantastic

src/cargo/ops/cargo_install.rs Show resolved Hide resolved
src/cargo/ops/common_for_install_and_uninstall.rs Outdated Show resolved Hide resolved
src/cargo/ops/common_for_install_and_uninstall.rs Outdated Show resolved Hide resolved
src/cargo/ops/common_for_install_and_uninstall.rs Outdated Show resolved Hide resolved
src/cargo/ops/common_for_install_and_uninstall.rs Outdated Show resolved Hide resolved
if !installed.get().contains(bin) {
failure::bail!("binary `{}` not installed as part of `{}`", bin, pkgid)
}
let dst = root.join("bin").into_path_unlocked();
Copy link
Member

Choose a reason for hiding this comment

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

To avoid extra uses of into_path_unlocked, could the bin path be learned from InstallTracker which internally holds locks?

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'm not sure I understand. Do you mean something like this?

    pub fn bin_dir(&self) -> PathBuf {
        self.v1_lock.parent().join("bin")
    }

Copy link
Member

Choose a reason for hiding this comment

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

Something like that yeah, but actually in retrospect I'm not sure if this is a great idea due to --no-track. In general into_path_unlocked is a code smell we want to avoid and is akin to unsafe sorta. In that sense we'd ideally thoroughly audit any usages of into_path_unlocked regularly (hah, like that ever will happen) and have sufficient comments indicating why it's justified to not require file locking at a particular time.

In the spirit of that I was largely hoping to minimize the number of calls to into_path_unlocked, but InstallTracker here isn't always available. Maybe though a method could be added to InstallTracker and then there'd be one-ish invocation of into_path_unlocked for --no-track, and a comment saying that if you say --no-track it's up to someone else to handle concurrent installs?

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 thought about this for a while. I'm reluctant to add bin_dir if it is used in one place but no other. I also don't see a clear way to reduce the into_path_unlocked. The dst for install is used in a large number of places. We could create a new abstraction (similar to Layout) which has a temporary lock file in $CARGO_INSTALL_ROOT which could wrap all the usage in a safer way. In this case, I don't see it significantly improving things.

FWIW, I'm not yet convinced on the utility of --no-track. If that is removed, something like bin_dir would work better.

Copy link
Member

Choose a reason for hiding this comment

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

Ok sounds reasonable to me!

src/cargo/ops/cargo_install.rs Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor Author

ehuss commented Apr 4, 2019

Thanks for the detailed review! I added more comments, and I think I addressed everything (except where I responded).

@alexcrichton
Copy link
Member

Ok a request for a comment and a request to look into reducing into_path_unlocked if possible, but otherwise r=me. Looking forward to this, thanks @ehuss!

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 5, 2019

📌 Commit 5a9ff8a has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2019
@bors
Copy link
Contributor

bors commented Apr 5, 2019

⌛ Testing commit 5a9ff8a with merge f5b60ac...

bors added a commit that referenced this pull request Apr 5, 2019
Add install-upgrade.

This implements the feature described in #6667. Instead of failing when `cargo install` detects a package is already installed, it will upgrade if the versions don't match, or do nothing (exit 0) if it is considered "up-to-date".

Closes #6667.

Notes:
- This feature rejects ambiguous `--version` input (such as `1.0`). This is not required, but seemed like a reasonable time to make the change.
- There is a slight change to the output on stable which reports what was installed/replaced.
- Added better support for `*` in `--version` (don't show warning).
@bors
Copy link
Contributor

bors commented Apr 5, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing f5b60ac to master...

@bors bors merged commit 5a9ff8a into rust-lang:master Apr 5, 2019
Centril added a commit to Centril/rust that referenced this pull request Apr 15, 2019
Update cargo

12 commits in 6f3e9c367abb497c64f360c3839dab5e74928d5c..ef0223f12597b5e0d9d2feed1b92c41306b1fc05
2019-04-04 14:11:33 +0000 to 2019-04-15 14:36:55 +0000
- Fix test include_overrides_gitignore. (rust-lang/cargo#6850)
- Clarify optional registry key behaviour (rust-lang/cargo#6851)
- Ensure Summary::checksum works for registry crates (rust-lang/cargo#6842)
- Better error if PathSource::walk can't access something. (rust-lang/cargo#6841)
- Improve warning in `cargo new` with parse error. (rust-lang/cargo#6839)
- Improve error message for `publish` key restriction. (rust-lang/cargo#6838)
- Remove `Freshness` from `DependencyQueue` (rust-lang/cargo#6832)
- testsuite: cleanup for `alternative-registries` (rust-lang/cargo#6837)
- Improve error message to rerun a test in a workspace. (rust-lang/cargo#6824)
- Fix mutable_borrow_reservation_conflict warning. (rust-lang/cargo#6829)
- Add install-upgrade. (rust-lang/cargo#6798)
- Clarify docs of install without <crate> (rust-lang/cargo#6823)
Centril added a commit to Centril/rust that referenced this pull request Apr 16, 2019
Update cargo

12 commits in 6f3e9c367abb497c64f360c3839dab5e74928d5c..ef0223f12597b5e0d9d2feed1b92c41306b1fc05
2019-04-04 14:11:33 +0000 to 2019-04-15 14:36:55 +0000
- Fix test include_overrides_gitignore. (rust-lang/cargo#6850)
- Clarify optional registry key behaviour (rust-lang/cargo#6851)
- Ensure Summary::checksum works for registry crates (rust-lang/cargo#6842)
- Better error if PathSource::walk can't access something. (rust-lang/cargo#6841)
- Improve warning in `cargo new` with parse error. (rust-lang/cargo#6839)
- Improve error message for `publish` key restriction. (rust-lang/cargo#6838)
- Remove `Freshness` from `DependencyQueue` (rust-lang/cargo#6832)
- testsuite: cleanup for `alternative-registries` (rust-lang/cargo#6837)
- Improve error message to rerun a test in a workspace. (rust-lang/cargo#6824)
- Fix mutable_borrow_reservation_conflict warning. (rust-lang/cargo#6829)
- Add install-upgrade. (rust-lang/cargo#6798)
- Clarify docs of install without <crate> (rust-lang/cargo#6823)
bors added a commit to rust-lang/rust that referenced this pull request Apr 16, 2019
Update cargo

16 commits in 6f3e9c367abb497c64f360c3839dab5e74928d5c..b6581d383ed596b133e330011658c6f83cf85c2f
2019-04-04 14:11:33 +0000 to 2019-04-16 16:02:11 +0000
- Fix new_warning_with_corrupt_ws missing "USER". (rust-lang/cargo#6857)
- Ignore Clippy redundant_closure (rust-lang/cargo#6855)
- Pass OsStr/OsString args through to the process spawned by cargo run. (rust-lang/cargo#6849)
- Bump to 0.37.0 (rust-lang/cargo#6852)
- Fix test include_overrides_gitignore. (rust-lang/cargo#6850)
- Clarify optional registry key behaviour (rust-lang/cargo#6851)
- Ensure Summary::checksum works for registry crates (rust-lang/cargo#6842)
- Better error if PathSource::walk can't access something. (rust-lang/cargo#6841)
- Improve warning in `cargo new` with parse error. (rust-lang/cargo#6839)
- Improve error message for `publish` key restriction. (rust-lang/cargo#6838)
- Remove `Freshness` from `DependencyQueue` (rust-lang/cargo#6832)
- testsuite: cleanup for `alternative-registries` (rust-lang/cargo#6837)
- Improve error message to rerun a test in a workspace. (rust-lang/cargo#6824)
- Fix mutable_borrow_reservation_conflict warning. (rust-lang/cargo#6829)
- Add install-upgrade. (rust-lang/cargo#6798)
- Clarify docs of install without <crate> (rust-lang/cargo#6823)
@ehuss ehuss mentioned this pull request Nov 4, 2019
bors added a commit that referenced this pull request Nov 21, 2019
Stabilize install-upgrade.

Tracking issue: #6797

This stabilizes the install-upgrade feature, which causes `cargo install` to reinstall a package if it appears to be out of date, or exit cleanly if it is up-to-date.

There are no changes from `-Zinstall-upgrade`. See [the old unstable docs](https://github.com/rust-lang/cargo/blob/6a7f505a185b000e38bdad64392098e0b2e50802/src/doc/src/reference/unstable.md#install-upgrade) for a refresher on the details of what it does.

This also stabilizes the following changes:
- `--version` no longer allows an implicit version requirement like `1.2`.  It must be either contain all 3 components (like `1.2.3`) or use a requirement operator (like `^1.2`).  This has been a warning for a very long time, and is now changed to a hard error.
- Added `--no-track` to disable install tracking.

**Motivation**

I just personally prefer this behavior, and it has been requested a few times in the past. I've been using it locally, and haven't run into any issues. If this goes into 1.41, then it will release on Jan 30, about 10 months since it was added in #6798.

**Concerns**

Regarding some of the concerns I had:

- Is it tracking the correct set of information?

  I'm satisfied with the current set. It also tracks, but does not use, the version of rustc and the version specified in the `--version` flag, in case we ever want to use that in the future. It is also designed to be backwards and forwards compatible, so more information can be added in the future. I think the current set strikes a good balance of tracking the really important things, without causing unnecessary rebuilds.

- Method to upgrade all outdated packages?

  This can always be added as a new flag or command in the future, and shouldn't block stabilization.

- Should `--no-track` be kept? Does it work correctly?

  I kept it. It's not too hard to support, and nobody said anything (other than maybe using a less confusing name).

- Should this be the default? Should there be a way to use the old behavior?

  I like it as the default, and don't see a real need for the old behavior. I think we could always bring back the old behavior with a flag in the future, but I would like to avoid it for simplicity. There is also the workaround of `which foo || cargo install foo`.

Closes #6797.
Closes #6727.
Closes #6485.
Closes #2082.
@ehuss ehuss added this to the 1.36.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: cargo install with upgrades
5 participants