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.

Sign up for GitHub

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

Jump to bottom

Crate checksum lookup query should match on semver build metadata #11447

Merged
merged 2 commits into from
Aug 15, 2023
Merged

Crate checksum lookup query should match on semver build metadata #11447

merged 2 commits into from
Aug 15, 2023

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Dec 1, 2022

Since crates.io allows crate versions to differ only by build metadata, a query using OptVersionReq::exact + next() can return nondeterministic results.

This change fixes the issue by adding an additional filter that ensures the version is equal (including build metadata).

It still feels somewhat wrong that a query using exact can match multiple crates, so an alternative fix would be to add a new variant of OptVersionReq that also matched on build metadata.

Fixes #11412

@rustbot
Copy link
Collaborator

rustbot commented Dec 1, 2022

r? @weihanglo

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2022
@Eh2406
Copy link
Contributor

Eh2406 commented Dec 1, 2022

This change fixes the issue by adding an additional filter that ensures the version is equal (including build metadata).

Is equal to what? A version requirement can't specify metadata.

How does the non-determinism cause problems except when we have a Locked version? I.E. If we made a new OptVersionReq when would we use it?

Also, there should be tests.

@arlosi
Copy link
Contributor Author

arlosi commented Dec 7, 2022

In both the places changed by this PR, we already have Version that we're trying to look up in the registry. Currently that's using a query with OptVersionReq::exact, and taking the first result.

Unfortunately, this query can match multiple crates that differ only by build metadata, and may return the incorrect one (where the metadata doesn't match what's in the Version we have). The crate returned by the query appears to be nondeterministic (probably due to HashMap), so sometimes the correct crate is returned and other times not.

This issue may be more complex to fix than this PR. This PR only fixes the issue of mixing up which checksum a given crate has, and whether a crate has been yanked.

If we made a new OptVersionReq when would we use it?

We could introduce a new variant of OptVersionReq that also matched on the build metadata. It would be used when the resolver had already decided on the specific version to be used, and we just needed to look up additional information about the crate (such as its checksum). We could also change the OptVersionReq::Locked to work this way, but I don't know what other implications that would have.

@Eh2406
Copy link
Contributor

Eh2406 commented Dec 7, 2022

Okay. I'm convinced that what we have now is just wrong. And that this is an incremental improvement, I don't think we need to let the perfect be the enemy of the good here.

I'd prefer if there was some kind of test, is there some way to make that happen?

@weihanglo weihanglo 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 Dec 21, 2022
@weihanglo
Copy link
Member

Ping @arlosi. Have you got time adding tests for this change? Thank you!

@bors
Copy link
Collaborator

bors commented Apr 6, 2023

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

@weihanglo
Copy link
Member

@arlosi, just checking in to see if you are still wanting to land this. Adding a test or two is suffcient I believe.

@rustbot rustbot added the A-registries Area: registries label Aug 11, 2023
@arlosi
Copy link
Contributor Author

arlosi commented Aug 11, 2023 •
edited
Loading

@weihanglo thanks for the reminder. I've added a test that I confirmed fails without the change.

@rustbot ready

@rustbot rustbot 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 Aug 11, 2023
weihanglo
weihanglo approved these changes Aug 15, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. Looks great, just the duplication of filter is a thing we may want to improve, as you said, by a new variant or something else.

@weihanglo
Copy link
Member

@Eh2406 do you have any comment on the latest change?

@Eh2406
Copy link
Contributor

Eh2406 commented Aug 15, 2023

@bor r=weihanglo

@weihanglo
Copy link
Member

You tagged the wrong user 😆.

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 15, 2023

📌 Commit fb98f3f has been approved by weihanglo

It is now in the queue for this repository.

@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 Aug 15, 2023
@bors
Copy link
Collaborator

bors commented Aug 15, 2023

⌛ Testing commit fb98f3f with merge a11f624...

@bors
Copy link
Collaborator

bors commented Aug 15, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing a11f624 to master...

@bors bors merged commit a11f624 into rust-lang:master Aug 15, 2023
19 checks passed
@weihanglo weihanglo mentioned this pull request Aug 18, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 19, 2023
@matthiaskrgr
Rollup merge of rust-lang#114979 - weihanglo:update-cargo, r=weihanglo
68005a8 
Update cargo

