From 771b2bca76ce895630c7a412cb051e5363e68ca3 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 22 May 2020 08:38:40 -0700 Subject: [PATCH 1/6] Repro --- crates/cargo-test-support/src/registry.rs | 8 ++++ tests/testsuite/build.rs | 55 +++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/crates/cargo-test-support/src/registry.rs b/crates/cargo-test-support/src/registry.rs index 6d3db585a..4bb6f2aa4 100644 --- a/crates/cargo-test-support/src/registry.rs +++ b/crates/cargo-test-support/src/registry.rs @@ -145,6 +145,7 @@ pub struct Package { alternative: bool, invalid_json: bool, proc_macro: bool, + links: Option, } #[derive(Clone)] @@ -245,6 +246,7 @@ impl Package { alternative: false, invalid_json: false, proc_macro: false, + links: None, } } @@ -368,6 +370,11 @@ impl Package { self } + pub fn links(&mut self, links: &str) -> &mut Package { + self.links = Some(links.to_string()); + self + } + /// Creates the package and place it in the registry. /// /// This does not actually use Cargo's publishing system, but instead @@ -420,6 +427,7 @@ impl Package { "cksum": cksum, "features": self.features, "yanked": self.yanked, + "links": self.links, }) .to_string(); diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 6605e1f53..8847c9137 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -4929,3 +4929,58 @@ hello stderr! let stdout = spawn(true); lines_match("hello_stdout!", &stdout); } + +use cargo_test_support::registry::Dependency; + +#[cargo_test] +fn eric() { + Package::new("blas-src", "0.6.1") + .add_dep(Dependency::new("openblas-src", "0.9").optional(true)) + .feature("openblas", &["openblas-src"]) + .publish(); + Package::new("openblas-src", "0.9.0") + .links("openblas") + .publish(); + Package::new("openblas-src", "0.6.1") + .links("openblas") + .publish(); + Package::new("ndarray-linalg", "0.12.0") + .add_dep(&Dependency::new("blas-src", "0.4")) // default-features=false + .add_dep(Dependency::new("ndarray", "0.13").enable_features(&["blas"])) + .publish(); + Package::new("blas-src", "0.4.0").publish(); + Package::new("blas-src", "0.2.1") + .feature("openblas", &["openblas-src"]) + .add_dep(Dependency::new("openblas-src", "0.6").optional(true)) + .publish(); + Package::new("ndarray", "0.13.1") + .add_dep(Dependency::new("blas-src", "0.2.0").optional(true)) + .feature("blas", &["blas-src"]) + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + blas-src = { version = "*", features = ["openblas"] } + openblas-src = { version = "*" } + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("generate-lockfile").run(); + cargo::util::paths::append(&p.root().join("Cargo.toml"), b"ndarray-linalg = \"0.12\"\n") + .unwrap(); + p.cargo("check") + .with_status(101) + .with_stderr_contains("[..]links to the native library `openblas`[..]") + .run(); + // This passes, what!? + p.cargo("check").run(); +} From 46b50bbb6779834afe5eef3db0649201b5d70aaa Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 22 May 2020 14:56:24 -0400 Subject: [PATCH 2/6] minimize test --- tests/testsuite/build.rs | 41 +++++++++++++++------------------------- 1 file changed, 15 insertions(+), 26 deletions(-) diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 8847c9137..4afea16b4 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -4933,29 +4933,19 @@ hello stderr! use cargo_test_support::registry::Dependency; #[cargo_test] -fn eric() { - Package::new("blas-src", "0.6.1") - .add_dep(Dependency::new("openblas-src", "0.9").optional(true)) - .feature("openblas", &["openblas-src"]) +fn reduced_reproduction_8249() { + Package::new("a-src", "0.1.0").links("a").publish(); + Package::new("a-src", "0.2.0").links("a").publish(); + + Package::new("b", "0.1.0") + .add_dep(Dependency::new("a-src", "0.1").optional(true)) .publish(); - Package::new("openblas-src", "0.9.0") - .links("openblas") + Package::new("b", "0.2.0") + .add_dep(Dependency::new("a-src", "0.2").optional(true)) .publish(); - Package::new("openblas-src", "0.6.1") - .links("openblas") - .publish(); - Package::new("ndarray-linalg", "0.12.0") - .add_dep(&Dependency::new("blas-src", "0.4")) // default-features=false - .add_dep(Dependency::new("ndarray", "0.13").enable_features(&["blas"])) - .publish(); - Package::new("blas-src", "0.4.0").publish(); - Package::new("blas-src", "0.2.1") - .feature("openblas", &["openblas-src"]) - .add_dep(Dependency::new("openblas-src", "0.6").optional(true)) - .publish(); - Package::new("ndarray", "0.13.1") - .add_dep(Dependency::new("blas-src", "0.2.0").optional(true)) - .feature("blas", &["blas-src"]) + + Package::new("c", "1.0.0") + .add_dep(&Dependency::new("b", "0.1.0")) .publish(); let p = project() @@ -4967,19 +4957,18 @@ fn eric() { version = "0.1.0" [dependencies] - blas-src = { version = "*", features = ["openblas"] } - openblas-src = { version = "*" } + b = { version = "*", features = ["a-src"] } + a-src = "*" "#, ) .file("src/lib.rs", "") .build(); p.cargo("generate-lockfile").run(); - cargo::util::paths::append(&p.root().join("Cargo.toml"), b"ndarray-linalg = \"0.12\"\n") - .unwrap(); + cargo::util::paths::append(&p.root().join("Cargo.toml"), b"c = \"*\"").unwrap(); p.cargo("check") .with_status(101) - .with_stderr_contains("[..]links to the native library `openblas`[..]") + .with_stderr_contains("[..]links to the native library `a`[..]") .run(); // This passes, what!? p.cargo("check").run(); From 053d8279ae69e3718d771a4dd39ebbf4ab30a18d Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 22 May 2020 21:37:02 -0400 Subject: [PATCH 3/6] don't need to clone this string --- src/cargo/core/registry.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index 2dccec54c..ebf57db4c 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -5,8 +5,8 @@ use log::{debug, trace}; use semver::VersionReq; use url::Url; -use crate::core::PackageSet; use crate::core::{Dependency, PackageId, Source, SourceId, SourceMap, Summary}; +use crate::core::{InternedString, PackageSet}; use crate::sources::config::SourceConfigMap; use crate::util::errors::{CargoResult, CargoResultExt}; use crate::util::{profile, CanonicalUrl, Config}; @@ -94,7 +94,7 @@ type LockedMap = HashMap< SourceId, HashMap< // This next level is keyed by the name of the package... - String, + InternedString, // ... and the value here is a list of tuples. The first element of each // tuple is a package which has the source/name used to get to this // point. The second element of each tuple is the list of locked @@ -207,9 +207,7 @@ impl<'cfg> PackageRegistry<'cfg> { .locked .entry(id.source_id()) .or_insert_with(HashMap::new); - let sub_vec = sub_map - .entry(id.name().to_string()) - .or_insert_with(Vec::new); + let sub_vec = sub_map.entry(id.name()).or_insert_with(Vec::new); sub_vec.push((id, deps)); } @@ -640,7 +638,7 @@ fn lock( ) -> Summary { let pair = locked .get(&summary.source_id()) - .and_then(|map| map.get(&*summary.name())) + .and_then(|map| map.get(&summary.name())) .and_then(|vec| vec.iter().find(|&&(id, _)| id == summary.package_id())); trace!("locking summary of {}", summary.package_id()); @@ -730,7 +728,7 @@ fn lock( // If anything does then we lock it to that and move on. let v = locked .get(&dep.source_id()) - .and_then(|map| map.get(&*dep.package_name())) + .and_then(|map| map.get(&dep.package_name())) .and_then(|vec| vec.iter().find(|&&(id, _)| dep.matches_id(id))); if let Some(&(id, _)) = v { trace!("\tsecond hit on {}", id); From 8418a3ae588cfd8b0fc9995c757e80270a007c83 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 22 May 2020 21:55:09 -0400 Subject: [PATCH 4/6] we dont need both maps --- src/cargo/core/registry.rs | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index ebf57db4c..beecf820c 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -91,16 +91,13 @@ pub struct PackageRegistry<'cfg> { type LockedMap = HashMap< // The first level of key-ing done in this hash map is the source that // dependencies come from, identified by a `SourceId`. - SourceId, - HashMap< - // This next level is keyed by the name of the package... - InternedString, - // ... and the value here is a list of tuples. The first element of each - // tuple is a package which has the source/name used to get to this - // point. The second element of each tuple is the list of locked - // dependencies that the first element has. - Vec<(PackageId, Vec)>, - >, + // The next level is keyed by the name of the package... + (SourceId, InternedString), + // ... and the value here is a list of tuples. The first element of each + // tuple is a package which has the source/name used to get to this + // point. The second element of each tuple is the list of locked + // dependencies that the first element has. + Vec<(PackageId, Vec)>, >; #[derive(PartialEq, Eq, Clone, Copy)] @@ -203,11 +200,10 @@ impl<'cfg> PackageRegistry<'cfg> { for dep in deps.iter() { trace!("\t-> {}", dep); } - let sub_map = self + let sub_vec = self .locked - .entry(id.source_id()) - .or_insert_with(HashMap::new); - let sub_vec = sub_map.entry(id.name()).or_insert_with(Vec::new); + .entry((id.source_id(), id.name())) + .or_insert_with(Vec::new); sub_vec.push((id, deps)); } @@ -637,8 +633,7 @@ fn lock( summary: Summary, ) -> Summary { let pair = locked - .get(&summary.source_id()) - .and_then(|map| map.get(&summary.name())) + .get(&(summary.source_id(), summary.name())) .and_then(|vec| vec.iter().find(|&&(id, _)| id == summary.package_id())); trace!("locking summary of {}", summary.package_id()); @@ -727,8 +722,7 @@ fn lock( // all known locked packages to see if they match this dependency. // If anything does then we lock it to that and move on. let v = locked - .get(&dep.source_id()) - .and_then(|map| map.get(&dep.package_name())) + .get(&(dep.source_id(), dep.package_name())) .and_then(|vec| vec.iter().find(|&&(id, _)| dep.matches_id(id))); if let Some(&(id, _)) = v { trace!("\tsecond hit on {}", id); From 61e592788c06b1213e1f71b41fc298b352101b29 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 23 May 2020 11:31:05 -0400 Subject: [PATCH 5/6] remove all residual state from previous lock files --- src/cargo/core/registry.rs | 6 ++++++ src/cargo/ops/resolve.rs | 1 + tests/testsuite/build.rs | 7 ++----- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs index beecf820c..2f63c2620 100644 --- a/src/cargo/core/registry.rs +++ b/src/cargo/core/registry.rs @@ -195,6 +195,12 @@ impl<'cfg> PackageRegistry<'cfg> { self.yanked_whitelist.extend(pkgs); } + /// remove all residual state from previous lock files. + pub fn clear_lock(&mut self) { + trace!("clear_lock"); + self.locked = HashMap::new(); + } + pub fn register_lock(&mut self, id: PackageId, deps: Vec) { trace!("register_lock: {}", id); for dep in deps.iter() { diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 37dc27fcb..e3341fe27 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -584,6 +584,7 @@ fn register_previous_locks( // the registry as a locked dependency. let keep = |id: &PackageId| keep(id) && !avoid_locking.contains(id); + registry.clear_lock(); for node in resolve.iter().filter(keep) { let deps = resolve .deps_not_replaced(node) diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 4afea16b4..b8d5707c1 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -4934,6 +4934,7 @@ use cargo_test_support::registry::Dependency; #[cargo_test] fn reduced_reproduction_8249() { + // https://github.com/rust-lang/cargo/issues/8249 Package::new("a-src", "0.1.0").links("a").publish(); Package::new("a-src", "0.2.0").links("a").publish(); @@ -4966,10 +4967,6 @@ fn reduced_reproduction_8249() { p.cargo("generate-lockfile").run(); cargo::util::paths::append(&p.root().join("Cargo.toml"), b"c = \"*\"").unwrap(); - p.cargo("check") - .with_status(101) - .with_stderr_contains("[..]links to the native library `a`[..]") - .run(); - // This passes, what!? + p.cargo("check").run(); p.cargo("check").run(); } From 8ce0d029718c8288c10eb4331f86b1bd48bda20d Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 28 May 2020 15:43:30 -0400 Subject: [PATCH 6/6] confirm that it is not a bug at resolver level --- crates/resolver-tests/tests/resolve.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index 4ba0f9ba4..337f422ab 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -1458,6 +1458,31 @@ fn conflict_store_more_then_one_match() { let _ = resolve_and_validated(vec![dep("nA")], ®, None); } +#[test] +fn bad_lockfile_from_8249() { + let input = vec![ + pkg!(("a-sys", "0.2.0")), + pkg!(("a-sys", "0.1.0")), + pkg!(("b", "0.1.0") => [ + dep_req("a-sys", "0.1"), // should be optional: true, but not deeded for now + ]), + pkg!(("c", "1.0.0") => [ + dep_req("b", "=0.1.0"), + ]), + pkg!("foo" => [ + dep_req("a-sys", "=0.2.0"), + { + let mut b = dep_req("b", "=0.1.0"); + b.set_features(vec!["a-sys"]); + b + }, + dep_req("c", "=1.0.0"), + ]), + ]; + let reg = registry(input); + let _ = resolve_and_validated(vec![dep("foo")], ®, None); +} + #[test] fn cyclic_good_error_message() { let input = vec![