Distinguish lockfile version req from normal dep in resolver error message #9847

weihanglo · 2021-08-27T16:47:34Z

Resolves #9840

This PR adds a new variant OptVersionReq::Locked as #9840 described.
The new variant represents as a locked version requirement that contains
an exact locked version and the original version requirement.

Previously we use exact version req to synthesize locked version req.
Now we make those need a truly locked to be OptVersionReq::Locked,
including:

[patch]: previous used exact version req to emulate locked version,
and it only need to lock dep version but not source ID, so we make
an extra method Dependency::lock_version for it.
Dependencies from lock files: No need to change the call site.

rust-highfive · 2021-08-27T16:47:37Z

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

weihanglo · 2021-08-27T16:48:14Z

TODO?

A new test case: I have no idea how to coin a test case to generate
resolver error with locked versions. Need some helps!
[replace] need to lock?: as aforementioned, [patch] locks deps.
Should we also make [replace] lock deps here? It seems not
required but I am not 100% sure.
solved with 725420e
I feel a bit uncertain in method matches for Locked variant.
Any optimization advice to remove the extra allocation?
solved with 4ecb543

weihanglo · 2021-08-27T17:00:11Z

r? @Eh2406
since you already involves in this issue 😆

Eh2406

This looks much like what I had in mind. Thank you!

I do want either @alexcrichton or @ehuss to double check that this is something we want. I came up with the idea, but I have missed things in the past.

Eh2406 · 2021-08-28T16:00:51Z

src/cargo/util/semver_ext.rs

@@ -49,22 +51,48 @@ impl OptVersionReq {
                    cmp.op == Op::Exact && cmp.minor.is_some() && cmp.patch.is_some()
                }
            }
+            OptVersionReq::Locked(..) => true,


Is the is_exact method still used anywhere? If sow why?

IIUC, for cargo install to decide whether to skip registry update or not. Though I found that some other duplicated logic laying there. 😂
https://github.com/rust-lang/cargo/pull/9847/files#diff-085d4f28bd6e49633caa467300082e94527f840dd487242b13bad2882a25451fR678

Eh2406 · 2021-08-28T16:05:46Z

