From 6644828028f52f84dedec07410eaf3f837fa7e88 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Thu, 19 Oct 2023 18:44:42 +0000 Subject: [PATCH 1/3] add test --- src/cargo/core/registry.rs | 2 +- tests/testsuite/registry.rs | 51 +++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index d7ecdf59bca..a91f2986a94 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -116,7 +116,7 @@ enum Kind { /// directive that we found in a lockfile, if present. pub struct LockedPatchDependency { /// The original `Dependency` directive, except "locked" so it's version - /// requirement is `=foo` and its `SourceId` has a "precise" listed. + /// requirement is Locked to `foo` and its `SourceId` has a "precise" listed. pub dependency: Dependency, /// The `PackageId` that was previously found in a lock file which /// `dependency` matches. diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index f485180c9bd..b5dff2746bf 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -3597,6 +3597,57 @@ fn differ_only_by_metadata() { [CHECKING] baz v0.0.1+b [CHECKING] foo v0.0.1 ([CWD]) [FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s +", + ) + .run(); + + Package::new("baz", "0.0.1+d").publish(); + + p.cargo("clean").run(); + p.cargo("check") + .with_stderr_contains("[CHECKING] baz v0.0.1+b") + .run(); +} + +#[cargo_test] +fn differ_only_by_metadata_with_lockfile() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies] + baz = "=0.0.1" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + Package::new("baz", "0.0.1+a").publish(); + Package::new("baz", "0.0.1+b").publish(); + Package::new("baz", "0.0.1+c").publish(); + + p.cargo("update --package baz --precise 0.0.1+b") + .with_stderr( + "\ +[UPDATING] [..] index +[..] baz v0.0.1+c -> v0.0.1+b +", + ) + .run(); + + p.cargo("check") + .with_stderr( + "\ +[DOWNLOADING] crates ... +[DOWNLOADED] [..] v0.0.1+b (registry `dummy-registry`) +[CHECKING] baz v0.0.1+b +[CHECKING] foo v0.0.1 ([CWD]) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]s ", ) .run(); From 1db86b0443d97af19c1d9ef9c9f6bda1e2f5e128 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Thu, 19 Oct 2023 18:58:19 +0000 Subject: [PATCH 2/3] requirements from replacements do not come from the lock file I think the core of the problem is that https://github.com/rust-lang/cargo/commit/725420e8d788e003ee82c15f66d50272aab493d1 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. --- src/cargo/util/toml/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index ee4ce743e1e..42562584331 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1274,8 +1274,7 @@ impl TomlManifest { replacement.unused_keys(), &mut cx.warnings, ); - dep.set_version_req(OptVersionReq::exact(&version)) - .lock_version(&version); + dep.set_version_req(OptVersionReq::exact(&version)); replace.push((spec, dep)); } Ok(replace) From 98766fbff4e3cd8f49181f8ca94d463be7c52f8f Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Thu, 19 Oct 2023 18:44:42 +0000 Subject: [PATCH 3/3] If there's a version in the lock file only use that exact version --- src/cargo/sources/registry/index.rs | 12 +++---- src/cargo/util/semver_ext.rs | 56 +++++++++-------------------- 2 files changed, 21 insertions(+), 47 deletions(-) diff --git a/src/cargo/sources/registry/index.rs b/src/cargo/sources/registry/index.rs index fbd2ef3c259..b9dbc76dd68 100644 --- a/src/cargo/sources/registry/index.rs +++ b/src/cargo/sources/registry/index.rs @@ -439,11 +439,9 @@ 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> { - let req = OptVersionReq::exact(pkg.version()); + let req = OptVersionReq::lock_to_exact(pkg.version()); let summary = self.summaries(pkg.name(), &req, load)?; - let summary = ready!(summary) - .filter(|s| s.package_id().version() == pkg.version()) - .next(); + let summary = ready!(summary).next(); Poll::Ready(Ok(summary .ok_or_else(|| internal(format!("no hash listed for {}", pkg)))? .as_summary() @@ -697,10 +695,8 @@ impl<'cfg> RegistryIndex<'cfg> { pkg: PackageId, load: &mut dyn RegistryData, ) -> Poll> { - let req = OptVersionReq::exact(pkg.version()); - let found = ready!(self.summaries(pkg.name(), &req, load))? - .filter(|s| s.package_id().version() == pkg.version()) - .any(|s| s.is_yanked()); + let req = OptVersionReq::lock_to_exact(pkg.version()); + let found = ready!(self.summaries(pkg.name(), &req, load))?.any(|s| s.is_yanked()); Poll::Ready(Ok(found)) } } diff --git a/src/cargo/util/semver_ext.rs b/src/cargo/util/semver_ext.rs index 2992eedfd8e..faab9fbce17 100644 --- a/src/cargo/util/semver_ext.rs +++ b/src/cargo/util/semver_ext.rs @@ -1,6 +1,5 @@ use semver::{Comparator, Op, Version, VersionReq}; use serde_untagged::UntaggedEnumVisitor; -use std::cmp::Ordering; use std::fmt::{self, Display}; #[derive(PartialEq, Eq, Hash, Clone, Debug)] @@ -44,6 +43,13 @@ impl OptVersionReq { OptVersionReq::Req(VersionReq::exact(version)) } + // Since some registries have allowed crate versions to differ only by build metadata, + // A query using OptVersionReq::exact return nondeterministic results. + // So we `lock_to` the exact version were interested in. + pub fn lock_to_exact(version: &Version) -> Self { + OptVersionReq::Locked(version.clone(), VersionReq::exact(version)) + } + pub fn is_exact(&self) -> bool { match self { OptVersionReq::Any => false, @@ -84,7 +90,16 @@ impl OptVersionReq { match self { OptVersionReq::Any => true, OptVersionReq::Req(req) => req.matches(version), - OptVersionReq::Locked(v, _) => v.cmp_precedence(version) == Ordering::Equal, + OptVersionReq::Locked(v, _) => { + // Generally, cargo is of the opinion that semver metadata should be ignored. + // If your registry has two versions that only differing metadata you get the bugs you deserve. + // 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 we pick 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. + v == version + } } } } @@ -316,40 +331,3 @@ fn is_req(value: &str) -> bool { }; "<>=^~".contains(first) || value.contains('*') || value.contains(',') } - -#[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); - for v in vers { - let v = Version::parse(v).unwrap(); - assert_eq!(exact.matches(&v), locked.matches(&v)); - } - } - - test_versions( - "1.0.0", - &["1.0.0", "1.0.1", "0.9.9", "0.10.0", "0.1.0", "1.0.0-pre"], - ); - test_versions("0.9.0", &["0.9.0", "0.9.1", "1.9.0", "0.0.9", "0.9.0-pre"]); - test_versions("0.0.2", &["0.0.2", "0.0.1", "0.0.3", "0.0.2-pre"]); - test_versions( - "0.1.0-beta2.a", - &[ - "0.1.0-beta2.a", - "0.9.1", - "0.1.0", - "0.1.1-beta2.a", - "0.1.0-beta2", - ], - ); - test_versions("0.1.0+meta", &["0.1.0", "0.1.0+meta", "0.1.0+any"]); - } -}