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

Eh2406 · 2023-10-04T22:42:52Z

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.

rustbot · 2023-10-04T22:42:57Z

r? @epage

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

epage · 2023-10-05T13:29:20Z

src/cargo/sources/registry/index.rs

@@ -437,11 +437,13 @@ impl<'cfg> RegistryIndex<'cfg> {
    /// checking the integrity of a downloaded package matching the checksum in
    /// the index file, aka [`IndexSummary`].
    pub fn hash(&mut self, pkg: PackageId, load: &mut dyn RegistryData) -> Poll<CargoResult<&str>> {
-        let req = OptVersionReq::exact(pkg.version());
+        let mut req = OptVersionReq::exact(pkg.version());
+        // Since crates.io allows crate versions to differ only by build metadata,


Because of rust-lang/crates.io#6518, should we change this to "Since some registries may allow"?

Or should we go a step further and make this a requirement on all registries? If not, should we at least document it as a best practice?

I am open to wordsmithing the comment.

I'm pretty sure we already documented it. If not we should.

Found it! https://doc.rust-lang.org/cargo/reference/resolver.html?highlight=metadata#version-metadata

In light of everything, maybe this should be "Since some registries may incorrectly allow ..."

(plus the other comment)

epage · 2023-10-05T13:32:16Z

src/cargo/sources/registry/index.rs

+        let mut req = OptVersionReq::exact(pkg.version());
+
+        // Since crates.io allows crate versions to differ only by build metadata,
+        // a query using OptVersionReq::exact + next() can return nondeterministic results.
+        // So we `lock_to` the exact version were interested in
+        req.lock_to(pkg.version());


nit: exact + lock_to seems like a weird dance. Should we have a new constructor for this purpose?

Yes. Sounds good.

epage · 2023-10-05T13:33:08Z

src/cargo/util/semver_ext.rs

-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn locked_has_the_same_with_exact() {
-        fn test_versions(target_ver: &str, vers: &[&str]) {
-            let ver = Version::parse(target_ver).unwrap();
-            let exact = OptVersionReq::exact(&ver);
-            let mut locked = exact.clone();
-            locked.lock_to(&ver);


Should we keep this test but change it so it requires metadata to be the same?

I think that's testing an implementation detail. If we want to add a test for this, we could extend the test added in #11447, to ensure that if we have a lock file we get the version specified in the lock file. I may give that a try.

I have expanded the test to make sure that the version from a lock file is used even if there other versions available from the registry. Confusingly, it passes on head. I am trying to determine what's going on.

Eh2406 · 2023-10-09T19:39:59Z

While attempting to implement the requested test I discovered that this PR does not change behavior. We apparently already had code that ensured that if a lock file was insufficient to uniquely identify a solution the resolver would prefer versions from the lockfile. Here. With that in place, a lock file containing semver build metadata will be respected through a somewhat circuitous route.

Lock file code will notice that the dependency requirement can be satisfied by a version in the lock file. It will therefore modify the dependency "lock_to" the particular version.
Despite having been "locked" multiple packages from the registry match the dependency requirement. (Because "locked" dependencies do not look at build metadata and there are more than one package that only differ on build metadata.)
The resolver is free to try/choose any matching package first. It happens to choose the one from the lock file so as to minimize the diff. And because package ID equality does depend on build metadata.

So we seem to have redundant mechanisms to ensure lock files are respected. Specifically, we have a mechanism that tell the resolver to prefer the versions from lock file AND we have a mechanism that locks dependencies to the version from lock file. The former is more elegant and simpler, but is not (yet?) capable of enforcing --locked nor determining when to skip network updates. Moving to only using the former is a laudable project but not in scope at this time.

In the meantime this PR makes the implementation of "locking a dependency to a version" actually match its description, while not changing any user facing behavior.

fix(replace): Partial-version spec support ### What does this PR try to resolve? #12614 changed package ID specs to allow fields in the version number to be optional. This earliest branch with this change is `rust-1.74.0` (beta). While `@Eh2406` was investigating version metadata issues in #12772, problems with the partial version change were found - `replace`s that specify version metadata were ignored **(fixed with this PR)** - This also extends out to any other place a PackageIDSpec may show up, like `cargo check -p <name>`@<spec>`` - We explicitly kept the same semantics of version requirements that pre-releases require opt-in. If nothing else, this gives us more room to change semantics in the future if we ever fix the semantics for pre-release. - `replace`s that don't specify version metadata when the `Cargo.lock` contained a version metadata, it would previously be ignored (with a warning) but now match **(unchanged with this PR)** - When the version metadata in `Cargo.lock` differed from the overriding `Cargo.toml`, cargo would panic **(now an error in this PR)** With this PR, we are acknowledging that we changed behavior in taking ignored replaces (because of differences with version metadata) and applying them. Seeing as version metadata is relatively rare, replaces are relatively rare, and differences in it for registries is unsupported, the impact seems very small. The questions before us are - Do we revert #12614 in `master` and `rust-1.74.0` or merge this PR into `master` - If we merge this PR into `master`, do we cherry-pick this into `rust-1.74.0` or revert #12614, giving ourselves more time to find problems ### How should we test and review this PR? The initial commit adds tests that pass as of #12614. Prior to #12614, these tests would have warned that the `replace` was unused and failed because `bar::bar` didn't exist. Each commit then changes the behavior (or not) and updates the corresponding test. ### Additional information

I think the core of the problem is that rust-lang@725420e was a mistake. Before that PR we used `=` constraints to imply that it came from lock file so when we saw a `=` constraints being constructed for a replace it should use the new system for marking something as coming from lock file. But I don't think that is the semantics that an `=` constraint implies in this context.

Eh2406 · 2023-10-19T19:07:31Z

This is unblocked, rebased, and ready for review.

I dealt with the issue of the new "locked means locked" semantics are not the ones we want for "replace" by not "locking" the replace dependencies. The plane = constraint gives the exact semantics that "replace" wants.

epage · 2023-10-19T19:18:08Z

@bors r+

bors · 2023-10-19T19:18:10Z

📌 Commit 98766fb has been approved by epage

It is now in the queue for this repository.

bors · 2023-10-19T19:19:16Z

⌛ Testing commit 98766fb with merge bf0a90d...

bors · 2023-10-19T20:11:44Z

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

Update cargo 22 commits in 8eb8acbb116e7923ea2ce33a50109933ed5ab375..d2f6a048529eb8e9ebc55d793abd63456c98fac2 2023-10-17 11:55:04 +0000 to 2023-10-20 18:25:30 +0000 - chore(deps): bump rustix from 0.38.18 to 0.38.19 (rust-lang/cargo#12851) - refactor: centralize logic of getting max resolve version (rust-lang/cargo#12860) - If there's a version in the lock file only use that exact version (rust-lang/cargo#12772) - Make the precise field of a source an Enum (rust-lang/cargo#12849) - fix(cli): Provide next steps for bad -Z flag (rust-lang/cargo#12857) - fix(remove): Preserve feature comments (rust-lang/cargo#12837) - docs: fix typo (rust-lang/cargo#12844) - chore(triagebot): auto label when PR review state changes (rust-lang/cargo#12856) - fix(add): Preserve more comments (rust-lang/cargo#12838) - ci: big ⚠️ to ensure the CNAME file is always there (rust-lang/cargo#12853) - docs(cargo-bench): `--bench` is passed in unconditionally to bench harnesses (rust-lang/cargo#12850) - docs(contrib): generate redirection HTML pages in CI (rust-lang/cargo#12846) - docs: remove review capacity notice (rust-lang/cargo#12842) - fix(help):Clarify install's positional (rust-lang/cargo#12841) - Adjust `-Zcheck-cfg` for new rustc syntax and behavior (rust-lang/cargo#12845) - fix(replace): Partial-version spec support (rust-lang/cargo#12806) - Print environment variables for build script executions with `-vv` (rust-lang/cargo#12829) - fix(cli): Suggest cargo-search on bad commands (rust-lang/cargo#12840) - docs(contrib): Policy on manifest editing (rust-lang/cargo#12836) - ci/contrib: use separate concurrency group (rust-lang/cargo#12835) - ci/contrib: do not fail on missing gh-pages (rust-lang/cargo#12834) - Clarify flag behavior in `cargo remove --help` (rust-lang/cargo#12823) r? ghost

rustbot assigned epage Oct 4, 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 Oct 4, 2023

Eh2406 mentioned this pull request Oct 4, 2023

move up looking at index summary enum #12749

Merged

epage reviewed Oct 5, 2023

View reviewed changes

Eh2406 force-pushed the Locked-means-locked branch from 2961584 to 48c6775 Compare October 9, 2023 19:48

epage mentioned this pull request Oct 11, 2023

fix(replace): Partial-version spec support #12806

Merged

This comment was marked as outdated.

Sign in to view

Eh2406 added 3 commits October 19, 2023 18:44

add test

6644828

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

98766fb

Eh2406 force-pushed the Locked-means-locked branch from 48c6775 to 98766fb Compare October 19, 2023 19:02

rustbot added the A-manifest Area: Cargo.toml issues label Oct 19, 2023

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 19, 2023

bors merged commit bf0a90d into rust-lang:master Oct 19, 2023
19 checks passed

Eh2406 deleted the Locked-means-locked branch October 20, 2023 03:05

weihanglo mentioned this pull request Oct 20, 2023

Update cargo rust-lang/rust#117000

Merged

ehuss added this to the 1.75.0 milestone Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Eh2406 commented Oct 4, 2023

rustbot commented Oct 4, 2023

epage Oct 5, 2023

Eh2406 Oct 5, 2023

Eh2406 Oct 5, 2023

epage Oct 9, 2023

epage Oct 9, 2023

epage Oct 5, 2023

Eh2406 Oct 5, 2023

epage Oct 5, 2023

Eh2406 Oct 5, 2023

Eh2406 Oct 6, 2023

Eh2406 commented Oct 9, 2023

This comment was marked as outdated.

This comment was marked as outdated.

Eh2406 commented Oct 19, 2023

epage commented Oct 19, 2023

bors commented Oct 19, 2023

bors commented Oct 19, 2023

bors commented Oct 19, 2023

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

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

Conversation

Eh2406 commented Oct 4, 2023

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Oct 4, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eh2406 commented Oct 9, 2023

This comment was marked as outdated.

This comment was marked as outdated.

Eh2406 commented Oct 19, 2023

epage commented Oct 19, 2023

bors commented Oct 19, 2023

bors commented Oct 19, 2023

bors commented Oct 19, 2023