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 cargo install with a semver metadata version. #9467

Merged
merged 1 commit into from
May 10, 2021

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented May 8, 2021

This fixes an issue where cargo install cargo-c --version 0.8.0+cargo-0.51 fails (returns a 404 error when downloading) when the index has not yet been populated through other means. The crux of the issue is that the PackageId interner was treating 0.8.0+cargo-0.51 and 0.8.0 the same. Due to a chain of events, the interner was getting populated with 0.8.0 first, and then from there on always returned 0.8.0. The full version information is needed to construct the download URL, so it was failing.

The reason the interner was getting populated with a version without the metadata is the following sequence of events:

  1. There is this "fast path" code path which checks if a version of a package is already installed before updating the index.
  2. Since the index doesn't exist yet, the resolver query returns zero entries (because the Registry Source is empty) here.
  3. That code checks if the package has been yanked (because it can't tell the difference between "yanked" and "index not downloaded, yet").
  4. It constructs a PackageId using a VersionReq where the build metadata has been removed (because version reqs don't have build metadata).
  5. When the real install continues (the error here is ignored for the purpose of this fast-path check if it is already installed), it downloads the index. However, the PackageId values created when parsing the index JSON files are now missing the build metadata because the interner is returning the wrong entries.
  6. When the download starts, the URL is built from the PackageId missing the build metadata.

I only changed PackageIdInner to pay attention to the build metadata. This seems a bit fragile, as perhaps PackageId should also pay attention to it. However, I don't really want to do an audit of every use of PackageId, and offhand I can't think of other situations where it would matter.

Closes #9410

@rust-highfive
Copy link

r? @alexcrichton

(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 May 8, 2021
@ehuss ehuss force-pushed the install-metadata branch from 93696cf to 9387a30 Compare May 9, 2021 00:23
@alexcrichton
Copy link
Member

@bors: r+

Thanks for tracking this down!

@bors
Copy link
Contributor

bors commented May 10, 2021

📌 Commit 9387a30 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 May 10, 2021
@bors
Copy link
Contributor

bors commented May 10, 2021

⌛ Testing commit 9387a30 with merge 4592422...

@bors
Copy link
Contributor

bors commented May 10, 2021

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 4592422 to master...

@bors bors merged commit 4592422 into rust-lang:master May 10, 2021
bors added a commit that referenced this pull request May 10, 2021
Bump index cache version to deal with semver metadata version mismatch.

#9467 has uncovered an issue with how versions are handled in the index cache.  When using a debug build of Cargo, it may panic due to the [cache contents changing](https://github.com/rust-lang/cargo/blob/5c455130b6001c7f54e872e161c27f6e996aff1f/src/cargo/sources/registry/index.rs#L606-L619).  The particular problem I am running into is that the index has an entry for `openssl-src 110.0.0` and `110.0.0+1.1.0f`. This is due to an issue with crates.io where it allows publishing multiple versions with the same metadata (rust-lang/crates.io#1059).  Cargos before #9467 would populate the index cache with two entries, both with version `110.0.0`.  Afterwards, there are two separate entries (`110.0.0` and `110.0.0+1.1.0f`).

The change here is to bump the index cache version so that new versions of cargo will clear the cache, and won't trigger the assertion.

This may be a bit of a heavy-handed solution, as I think this only affects debug builds of cargo.  However, I instantly started running into this problem, so I suspect it will be a real annoyance for anyone developing cargo. Happy to discuss other possible solutions.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 12, 2021
Update cargo

8 commits in e51522ab3db23b0d8f1de54eb1f0113924896331..070e459c2d8b79c5b2ac5218064e7603329c92ae
2021-05-07 21:29:52 +0000 to 2021-05-11 18:12:23 +0000
- Fix rustdoc warnings (rust-lang/cargo#9468)
- Improve performance of git status check in `cargo package`. (rust-lang/cargo#9478)
- Link to the new rustc tests chapter. (rust-lang/cargo#9477)
- Bump index cache version to deal with semver metadata version mismatch. (rust-lang/cargo#9476)
- Fix Url::into_string deprecation warning (rust-lang/cargo#9475)
- Fix rust-lang/cargo#4482 and rust-lang/cargo#9449: set Fossil ignore and clean settings locally (rust-lang/cargo#9469)
- Improve two error messages (rust-lang/cargo#9472)
- Fix `cargo install` with a semver metadata version. (rust-lang/cargo#9467)
@ehuss ehuss added this to the 1.54.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.

cargo can't install crates with a version string including the + character
4 participants