15 commits in 7c3904d6c3ed54e8a413023519b55a536ad44d5b..80eca0e58fb2ff52c1e94fc191b55b37ed73e0e4
2023-08-14 20:11:43 +0000 to 2023-08-19 00:52:06 +0000
- chore: Downgrade serde below the binary blob (rust-lang/cargo#12528)
- Improve error message for when no credential providers are available (rust-lang/cargo#12526)
- Fix typo: "use" -> "used" (rust-lang/cargo#12522)
- Document layout SemVer compatibility. (rust-lang/cargo#12169)
- Make cargo-credential-gnome-secret built-in as cargo:libsecret (rust-lang/cargo#12521)
- login: allow passing additional args to provider (rust-lang/cargo#12499)
- cargo-credential-gnome-secret: dynamically load libsecret (rust-lang/cargo#12518)
- credential-providers: make 1password no longer built-in (rust-lang/cargo#12507)
- Print environment variables for `cargo run` in extra verbose mode (rust-lang/cargo#12498)
- chore(cargo-util): bump version to 0.2.6 (rust-lang/cargo#12517)
- credential: rename cargo:basic to cargo:token-from-stdout (rust-lang/cargo#12512)
- fix(xtask-bump-check): query by package name to detect changes (rust-lang/cargo#12513)
- ci: use pull request head commit whenever possible (rust-lang/cargo#12508)
- Update hermit-abi (rust-lang/cargo#12504)
- Crate checksum lookup query should match on semver build metadata (rust-lang/cargo#11447)

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 19, 2023
@bors
Auto merge of rust-lang#114979 - weihanglo:update-cargo, r=weihanglo
6ef7d16 
Update cargo

15 commits in 7c3904d6c3ed54e8a413023519b55a536ad44d5b..80eca0e58fb2ff52c1e94fc191b55b37ed73e0e4
2023-08-14 20:11:43 +0000 to 2023-08-19 00:52:06 +0000
- chore: Downgrade serde below the binary blob (rust-lang/cargo#12528)
- Improve error message for when no credential providers are available (rust-lang/cargo#12526)
- Fix typo: "use" -> "used" (rust-lang/cargo#12522)
- Document layout SemVer compatibility. (rust-lang/cargo#12169)
- Make cargo-credential-gnome-secret built-in as cargo:libsecret (rust-lang/cargo#12521)
- login: allow passing additional args to provider (rust-lang/cargo#12499)
- cargo-credential-gnome-secret: dynamically load libsecret (rust-lang/cargo#12518)
- credential-providers: make 1password no longer built-in (rust-lang/cargo#12507)
- Print environment variables for `cargo run` in extra verbose mode (rust-lang/cargo#12498)
- chore(cargo-util): bump version to 0.2.6 (rust-lang/cargo#12517)
- credential: rename cargo:basic to cargo:token-from-stdout (rust-lang/cargo#12512)
- fix(xtask-bump-check): query by package name to detect changes (rust-lang/cargo#12513)
- ci: use pull request head commit whenever possible (rust-lang/cargo#12508)
- Update hermit-abi (rust-lang/cargo#12504)
- Crate checksum lookup query should match on semver build metadata (rust-lang/cargo#11447)

r? ghost
@ehuss ehuss added this to the 1.73.0 milestone Aug 22, 2023
@Eh2406 Eh2406 mentioned this pull request Oct 4, 2023
Merged
bors added a commit that referenced this pull request Oct 19, 2023
@bors
Auto merge of #12772 - Eh2406:Locked-means-locked, r=epage
bf0a90d 
If there's a version in the lock file only use that exact version

### What does this PR try to resolve?

Generally, cargo is of the opinion that semver metadata should be ignored.
It's is relevant to dependency resolution as any other description of the version, just like the description field in cargo.toml.
If your registry has two versions that only differing metadata you get the bugs you deserve.
We implement functionality to make it easier for our users or for us to maintain.

~~So let's use `==` because it's less code to write and test.~~

We also believe that lock files should ensure reproducibility
 and protect against mutations from the registry.
 In this circumstance these two goals are in conflict, and this PR picks reproducibility.
 If the lock file tells us that there is a version called `1.0.0+bar` then
 we should not silently use `1.0.0+foo` even though they have the same version.

This is one of the alternatives/follow-ups discussed in #11447.
It was separated from #12749, to allow for separate discussion and because I was being too clever by half.

### How should we test and review this PR?

All tests still pass except for the ones that were removed. The removed tests were only added to verify the on intuitive behavior of the older implementation in #9847.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
A-registries Area: registries S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.

Checksum of yanked version causing crate download to fail
6 participants
@arlosi @rustbot @Eh2406 @weihanglo @bors @ehuss