Try installing exact versions before updating #8022

illicitonion · 2020-03-20T00:16:51Z

When an exact version is being installed, if we already have that
version from the index, we don't need to update the index before
installing it. Don't do this if it's not an exact version, because the
update may find us a newer version.

This is particularly useful for scripts which unconditionally run
cargo install some-crate --version=1.2.3. Before install-update, I
wrote a crate to do this
(https://crates.io/crates/cargo-ensure-installed) which I'm trying to
replace with just cargo install, but the extra latency of updating the
index for a no-op is noticeable.

rust-highfive · 2020-03-20T00:16:54Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

illicitonion · 2020-03-20T00:17:27Z

Cargo.toml

@@ -53,7 +53,7 @@ percent-encoding = "2.0"
 remove_dir_all = "0.5.2"
 rustfix = "0.5.0"
 same-file = "1"
-semver = { version = "0.9.0", features = ["serde"] }
+semver = { git = "https://github.com/illicitonion/semver.git", rev = "f1f912703f67ed63b751c525839731a90239bdcf", features = ["serde"] }


I'd probably want an upstream release of this (including dtolnay/semver#205) before merging this PR, but figured I'd see if there was appetite for it first.

illicitonion · 2020-03-20T00:18:47Z

src/cargo/ops/common_for_install_and_uninstall.rs

+                    }
+                }
+            }
+            if let NeedsUpdate::TryWithoutFirst = needs_update {


This enum variant is weirdly tightly coupled to the style of source being used, but the args here already seem pretty tightly coupled to their caller styles. I tried a few different ways of factoring this out to avoid that, and none of them seemed great...

When an exact version is being installed, if we already have that version from the index, we don't need to update the index before installing it. Don't do this if it's not an exact version, because the update may find us a newer version. This is particularly useful for scripts which unconditionally run `cargo install some-crate --version=1.2.3`. Before install-update, I wrote a crate to do this (https://crates.io/crates/cargo-ensure-installed) which I'm trying to replace with just `cargo install`, but the extra latency of updating the index for a no-op is noticeable. This introduces an interesting edge-case around yanked crates; the yanked-ness of crates will no longer change on install for exact version matches which were already downloaded. This feels niche enough to hopefully not matter...

alexcrichton

Could you add some tests here as well for this behavior?

The point about yanks feels pretty important though? It's basically subverting yanking behavior because you'll be able to install a yanked crate with --vers =a.b.c so long as you haven't updated the index?

src/cargo/ops/cargo_install.rs

src/cargo/ops/common_for_install_and_uninstall.rs

illicitonion · 2020-03-21T00:47:27Z

Thanks for the review! :) I restructured so that we instead just do a "is this already installed" check, and bail out early if so, by extracting some helper functions. Accordingly, the yank behaviour is fixed too (and I added a test for it).

It's not clear to me whether the helper functions are exactly the right ones - I can imagine some of them could either be bigger or smaller, but I don't have a great instinct for the abstractions in cargo to know what naturally groups together. I'm very happy to change them around with a little guidance.

I also added some tests :)

ehuss · 2020-03-26T21:42:09Z

src/cargo/ops/common_for_install_and_uninstall.rs

-            vers
-        };
-        let dep = Dependency::parse_no_deprecated(name, vers_spec, source.source_id())?;
+        let vers = vers.map(|v| v.to_string());


I'm a bit uncertain, why take a string, parse it to a VersionReq, only to immediately convert it back to a string where parse_no_deprecated will parse it again? Couldn't select_pkg keep the Option<&str> to avoid that?

The previous code did the same, so I was trying to preserve the exact semantics. If you take a look at the removed code just above this line, it roughly does:
if looks like a semver, parse it as one, then convert it back to string
else parse it to make sure it's a semver and then use the string after converting it to a VersionReq and manually re-implementing tostring for it (the format!("={}", v))

I'm worried that if we just pass the &str through a side-channel, it will skip running some important code.

I can happily read through parse_no_deprecated and work out why we're doing the str -> VersionReq -> str conversion and maybe remove that, but it feels like that would be a separate change :)

