move up looking at index summary enum #12749

Eh2406 · 2023-09-28T21:03:30Z

This builds on the work from #12643 and the discussion of the overall approach is at https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/better.20error.20messages.20for.20filtered.20versions.2E

What does this PR try to resolve?

This uses the enum added in #12643 to delay making decisions about yanked and –– precise versions until higher in the stack.

How should we test and review this PR?

This is an internal re-factoring and all the tests still pass. BUT, this is also tricky interleaved code where small changes may have important impacts to semantics.

Additional information

I will try and call out potentially breaking changes as inline comments. If any of these are controversial enough to deserve separate discussion I'm happy split it into separate PR's.

rustbot · 2023-09-28T21:03:35Z

r? @ehuss

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

Eh2406 · 2023-09-28T21:04:23Z

r? @epage

src/cargo/sources/registry/index.rs

Eh2406 · 2023-09-28T21:06:25Z

src/cargo/sources/registry/mod.rs

+            .precise_registry_version(dep.package_name().as_str())
+            .filter(|(c, _)| req.matches(c))
+        {
+            req.lock_to(&requested);


lock_to was changed have the same semantics as --precise, but it's possible we should have two different kind of locked requirements.

src/cargo/sources/registry/mod.rs

Eh2406 · 2023-09-28T21:11:48Z

src/cargo/sources/registry/index.rs

-                callback,
-                false
-            )?);
+            ready!(self.query_inner_with_online(name, req, load, callback, false)?);
            if called {


by moving filtering of yanked versions from beneath this code to above this code there may be situations in which called is now different. I don't think this will break anything, tests still pass, but it needs more thinking than I gave it.

So we query offline and see if anything came up, if it did, we move on. We then query online.

So I guess in theory if what the offline check found was yanked, then we might move on, filter it out, and then error instead of switching to on. Whether that can actually happen and cause problems needs a wider view of this stack than I have at this time. From how this is all behaving (this should likely only happen with locked packages which will be allow-listed), I'm assuming it will be fine?

That analysis sounds correct.

(this should likely only happen with locked packages which will be allow-listed)

I think the problematic case is that there is a downloaded version that has been yanked and is not in the allowed list. The old behavior would filter out the version because it yanked and not in the allow list, meaning that the first pass would see no candidates and so fallback to the second pass. Whereas, the new code will see the yanked version witch is available off-line so return it is the only candidate not falling back to the second pass.

So it looks like we are allowing yanked in that first pass, which is fine because its a lockfile, bypassing this problem. Do I have that right?

src/cargo/util/semver_ext.rs

src/cargo/sources/registry/mod.rs

bors · 2023-10-11T15:21:12Z

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

Eh2406 · 2023-10-16T17:31:20Z

For anyone following along this is block on #12772

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.

Eh2406 · 2023-10-24T18:43:53Z

Rebased and updated to have a different variant of OptVersionReq for the different semantics.

epage

Feel free to r= me when its ready

Eh2406 · 2023-10-24T18:57:01Z

I cannot reproduce the build failures locally.

Eh2406 · 2023-10-24T19:54:37Z

@bors r=epage

bors · 2023-10-24T19:54:39Z

📌 Commit 9ddaa61 has been approved by epage

It is now in the queue for this repository.

bors · 2023-10-24T19:55:46Z

⌛ Testing commit 9ddaa61 with merge 9f87523...

move up looking at index summary enum This builds on the work from #12643 and the discussion of the overall approach is at https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/better.20error.20messages.20for.20filtered.20versions.2E ### What does this PR try to resolve? This uses the enum added in #12643 to delay making decisions about yanked and `–– precise` versions until higher in the stack. ### How should we test and review this PR? This is an internal re-factoring and all the tests still pass. BUT, this is also tricky interleaved code where small changes may have important impacts to semantics. ### Additional information I will try and call out potentially breaking changes as inline comments. If any of these are controversial enough to deserve separate discussion I'm happy split it into separate PR's.

bors · 2023-10-24T20:11:41Z

💔 Test failed - checks-actions

Eh2406 · 2023-10-25T14:28:28Z

@bors r=epage

bors · 2023-10-25T14:28:30Z

📌 Commit 57b9372 has been approved by epage

It is now in the queue for this repository.

bors · 2023-10-25T14:29:36Z

⌛ Testing commit 57b9372 with merge 9bf67a1...

bors · 2023-10-25T15:27:51Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 9bf67a1 to master...

Update cargo 8 commits in df3509237935f9418351b77803df7bc05c009b3d..708383d620e183a9ece69b8fe930c411d83dee27 2023-10-24 23:09:01 +0000 to 2023-10-27 21:09:26 +0000 - feat(doc): Print the generated docs links (rust-lang/cargo#12859) - feat(toml): Allow version-less manifests (rust-lang/cargo#12786) - Remove outdated option to `-Zcheck-cfg` warnings (rust-lang/cargo#12884) - Remove duplicate binaries during install (rust-lang/cargo#12868) - refactor(shell): Write at once rather than in fragments (rust-lang/cargo#12880) - docs(ref): Link to docs.rs metadata table (rust-lang/cargo#12879) - docs(contrib): Describe how to add a new package (rust-lang/cargo#12878) - move up looking at index summary enum (rust-lang/cargo#12749) r? ghost

Bug 12920 Fix for #12920, a regression introduced in #12749. This also as a test case for an existing Terrible error message.

query{_vec} use IndexSummary This builds on the work from #12749 and the discussion of the overall approach is at https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/better.20error.20messages.20for.20filtered.20versions.2E ### What does this PR try to resolve? Changing the two traits `Registry` and `Source` to use the new `IndexSummary' involves a lot of changes all throughout the code base. This would be hard to review if it also included any logic changes. So this PR is just adding the type to the trait and immediately unwrapping every place it is used. The hope is that reviewing changes to logic/ergonomics will be easier to review once the mechanical changes have been merged. ### How should we test and review this PR? This is an internal re-factoring and all the tests still pass.

rustbot assigned ehuss Sep 28, 2023

rustbot added A-registries Area: registries A-semver Area: semver specifications, version matching, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2023

rustbot assigned epage and unassigned ehuss Sep 28, 2023

Eh2406 commented Sep 28, 2023

View reviewed changes

src/cargo/sources/registry/index.rs Outdated Show resolved Hide resolved

Eh2406 commented Sep 28, 2023

View reviewed changes

src/cargo/sources/registry/mod.rs Show resolved Hide resolved

Eh2406 commented Sep 28, 2023

View reviewed changes

src/cargo/sources/registry/mod.rs Show resolved Hide resolved

Eh2406 commented Sep 28, 2023

View reviewed changes

src/cargo/util/semver_ext.rs Outdated Show resolved Hide resolved

epage reviewed Sep 29, 2023

View reviewed changes

src/cargo/util/semver_ext.rs Outdated Show resolved Hide resolved

epage reviewed Sep 29, 2023

View reviewed changes

src/cargo/util/semver_ext.rs Outdated Show resolved Hide resolved

epage reviewed Sep 29, 2023

View reviewed changes

src/cargo/sources/registry/mod.rs Outdated Show resolved Hide resolved

epage reviewed Sep 29, 2023

View reviewed changes

src/cargo/sources/registry/mod.rs Show resolved Hide resolved

Eh2406 mentioned this pull request Oct 4, 2023

If there's a version in the lock file only use that exact version #12772

Merged

Eh2406 force-pushed the move_up branch from 5e9c374 to d0bb8f0 Compare October 9, 2023 19:39

Eh2406 force-pushed the move_up branch from d0bb8f0 to e85a330 Compare October 24, 2023 18:41

Eh2406 force-pushed the move_up branch from e85a330 to 9ddaa61 Compare October 24, 2023 18:49

epage approved these changes Oct 24, 2023

View reviewed changes

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 Oct 24, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 24, 2023

Eh2406 added 2 commits October 25, 2023 14:25

move handling precise up

dff1874

move yank handling up

57b9372

Eh2406 force-pushed the move_up branch from 9ddaa61 to 57b9372 Compare October 25, 2023 14:25

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 Oct 25, 2023

bors merged commit 9bf67a1 into rust-lang:master Oct 25, 2023
20 checks passed

weihanglo mentioned this pull request Oct 28, 2023

Update cargo rust-lang/rust#117302

Merged

Eh2406 deleted the move_up branch November 1, 2023 19:02

ehuss mentioned this pull request Nov 6, 2023

Panic at "cannot update precise" in rust-lang/rust #12920

Closed

ehuss added this to the 1.75.0 milestone Nov 6, 2023

Eh2406 mentioned this pull request Nov 6, 2023

Bug 12920 #12923

Merged

bors added a commit that referenced this pull request Nov 6, 2023

Auto merge of #12923 - Eh2406:bug_12920, r=epage

f095603

Bug 12920 Fix for #12920, a regression introduced in #12749. This also as a test case for an existing Terrible error message.

Eh2406 mentioned this pull request Nov 13, 2023

query{_vec} use IndexSummary #12970

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move up looking at index summary enum #12749

move up looking at index summary enum #12749

Eh2406 commented Sep 28, 2023

rustbot commented Sep 28, 2023

Eh2406 commented Sep 28, 2023

Eh2406 Sep 28, 2023

Eh2406 Sep 28, 2023 •

edited

Loading

epage Sep 29, 2023 •

edited

Loading

Eh2406 Oct 4, 2023

epage Oct 24, 2023

bors commented Oct 11, 2023

Eh2406 commented Oct 16, 2023

Eh2406 commented Oct 24, 2023

epage left a comment

Eh2406 commented Oct 24, 2023

Eh2406 commented Oct 24, 2023

bors commented Oct 24, 2023

bors commented Oct 24, 2023

bors commented Oct 24, 2023

Eh2406 commented Oct 25, 2023

bors commented Oct 25, 2023

bors commented Oct 25, 2023

bors commented Oct 25, 2023

move up looking at index summary enum #12749

move up looking at index summary enum #12749

Conversation

Eh2406 commented Sep 28, 2023

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Sep 28, 2023

Eh2406 commented Sep 28, 2023

Eh2406 Sep 28, 2023

Choose a reason for hiding this comment

Eh2406 Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

epage Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Eh2406 Oct 4, 2023

Choose a reason for hiding this comment

epage Oct 24, 2023

Choose a reason for hiding this comment

bors commented Oct 11, 2023

Eh2406 commented Oct 16, 2023

Eh2406 commented Oct 24, 2023

epage left a comment

Choose a reason for hiding this comment

Eh2406 commented Oct 24, 2023

Eh2406 commented Oct 24, 2023

bors commented Oct 24, 2023

bors commented Oct 24, 2023

bors commented Oct 24, 2023

Eh2406 commented Oct 25, 2023

bors commented Oct 25, 2023

bors commented Oct 25, 2023

bors commented Oct 25, 2023

Eh2406 Sep 28, 2023 •

edited

Loading

epage Sep 29, 2023 •

edited

Loading