src/cargo/util/semver_ext.rs

        }
    }

    pub fn matches(&self, version: &Version) -> bool {
        match self {
            OptVersionReq::Any => true,
            OptVersionReq::Req(req) => req.matches(version),
+            OptVersionReq::Locked(v, _) => VersionReq::exact(v).matches(version),


Can thes just be v == version? Or are there problems with the metadata part? Are there test to make sure we don't accidentally switch to the "simpler code" if it does not work?

Yes the metadata is counted in when comparing two Versions. We can reimplement the logic from our side since it is simple, but I am uncertain whether it is appropriate to do that.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=1c01a049bc0aba5850ff8e048c5536cf

I've updated the logic with 4ecb543. The test cases are shamelessly copied from semver crate. Do you think it works?

alexcrichton · 2021-09-08T15:14:58Z

FWIW to me this seems like a great idea. I agree though that it'd be best to reduce the dependence on is_exact if we can, since that was previously muddied between = and locked dependencies, but ideally we don't treat = dependencies any different than anything else.

`OptVersionReq::Locked` stores the original version requirement and the acutal locked version. This makes error message describing more precise.

Previously we use exact semver version req to synthesize locked version req. Now we make those need a truly locked to be `OptVersionReq::Locked`.

When encounter resolver error with locked dependency, we now show original version req along with the exact locked version.

Instead of creating another VersionReq, this can reduce some extra efforts on version matching logic. Though it also may introduces divergence from semver's matching logic. Accordingly, we borrow unit tests from semver crate and compare the matching results to prevent future breaks.

Eh2406 · 2021-10-05T15:59:11Z

I am sorry. I dropped the ball on this. It looks like Alex like the approach. @weihanglo did you have more clean ups in mind? Do you think this is ready to merge?

weihanglo · 2021-10-05T18:04:50Z

Yep, @Eh2406. I think after 4ecb543 and 725420e, it should be ready to merge.

One thing I am not sure is how to create a test case for the "(locked to a.b.c)" message? There is one already but do we need a dedicated one?

Eh2406 · 2021-10-05T18:16:36Z

Tests for the resolver error messages are woefully lacking. So, having one test that can see the change in behavior feels adequate. If you feel like adding more tests (in another PR) I would not object!

Thank you for working on this!
@bors r+

bors · 2021-10-05T18:16:38Z

📌 Commit 725420e has been approved by Eh2406

bors · 2021-10-05T18:16:44Z

⌛ Testing commit 725420e with merge 4a166de...

bors · 2021-10-05T18:48:59Z

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing 4a166de to master...

Update cargo 7 commits in d56b42c549dbb7e7d0f712c51b39400260d114d4..c7957a74bdcf3b11e7154c1a9401735f23ebd484 2021-09-27 13:44:18 +0000 to 2021-10-11 20:17:07 +0000 - Add some more information to verbose version. (rust-lang/cargo#9968) - Skip all `cargo fix` that tends to write to registry cache. (rust-lang/cargo#9938) - Stabilize named profiles (rust-lang/cargo#9943) - Update git2 (rust-lang/cargo#9963) - Distinguish lockfile version req from normal dep in resolver error message (rust-lang/cargo#9847) - nit: Allocated slightly bigger vec than needed (rust-lang/cargo#9954) - Add shell completion for shorthand commands (rust-lang/cargo#9951)

Use locked_version more In #9847 we added better tracking for when a requirement came from a lockfile. This uses that tracking in a few more error messages. Closes #10391

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.

rust-highfive assigned ehuss Aug 27, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 27, 2021

weihanglo changed the title ~~Issue 9840~~ Distinguish lockfile from normal =a.b.c dependencies in resolver error message Aug 27, 2021

weihanglo changed the title ~~Distinguish lockfile from normal =a.b.c dependencies in resolver error message~~ Distinguish lockfile version req from normal dep in resolver error message Aug 27, 2021

rust-highfive assigned Eh2406 and unassigned ehuss Aug 27, 2021

Eh2406 reviewed Aug 28, 2021

View reviewed changes

weihanglo added 4 commits September 20, 2021 11:31

Add Locked variant for OptVersionReq

d6fd499

`OptVersionReq::Locked` stores the original version requirement and the acutal locked version. This makes error message describing more precise.

Update synthetic locked version_req to real locked

c6762ce

Previously we use exact semver version req to synthesize locked version req. Now we make those need a truly locked to be `OptVersionReq::Locked`.

Show original version_req for locked dependency

d226234

When encounter resolver error with locked dependency, we now show original version req along with the exact locked version.

weihanglo force-pushed the issue-9840 branch from 66fcbdd to 4ecb543 Compare September 20, 2021 03:38

Lock [replace] source as well

725420e

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 5, 2021

bors merged commit 4a166de into rust-lang:master Oct 5, 2021

ehuss mentioned this pull request Oct 12, 2021

Update cargo rust-lang/rust#89802

Merged

weihanglo deleted the issue-9840 branch December 4, 2021 23:46

ehuss added this to the 1.57.0 milestone Feb 6, 2022

Eh2406 mentioned this pull request Mar 2, 2022

Use locked_version more #10449

Merged

This was referenced Sep 28, 2023

move up looking at index summary enum #12749

Merged

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distinguish lockfile version req from normal dep in resolver error message #9847

Distinguish lockfile version req from normal dep in resolver error message #9847

weihanglo commented Aug 27, 2021

rust-highfive commented Aug 27, 2021

weihanglo commented Aug 27, 2021 •

edited

Loading

weihanglo commented Aug 27, 2021

Eh2406 left a comment

Eh2406 Aug 28, 2021

weihanglo Aug 28, 2021

Eh2406 Aug 28, 2021

weihanglo Aug 28, 2021

weihanglo Sep 20, 2021

alexcrichton commented Sep 8, 2021

Eh2406 commented Oct 5, 2021

weihanglo commented Oct 5, 2021

Eh2406 commented Oct 5, 2021

bors commented Oct 5, 2021

bors commented Oct 5, 2021

bors commented Oct 5, 2021

Distinguish lockfile version req from normal dep in resolver error message #9847

Distinguish lockfile version req from normal dep in resolver error message #9847

Conversation

weihanglo commented Aug 27, 2021

rust-highfive commented Aug 27, 2021

weihanglo commented Aug 27, 2021 • edited Loading

TODO?

weihanglo commented Aug 27, 2021

Eh2406 left a comment

Choose a reason for hiding this comment

Eh2406 Aug 28, 2021

Choose a reason for hiding this comment

weihanglo Aug 28, 2021

Choose a reason for hiding this comment

Eh2406 Aug 28, 2021

Choose a reason for hiding this comment

weihanglo Aug 28, 2021

Choose a reason for hiding this comment

weihanglo Sep 20, 2021

Choose a reason for hiding this comment

alexcrichton commented Sep 8, 2021

Eh2406 commented Oct 5, 2021

weihanglo commented Oct 5, 2021

Eh2406 commented Oct 5, 2021

bors commented Oct 5, 2021

bors commented Oct 5, 2021

bors commented Oct 5, 2021

weihanglo commented Aug 27, 2021 •

edited

Loading