ehuss · 2020-03-26T21:44:05Z

src/cargo/ops/cargo_install.rs

            path.read_packages()
        })?
    } else {
+        let mut source = map.load(source_id, &HashSet::new())?;


This function is already pretty long and gnarly. Can this new section of code somehow be moved out to another function?

Yeah, that's fair - because of how many shared variables there are between this code and the whole rest of the method, and because we're early-returning based on the code, I was leaving it in-line, and trying to re-use some variables in the closure, but I've pulled it out... We should probably break this up in general, I guess :)

ehuss · 2020-03-26T21:45:04Z

src/cargo/ops/cargo_install.rs

+        } else if source.source_id().is_registry() {
+            Some(VersionReq::any())


I'm a bit confused by this check. Why is it needed if the code below immediately checks for is_exact?

vers is then passed to select_pkg. I've re-structured this to make it a bit more obvious :)

There is something really subtle here, so I think it might be worth changing this. From a naive look, select_pkg with a version of None should use the "any" requirement implicitly as parse_no_deprecated does this. However, due to the round-trip of string->VersionReq->string->VersionReq, the "any" requirement gets changed to "*" which is different from VersionReq::any, as "*" means "do not select pre-release versions" (see dtolnay/semver#161). This is behavior Cargo relies on.

I'd like this to be a bit more explicit. I'm thinking, change it back so that select_pkg takes Option<&str>, and structure it more like it used to be. That way, there is an explicit step (where the old comment Avoid pre-release versions was) that can very clearly call out this behavior. That way there isn't this distant assumption about what VersionReq::any means. installed_exact_package can throw away whatever work it does to determine if it is an exact version, I don't think it helps to pass in a VersionReq to select_pkg.

src/cargo/ops/cargo_install.rs

ehuss

Sorry for the delay!

src/cargo/ops/cargo_install.rs

ehuss · 2020-04-05T17:14:30Z

src/cargo/ops/cargo_install.rs

+        } else if source.source_id().is_registry() {
+            Some(VersionReq::any())


There is something really subtle here, so I think it might be worth changing this. From a naive look, select_pkg with a version of None should use the "any" requirement implicitly as parse_no_deprecated does this. However, due to the round-trip of string->VersionReq->string->VersionReq, the "any" requirement gets changed to "*" which is different from VersionReq::any, as "*" means "do not select pre-release versions" (see dtolnay/semver#161). This is behavior Cargo relies on.

I'd like this to be a bit more explicit. I'm thinking, change it back so that select_pkg takes Option<&str>, and structure it more like it used to be. That way, there is an explicit step (where the old comment Avoid pre-release versions was) that can very clearly call out this behavior. That way there isn't this distant assumption about what VersionReq::any means. installed_exact_package can throw away whatever work it does to determine if it is an exact version, I don't think it helps to pass in a VersionReq to select_pkg.

Handling of these is coupled, so do the handling in one place, close to where we parse the command line flags, so we can just pass in a single derived object.

It only ever actually uses one, so let's reflect that in the types.

illicitonion · 2020-04-13T02:08:29Z

Thanks for the review! I progressively got the signature of select_pkg into something that seems more reasonable to me - it now both reflects that name and list_all are exclusive, and pushes the coupled name + version parsing into one place, though I can switch it back to be closer to the previous logic & locations if you'd prefer.

Individual commits are probably useful to review separately.

bors · 2020-04-20T14:42:48Z

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

ehuss · 2020-04-28T02:54:04Z

Oh, nevermind, I missed the merge commit from last week.

bors · 2020-04-29T21:37:36Z

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

Remove enum, pull out function and dependent function

alexcrichton · 2020-05-04T15:38:31Z

r? @ehuss

Switching reviewers since it looks like @ehuss has a better handle on this than I

src/cargo/ops/cargo_install.rs

ehuss · 2020-05-08T21:07:10Z

Thanks! This looks good to me. Just waiting on getting semver update. I pinged Steve about it, and he said he'll try to take a look at it next week.

Eh2406 · 2020-05-09T01:40:15Z

We already have a hack in the code for is_exact:

cargo/src/cargo/core/dependency.rs

Lines 395 to 398 in ae08099

    
           pub fn is_locked(&self) -> bool { 
        
               // Kind of a hack to figure this out, but it works! 
        
               self.inner.req.to_string().starts_with('=') 
        
           }

If it is added to upstream we should remove our use of that hack.
Edit I see the difference now. Sorry for the noise.

illicitonion · 2020-05-09T15:46:27Z

We already have a hack in the code for is_exact:

cargo/src/cargo/core/dependency.rs

Lines 395 to 398 in ae08099

pub fn is_locked(&self) -> bool {

// Kind of a hack to figure this out, but it works!

self.inner.req.to_string().starts_with('=')

}

If it is added to upstream we should remove our use of that hack.
Edit I see the difference now. Sorry for the noise.

Ooh - it reads to me like my new code should probably also call is_locked, and we should probably update the is_locked implementation to use the new semver function if it's accepted... Does that sound reasonable? (Alternatives being: Leave these as two independent things, or inline the new semver is_exact function where is_locked is currently called)

bors · 2020-05-14T14:31:29Z

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

ehuss · 2020-05-20T18:44:15Z

@illicitonion if you want to switch it to do something similar to is_locked and remove the semver bump, we can probably just land this now, and update it later when a new semver release is made.

I don't care too much if it is inlined or a shared utility function, whatever is easiest.

semver hasn't merged the upstream PR (yet)

illicitonion · 2020-05-20T20:40:29Z

@illicitonion if you want to switch it to do something similar to is_locked and remove the semver bump, we can probably just land this now, and update it later when a new semver release is made.

I don't care too much if it is inlined or a shared utility function, whatever is easiest.

Done and green - thanks @ehuss!

ehuss · 2020-05-20T20:48:39Z

Thanks!
@bors r+

bors · 2020-05-20T20:48:41Z

📌 Commit b719272 has been approved by ehuss

bors · 2020-05-20T20:48:49Z

⌛ Testing commit b719272 with merge 9b94513...

bors · 2020-05-20T21:17:57Z

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 9b94513 to master...

Update cargo 7 commits in 500b2bd01c958f5a33b6aa3f080bea015877b83c..9fcb8c1d20c17f51054f7aa4e08ff28d381fe096 2020-05-18 17:12:54 +0000 to 2020-05-25 16:25:36 +0000 - Bump to semver 0.10 for `VersionReq::is_exact` (rust-lang/cargo#8279) - Fix panic with `cargo tree --target=all -Zfeatures=all` (rust-lang/cargo#8269) - Fix nightly tests with llvm-tools. (rust-lang/cargo#8272) - Provide better error messages for a bad `patch`. (rust-lang/cargo#8248) - Try installing exact versions before updating (rust-lang/cargo#8022) - Document unstable `strip` profile feature (rust-lang/cargo#8262) - Add option to strip binaries (rust-lang/cargo#8246)

rust-highfive assigned alexcrichton Mar 20, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 20, 2020

illicitonion commented Mar 20, 2020

View reviewed changes

illicitonion force-pushed the trywithout branch from c559ef8 to cd81fe9 Compare March 20, 2020 14:07

Update to semver forked from newer master

e8b0344

illicitonion force-pushed the trywithout branch from cd81fe9 to 10c1240 Compare March 20, 2020 14:19

illicitonion force-pushed the trywithout branch from 10c1240 to a721e62 Compare March 20, 2020 14:36

alexcrichton reviewed Mar 20, 2020

View reviewed changes

src/cargo/ops/cargo_install.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 Show resolved Hide resolved

illicitonion added 3 commits March 21, 2020 00:04

Explicitly check whether installed first instead

137e518

Merge branch 'master' into trywithout

598183e

Fix up updating logic for multiple installs

1e2c233

ehuss reviewed Mar 26, 2020

View reviewed changes

illicitonion added 2 commits March 28, 2020 02:05

Review comments

14e3e41

Remove unnecessary lifetime parameter

3c96c05

ehuss reviewed Apr 5, 2020

View reviewed changes

ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 8, 2020

illicitonion added 4 commits April 13, 2020 02:41

Commenty review comments

7a83d61

select_pkg takes a Dependency not name + vers

6eef9a8

Handling of these is coupled, so do the handling in one place, close to where we parse the command line flags, so we can just pass in a single derived object.

select_pkg takes Dependency _or_ list_all not both

bf6a627

It only ever actually uses one, so let's reflect that in the types.

Merge branch 'master' into trywithout

0a38196

illicitonion added 2 commits April 14, 2020 00:22

Merge branch 'master' into trywithout

7b8d9b3

Add a little docstring

4432ac3

ehuss added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Apr 14, 2020

illicitonion added 3 commits May 2, 2020 11:50

Merge branch 'master' into trywithout

624ce68

Simplify

d2b2775

Remove enum, pull out function and dependent function

Simplify git_package/pkg handling

9d1c6a7

rust-highfive assigned ehuss and unassigned alexcrichton May 4, 2020

ehuss reviewed May 8, 2020

View reviewed changes

src/cargo/ops/cargo_install.rs Outdated Show resolved Hide resolved

Remove stray debugging line

18ceb9f

Merge branch 'master' into trywithout

8064909

Switch to using existing is_locked implementation

b719272

semver hasn't merged the upstream PR (yet)

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 May 20, 2020

bors merged commit 9b94513 into rust-lang:master May 20, 2020

illicitonion deleted the trywithout branch May 20, 2020 21:30

ehuss mentioned this pull request May 26, 2020

Update cargo rust-lang/rust#72596

Merged

ehuss mentioned this pull request Jul 18, 2020

Add space in flag in release notes #8505

Closed

ehuss added this to the 1.45.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try installing exact versions before updating #8022

Try installing exact versions before updating #8022

illicitonion commented Mar 20, 2020 •

edited

Loading

rust-highfive commented Mar 20, 2020

illicitonion Mar 20, 2020

illicitonion Mar 20, 2020

alexcrichton left a comment

illicitonion commented Mar 21, 2020

ehuss Mar 26, 2020

illicitonion Mar 28, 2020

ehuss Mar 26, 2020

illicitonion Mar 28, 2020

ehuss Mar 26, 2020

illicitonion Mar 28, 2020

ehuss Apr 5, 2020

ehuss left a comment

ehuss Apr 5, 2020

illicitonion commented Apr 13, 2020

bors commented Apr 20, 2020

ehuss commented Apr 28, 2020

bors commented Apr 29, 2020

alexcrichton commented May 4, 2020

ehuss commented May 8, 2020

Eh2406 commented May 9, 2020 •

edited

Loading

illicitonion commented May 9, 2020

bors commented May 14, 2020

ehuss commented May 20, 2020

illicitonion commented May 20, 2020

ehuss commented May 20, 2020

bors commented May 20, 2020

bors commented May 20, 2020

bors commented May 20, 2020

		} else if source.source_id().is_registry() {
		Some(VersionReq::any())

Try installing exact versions before updating #8022

Try installing exact versions before updating #8022

Conversation

illicitonion commented Mar 20, 2020 • edited Loading

rust-highfive commented Mar 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton left a comment

Choose a reason for hiding this comment

illicitonion commented Mar 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illicitonion commented Apr 13, 2020

bors commented Apr 20, 2020

ehuss commented Apr 28, 2020

bors commented Apr 29, 2020

alexcrichton commented May 4, 2020

ehuss commented May 8, 2020

Eh2406 commented May 9, 2020 • edited Loading

illicitonion commented May 9, 2020

bors commented May 14, 2020

ehuss commented May 20, 2020

illicitonion commented May 20, 2020

ehuss commented May 20, 2020

bors commented May 20, 2020

bors commented May 20, 2020

bors commented May 20, 2020

illicitonion commented Mar 20, 2020 •

edited

Loading

Eh2406 commented May 9, 2020 •

edited

Loading