From 2fc644a7cd46ea8483b59fe00479846d888f6e29 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 11:03:07 -0500 Subject: [PATCH 1/7] test(spec): Make room for more tests --- src/cargo/core/package_id_spec.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/package_id_spec.rs b/src/cargo/core/package_id_spec.rs index 37a6f5e7b3c..313bcf6a4a5 100644 --- a/src/cargo/core/package_id_spec.rs +++ b/src/cargo/core/package_id_spec.rs @@ -444,11 +444,10 @@ mod tests { fn matching() { let url = Url::parse("https://example.com").unwrap(); let sid = SourceId::for_registry(&url).unwrap(); - let foo = PackageId::new("foo", "1.2.3", sid).unwrap(); - let bar = PackageId::new("bar", "1.2.3", sid).unwrap(); + let foo = PackageId::new("foo", "1.2.3", sid).unwrap(); assert!(PackageIdSpec::parse("foo").unwrap().matches(foo)); - assert!(!PackageIdSpec::parse("foo").unwrap().matches(bar)); + assert!(!PackageIdSpec::parse("bar").unwrap().matches(foo)); assert!(PackageIdSpec::parse("foo:1.2.3").unwrap().matches(foo)); assert!(!PackageIdSpec::parse("foo:1.2.2").unwrap().matches(foo)); assert!(PackageIdSpec::parse("foo@1.2.3").unwrap().matches(foo)); From 1bf12a567191f7849c7cd202af5b323abe32c33f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 11:08:22 -0500 Subject: [PATCH 2/7] test(spec): Cover meta/pre cases for partial versions We already tested with missing minor/patch --- src/cargo/core/package_id_spec.rs | 36 +++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/cargo/core/package_id_spec.rs b/src/cargo/core/package_id_spec.rs index 313bcf6a4a5..11cecb2d9f6 100644 --- a/src/cargo/core/package_id_spec.rs +++ b/src/cargo/core/package_id_spec.rs @@ -453,5 +453,41 @@ mod tests { assert!(PackageIdSpec::parse("foo@1.2.3").unwrap().matches(foo)); assert!(!PackageIdSpec::parse("foo@1.2.2").unwrap().matches(foo)); assert!(PackageIdSpec::parse("foo@1.2").unwrap().matches(foo)); + + let meta = PackageId::new("meta", "1.2.3+hello", sid).unwrap(); + assert!(PackageIdSpec::parse("meta").unwrap().matches(meta)); + assert!(PackageIdSpec::parse("meta@1").unwrap().matches(meta)); + assert!(PackageIdSpec::parse("meta@1.2").unwrap().matches(meta)); + assert!(PackageIdSpec::parse("meta@1.2.3").unwrap().matches(meta)); + assert!(!PackageIdSpec::parse("meta@1.2.3-alpha.0") + .unwrap() + .matches(meta)); + assert!(PackageIdSpec::parse("meta@1.2.3+hello") + .unwrap() + .matches(meta)); + assert!(PackageIdSpec::parse("meta@1.2.3+bye") + .unwrap() + .matches(meta)); + + let pre = PackageId::new("pre", "1.2.3-alpha.0", sid).unwrap(); + assert!(PackageIdSpec::parse("pre").unwrap().matches(pre)); + assert!(!PackageIdSpec::parse("pre@1").unwrap().matches(pre)); + assert!(!PackageIdSpec::parse("pre@1.2").unwrap().matches(pre)); + assert!(!PackageIdSpec::parse("pre@1.2.3").unwrap().matches(pre)); + assert!(PackageIdSpec::parse("pre@1.2.3-alpha.0") + .unwrap() + .matches(pre)); + assert!(!PackageIdSpec::parse("pre@1.2.3-alpha.1") + .unwrap() + .matches(pre)); + assert!(!PackageIdSpec::parse("pre@1.2.3-beta.0") + .unwrap() + .matches(pre)); + assert!(!PackageIdSpec::parse("pre@1.2.3+hello") + .unwrap() + .matches(pre)); + assert!(PackageIdSpec::parse("pre@1.2.3-alpha.0+hello") + .unwrap() + .matches(pre)); } } From ae068fbf4c73d29b54b9b82f8d369baba95f8045 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 10 Oct 2023 16:18:56 -0500 Subject: [PATCH 3/7] test(replace): Show behavior post-#12614 PR #12614 - Changed ignored replaces to not-ignored - Has bugs in matching replaces --- tests/testsuite/replace.rs | 144 +++++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+) diff --git a/tests/testsuite/replace.rs b/tests/testsuite/replace.rs index b583de7b7d5..15df0f9feeb 100644 --- a/tests/testsuite/replace.rs +++ b/tests/testsuite/replace.rs @@ -1298,3 +1298,147 @@ fn override_plus_dep() { .with_stderr_contains("error: cyclic package dependency: [..]") .run(); } + +#[cargo_test] +fn override_different_metadata() { + Package::new("bar", "0.1.0+a").publish(); + + let bar = git::repo(&paths::root().join("override")) + .file("Cargo.toml", &basic_manifest("bar", "0.1.0")) + .file("src/lib.rs", "pub fn bar() {}") + .build(); + + let p = project() + .file( + "Cargo.toml", + &format!( + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies] + bar = "0.1.0" + + [replace] + "bar:0.1.0" = {{ git = '{}' }} + "#, + bar.url() + ), + ) + .file( + "src/lib.rs", + "extern crate bar; pub fn foo() { bar::bar(); }", + ) + .build(); + + p.cargo("check") + .with_stderr( + "\ +[UPDATING] `dummy-registry` index +[UPDATING] git repository `[..]` +thread 'main' panicked at src/cargo/core/resolver/dep_cache.rs:179:13: +assertion `left == right` failed + left: Version { major: 0, minor: 1, patch: 0 } + right: Version { major: 0, minor: 1, patch: 0, build: BuildMetadata(\"a\") } +note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace +", + ) + .with_status(101) + .run(); +} + +#[cargo_test] +fn override_different_metadata_2() { + Package::new("bar", "0.1.0+a").publish(); + + let bar = git::repo(&paths::root().join("override")) + .file("Cargo.toml", &basic_manifest("bar", "0.1.0+a")) + .file("src/lib.rs", "pub fn bar() {}") + .build(); + + let p = project() + .file( + "Cargo.toml", + &format!( + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies] + bar = "0.1.0" + + [replace] + "bar:0.1.0+notTheBuild" = {{ git = '{}' }} + "#, + bar.url() + ), + ) + .file( + "src/lib.rs", + "extern crate bar; pub fn foo() { bar::bar(); }", + ) + .build(); + + p.cargo("check") + .with_stderr( + "\ +[UPDATING] `dummy-registry` index +[UPDATING] git repository `[..]` +[CHECKING] bar v0.1.0+a (file://[..]) +[CHECKING] foo v0.0.1 ([CWD]) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] +", + ) + .run(); +} + +#[cargo_test] +fn override_different_metadata_3() { + Package::new("bar", "0.1.0+a").publish(); + + let bar = git::repo(&paths::root().join("override")) + .file("Cargo.toml", &basic_manifest("bar", "0.1.0+a")) + .file("src/lib.rs", "pub fn bar() {}") + .build(); + + let p = project() + .file( + "Cargo.toml", + &format!( + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies] + bar = "0.1.0" + + [replace] + "bar:0.1.0" = {{ git = '{}' }} + "#, + bar.url() + ), + ) + .file( + "src/lib.rs", + "extern crate bar; pub fn foo() { bar::bar(); }", + ) + .build(); + + p.cargo("check") + .with_stderr( + "\ +[UPDATING] `dummy-registry` index +[UPDATING] git repository `[..]` +[CHECKING] bar v0.1.0+a (file://[..]) +[CHECKING] foo v0.0.1 ([CWD]) +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] +", + ) + .run(); +} From 0ec32743266716308a88256a5ed8ba4cdcd61000 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 10 Oct 2023 16:39:46 -0500 Subject: [PATCH 4/7] fix(spec): Ensure PackageIdSpec respects version 'build' field --- src/cargo/core/package_id_spec.rs | 13 ++++++------- src/cargo/util/semver_ext.rs | 24 ++++++++++++++---------- tests/testsuite/replace.rs | 20 +++++++++++++++----- 3 files changed, 35 insertions(+), 22 deletions(-) diff --git a/src/cargo/core/package_id_spec.rs b/src/cargo/core/package_id_spec.rs index 11cecb2d9f6..901c56ae19c 100644 --- a/src/cargo/core/package_id_spec.rs +++ b/src/cargo/core/package_id_spec.rs @@ -176,8 +176,7 @@ impl PackageIdSpec { } if let Some(ref v) = self.version { - let req = v.exact_req(); - if !req.matches(package_id.version()) { + if !v.matches(package_id.version()) { return false; } } @@ -465,15 +464,15 @@ mod tests { assert!(PackageIdSpec::parse("meta@1.2.3+hello") .unwrap() .matches(meta)); - assert!(PackageIdSpec::parse("meta@1.2.3+bye") + assert!(!PackageIdSpec::parse("meta@1.2.3+bye") .unwrap() .matches(meta)); let pre = PackageId::new("pre", "1.2.3-alpha.0", sid).unwrap(); assert!(PackageIdSpec::parse("pre").unwrap().matches(pre)); - assert!(!PackageIdSpec::parse("pre@1").unwrap().matches(pre)); - assert!(!PackageIdSpec::parse("pre@1.2").unwrap().matches(pre)); - assert!(!PackageIdSpec::parse("pre@1.2.3").unwrap().matches(pre)); + assert!(PackageIdSpec::parse("pre@1").unwrap().matches(pre)); + assert!(PackageIdSpec::parse("pre@1.2").unwrap().matches(pre)); + assert!(PackageIdSpec::parse("pre@1.2.3").unwrap().matches(pre)); assert!(PackageIdSpec::parse("pre@1.2.3-alpha.0") .unwrap() .matches(pre)); @@ -486,7 +485,7 @@ mod tests { assert!(!PackageIdSpec::parse("pre@1.2.3+hello") .unwrap() .matches(pre)); - assert!(PackageIdSpec::parse("pre@1.2.3-alpha.0+hello") + assert!(!PackageIdSpec::parse("pre@1.2.3-alpha.0+hello") .unwrap() .matches(pre)); } diff --git a/src/cargo/util/semver_ext.rs b/src/cargo/util/semver_ext.rs index 0be5bca68f2..ae103b4b67b 100644 --- a/src/cargo/util/semver_ext.rs +++ b/src/cargo/util/semver_ext.rs @@ -182,16 +182,20 @@ impl PartialVersion { } } - pub fn exact_req(&self) -> VersionReq { - VersionReq { - comparators: vec![Comparator { - op: semver::Op::Exact, - major: self.major, - minor: self.minor, - patch: self.patch, - pre: self.pre.as_ref().cloned().unwrap_or_default(), - }], - } + /// Check if this matches a version, including build metadata + /// + /// Build metadata does not affect version precedence but may be necessary for uniquely + /// identifying a package. + pub fn matches(&self, version: &Version) -> bool { + self.major == version.major + && self.minor.map(|f| f == version.minor).unwrap_or(true) + && self.patch.map(|f| f == version.patch).unwrap_or(true) + && self.pre.as_ref().map(|f| f == &version.pre).unwrap_or(true) + && self + .build + .as_ref() + .map(|f| f == &version.build) + .unwrap_or(true) } } diff --git a/tests/testsuite/replace.rs b/tests/testsuite/replace.rs index 15df0f9feeb..ee2314fd797 100644 --- a/tests/testsuite/replace.rs +++ b/tests/testsuite/replace.rs @@ -1350,7 +1350,7 @@ note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace } #[cargo_test] -fn override_different_metadata_2() { +fn override_respects_spec_metadata() { Package::new("bar", "0.1.0+a").publish(); let bar = git::repo(&paths::root().join("override")) @@ -1387,12 +1387,22 @@ fn override_different_metadata_2() { .with_stderr( "\ [UPDATING] `dummy-registry` index -[UPDATING] git repository `[..]` -[CHECKING] bar v0.1.0+a (file://[..]) -[CHECKING] foo v0.0.1 ([CWD]) -[FINISHED] dev [unoptimized + debuginfo] target(s) in [..] +[WARNING] package replacement is not used: https://github.com/rust-lang/crates.io-index#bar@0.1.0+notTheBuild +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.1.0+a (registry `dummy-registry`) +[CHECKING] bar v0.1.0+a +[CHECKING] foo v0.0.1 ([..]/foo) +[..] +[..] +[..] +[..] +[..] +[..] +[..] +error: could not compile `foo` (lib) due to previous error ", ) + .with_status(101) .run(); } From d318ed4c147cefcae2701c0582a291fe3b369fb2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 17 Oct 2023 10:09:12 -0500 Subject: [PATCH 5/7] fix(spec): Require opt-in for pre-release --- src/cargo/core/package_id_spec.rs | 6 +++--- src/cargo/util/semver_ext.rs | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/cargo/core/package_id_spec.rs b/src/cargo/core/package_id_spec.rs index 901c56ae19c..53d99b84ba7 100644 --- a/src/cargo/core/package_id_spec.rs +++ b/src/cargo/core/package_id_spec.rs @@ -470,9 +470,9 @@ mod tests { let pre = PackageId::new("pre", "1.2.3-alpha.0", sid).unwrap(); assert!(PackageIdSpec::parse("pre").unwrap().matches(pre)); - assert!(PackageIdSpec::parse("pre@1").unwrap().matches(pre)); - assert!(PackageIdSpec::parse("pre@1.2").unwrap().matches(pre)); - assert!(PackageIdSpec::parse("pre@1.2.3").unwrap().matches(pre)); + assert!(!PackageIdSpec::parse("pre@1").unwrap().matches(pre)); + assert!(!PackageIdSpec::parse("pre@1.2").unwrap().matches(pre)); + assert!(!PackageIdSpec::parse("pre@1.2.3").unwrap().matches(pre)); assert!(PackageIdSpec::parse("pre@1.2.3-alpha.0") .unwrap() .matches(pre)); diff --git a/src/cargo/util/semver_ext.rs b/src/cargo/util/semver_ext.rs index ae103b4b67b..2992eedfd8e 100644 --- a/src/cargo/util/semver_ext.rs +++ b/src/cargo/util/semver_ext.rs @@ -187,6 +187,11 @@ impl PartialVersion { /// Build metadata does not affect version precedence but may be necessary for uniquely /// identifying a package. pub fn matches(&self, version: &Version) -> bool { + if !version.pre.is_empty() && self.pre.is_none() { + // Pre-release versions must be explicitly opted into, if for no other reason than to + // give us room to figure out and define the semantics + return false; + } self.major == version.major && self.minor.map(|f| f == version.minor).unwrap_or(true) && self.patch.map(|f| f == version.patch).unwrap_or(true) From 7502318a854233bac625fb877f1c0242a803c74a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 10 Oct 2023 21:31:31 -0500 Subject: [PATCH 6/7] fix(replace): Error, rather than assert, on version mismatch --- src/cargo/core/resolver/dep_cache.rs | 19 ++++++++++++++----- tests/testsuite/replace.rs | 12 ++++++------ 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index 7b6e0661f17..9041c5b0f9e 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -173,11 +173,20 @@ impl<'a> RegistryQueryer<'a> { ))); } - // The dependency should be hard-coded to have the same name and an - // exact version requirement, so both of these assertions should - // never fail. - assert_eq!(s.version(), summary.version()); - assert_eq!(s.name(), summary.name()); + assert_eq!( + s.name(), + summary.name(), + "dependency should be hard coded to have the same name" + ); + if s.version() != summary.version() { + return Poll::Ready(Err(anyhow::anyhow!( + "replacement specification `{}` matched {} and tried to override it with {}\n\ + avoid matching unrelated packages by being more specific", + spec, + summary.version(), + s.version(), + ))); + } let replace = if s.source_id() == summary.source_id() { debug!("Preventing\n{:?}\nfrom replacing\n{:?}", summary, s); diff --git a/tests/testsuite/replace.rs b/tests/testsuite/replace.rs index ee2314fd797..a0bce4e2489 100644 --- a/tests/testsuite/replace.rs +++ b/tests/testsuite/replace.rs @@ -1300,7 +1300,7 @@ fn override_plus_dep() { } #[cargo_test] -fn override_different_metadata() { +fn override_generic_matching_other_versions() { Package::new("bar", "0.1.0+a").publish(); let bar = git::repo(&paths::root().join("override")) @@ -1338,11 +1338,11 @@ fn override_different_metadata() { "\ [UPDATING] `dummy-registry` index [UPDATING] git repository `[..]` -thread 'main' panicked at src/cargo/core/resolver/dep_cache.rs:179:13: -assertion `left == right` failed - left: Version { major: 0, minor: 1, patch: 0 } - right: Version { major: 0, minor: 1, patch: 0, build: BuildMetadata(\"a\") } -note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace +[ERROR] failed to get `bar` as a dependency of package `foo v0.0.1 ([..]/foo)` + +Caused by: + replacement specification `https://github.com/rust-lang/crates.io-index#bar@0.1.0` matched 0.1.0+a and tried to override it with 0.1.0 + avoid matching unrelated packages by being more specific ", ) .with_status(101) From 1e340663606ae0f16f44df353fa19e6474999dfd Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 10 Oct 2023 21:35:02 -0500 Subject: [PATCH 7/7] test(replace): Clarify name for remaining new test --- tests/testsuite/replace.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testsuite/replace.rs b/tests/testsuite/replace.rs index a0bce4e2489..b9de51d2fe6 100644 --- a/tests/testsuite/replace.rs +++ b/tests/testsuite/replace.rs @@ -1407,7 +1407,7 @@ error: could not compile `foo` (lib) due to previous error } #[cargo_test] -fn override_different_metadata_3() { +fn override_spec_metadata_is_optional() { Package::new("bar", "0.1.0+a").publish(); let bar = git::repo(&paths::root().join("override"))