From 1a1ba8054b3a1632e1f2234b18d455dba86432be Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Tue, 18 Jun 2019 17:30:07 -0400 Subject: [PATCH 1/6] `cur` is not as helpful as `age` when debugging --- src/cargo/core/resolver/mod.rs | 17 +++++++---------- src/cargo/core/resolver/types.rs | 10 ++++------ 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 4a21f42a58a..a19abefdc63 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -208,7 +208,7 @@ fn activate_deps_loop( while let Some((just_here_for_the_error_messages, frame)) = remaining_deps.pop_most_constrained() { - let (mut parent, (mut cur, (mut dep, candidates, mut features))) = frame; + let (mut parent, (mut dep, candidates, mut features)) = frame; // If we spend a lot of time here (we shouldn't in most cases) then give // a bit of a visual indicator as to what we're doing. @@ -217,7 +217,7 @@ fn activate_deps_loop( trace!( "{}[{}]>{} {} candidates", parent.name(), - cur, + cx.age(), dep.package_name(), candidates.len() ); @@ -263,7 +263,7 @@ fn activate_deps_loop( trace!( "{}[{}]>{} -- no candidates", parent.name(), - cur, + cx.age(), dep.package_name() ); @@ -308,7 +308,6 @@ fn activate_deps_loop( Some((candidate, has_another, frame)) => { // Reset all of our local variables used with the // contents of `frame` to complete our backtrack. - cur = frame.cur; cx = frame.context; remaining_deps = frame.remaining_deps; remaining_candidates = frame.remaining_candidates; @@ -353,7 +352,6 @@ fn activate_deps_loop( // if we can. let backtrack = if has_another { Some(BacktrackFrame { - cur, context: Context::clone(&cx), remaining_deps: remaining_deps.clone(), remaining_candidates: remaining_candidates.clone(), @@ -376,7 +374,7 @@ fn activate_deps_loop( trace!( "{}[{}]>{} trying {}", parent.name(), - cur, + cx.age(), dep.package_name(), candidate.version() ); @@ -409,7 +407,7 @@ fn activate_deps_loop( if let Some(conflicting) = frame .remaining_siblings .clone() - .filter_map(|(_, (ref new_dep, _, _))| { + .filter_map(|(ref new_dep, _, _)| { past_conflicting_activations.conflicting(&cx, new_dep) }) .next() @@ -526,7 +524,7 @@ fn activate_deps_loop( trace!( "{}[{}]>{} skipping {} ", parent.name(), - cur, + cx.age(), dep.package_name(), pid.version() ); @@ -700,7 +698,6 @@ fn activate( #[derive(Clone)] struct BacktrackFrame { - cur: usize, context: Context, remaining_deps: RemainingDeps, remaining_candidates: RemainingCandidates, @@ -759,7 +756,7 @@ impl RemainingCandidates { dep: &Dependency, parent: PackageId, ) -> Option<(Summary, bool)> { - 'main: for (_, b) in self.remaining.by_ref() { + 'main: for b in self.remaining.by_ref() { let b_id = b.package_id(); // The `links` key in the manifest dictates that there's only one // package in a dependency graph, globally, with that particular diff --git a/src/cargo/core/resolver/types.rs b/src/cargo/core/resolver/types.rs index ea8d3a22c43..0797403eeba 100644 --- a/src/cargo/core/resolver/types.rs +++ b/src/cargo/core/resolver/types.rs @@ -146,7 +146,7 @@ impl DepsFrame { pub fn flatten<'a>(&'a self) -> impl Iterator + 'a { self.remaining_siblings .clone() - .map(move |(_, (d, _, _))| (self.parent.package_id(), d)) + .map(move |(d, _, _)| (self.parent.package_id(), d)) } } @@ -202,7 +202,7 @@ impl RemainingDeps { self.data.insert((x, insertion_time)); self.time += 1; } - pub fn pop_most_constrained(&mut self) -> Option<(bool, (Summary, (usize, DepInfo)))> { + pub fn pop_most_constrained(&mut self) -> Option<(bool, (Summary, DepInfo))> { while let Some((mut deps_frame, insertion_time)) = self.data.remove_min() { let just_here_for_the_error_messages = deps_frame.just_for_error_messages; @@ -325,12 +325,10 @@ impl Iterator for RcVecIter where T: Clone, { - type Item = (usize, T); + type Item = T; fn next(&mut self) -> Option { - self.rest - .next() - .and_then(|i| self.vec.get(i).map(|val| (i, val.clone()))) + self.rest.next().and_then(|i| self.vec.get(i).cloned()) } fn size_hint(&self) -> (usize, Option) { From 5a30d17238ac45d9d4a92852796fb915034272a9 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 19 Jun 2019 11:34:58 -0400 Subject: [PATCH 2/6] `conflicting_activations` should always apply to `cx` This is a small assert, but it scuttled a pub/priv deps PR, so lets do it all the time. --- src/cargo/core/resolver/mod.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index a19abefdc63..e4e702e9d06 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -983,7 +983,12 @@ fn find_candidate( .any(|c| *c == ConflictReason::PublicDependency) { // we dont have abnormal situations. So we can ask `cx` for how far back we need to go. - cx.is_conflicting(Some(parent.package_id()), conflicting_activations) + let a = cx.is_conflicting(Some(parent.package_id()), conflicting_activations); + // If the `conflicting_activations` does not apply to `cx`, then something went very wrong + // in building it. But we will just fall back to laboriously trying all possibilities witch + // will give us the correct answer so only `assert` if there is a developer to debug it. + debug_assert!(a.is_some()); + a } else { None }; From f4bd3a4c6e7d95c07ec75d4ae9163666e58f5792 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 19 Jun 2019 11:59:33 -0400 Subject: [PATCH 3/6] dont have arg if it is all ways `pkg_id("root")` --- crates/resolver-tests/src/lib.rs | 21 +--- crates/resolver-tests/tests/resolve.rs | 162 ++++++++----------------- 2 files changed, 57 insertions(+), 126 deletions(-) diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index f13cfb72330..63750c28564 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -17,16 +17,11 @@ use proptest::sample::Index; use proptest::string::string_regex; use varisat::{self, ExtendFormula}; -pub fn resolve( - pkg: PackageId, - deps: Vec, - registry: &[Summary], -) -> CargoResult> { - resolve_with_config(pkg, deps, registry, None) +pub fn resolve(deps: Vec, registry: &[Summary]) -> CargoResult> { + resolve_with_config(deps, registry, None) } pub fn resolve_and_validated( - pkg: PackageId, deps: Vec, registry: &[Summary], sat_resolve: Option<&mut SatResolve>, @@ -36,11 +31,11 @@ pub fn resolve_and_validated( } else { SatResolve::new(registry).sat_resolve(&deps) }; - let resolve = resolve_with_config_raw(pkg, deps, registry, None); + let resolve = resolve_with_config_raw(deps, registry, None); assert_eq!(resolve.is_ok(), should_resolve); let resolve = resolve?; - let mut stack = vec![pkg]; + let mut stack = vec![pkg_id("root")]; let mut used = HashSet::new(); let mut links = HashSet::new(); while let Some(p) = stack.pop() { @@ -92,17 +87,15 @@ pub fn resolve_and_validated( } pub fn resolve_with_config( - pkg: PackageId, deps: Vec, registry: &[Summary], config: Option<&Config>, ) -> CargoResult> { - let resolve = resolve_with_config_raw(pkg, deps, registry, config)?; + let resolve = resolve_with_config_raw(deps, registry, config)?; Ok(resolve.sort()) } pub fn resolve_with_config_raw( - pkg: PackageId, deps: Vec, registry: &[Summary], config: Option<&Config>, @@ -158,7 +151,7 @@ pub fn resolve_with_config_raw( used: HashSet::new(), }; let summary = Summary::new( - pkg, + pkg_id("root"), deps, &BTreeMap::>::new(), None::, @@ -856,7 +849,6 @@ fn meta_test_deep_trees_from_strategy() { let reg = registry(input.clone()); for this in input.iter().rev().take(10) { let res = resolve( - pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], ®, ); @@ -898,7 +890,6 @@ fn meta_test_multiple_versions_strategy() { let reg = registry(input.clone()); for this in input.iter().rev().take(10) { let res = resolve( - pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], ®, ); diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index 6efe6d71114..cc2fe7a345b 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -6,8 +6,8 @@ use cargo::util::Config; use resolver_tests::{ assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, dep_req_kind, loc_names, names, - pkg, pkg_id, pkg_loc, registry, registry_strategy, remove_dep, resolve, - resolve_and_validated, resolve_with_config, PrettyPrintRegistry, SatResolve, ToDep, ToPkgId, + pkg, pkg_id, pkg_loc, registry, registry_strategy, remove_dep, resolve, resolve_and_validated, + resolve_with_config, PrettyPrintRegistry, SatResolve, ToDep, ToPkgId, }; use proptest::prelude::*; @@ -45,7 +45,6 @@ proptest! { // So we try some of the most complicated. for this in input.iter().rev().take(20) { let _ = resolve_and_validated( - pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], ®, Some(&mut sat_resolve), @@ -82,13 +81,11 @@ proptest! { // minimal-versions change what order the candidates // are tried but not the existence of a solution let res = resolve( - pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], ®, ); let mres = resolve_with_config( - pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], ®, Some(&config), @@ -128,13 +125,11 @@ proptest! { // So we try some of the most complicated. for this in input.iter().rev().take(10) { if resolve( - pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], ®, ).is_ok() { prop_assert!( resolve( - pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], &removed_reg, ).is_ok(), @@ -158,7 +153,6 @@ proptest! { // So we try some of the most complicated. for this in input.iter().rev().take(10) { let res = resolve( - pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], ®, ); @@ -184,7 +178,6 @@ proptest! { ); let res = resolve( - pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], &new_reg, ); @@ -218,7 +211,6 @@ proptest! { ); let res = resolve( - pkg_id("root"), vec![dep_req(&this.name(), &format!("={}", this.version()))], &new_reg, ); @@ -245,7 +237,7 @@ fn pub_fail() { pkg!(("kB", "0.0.3") => [dep_req("a", ">= 0.0.5"),dep("e"),]), ]; let reg = registry(input.clone()); - assert!(resolve_and_validated(pkg_id("root"), vec![dep("kB")], ®, None).is_err()); + assert!(resolve_and_validated(vec![dep("kB")], ®, None).is_err()); } #[test] @@ -257,7 +249,7 @@ fn basic_public_dependency() { pkg!("C" => [dep("A"), dep("B")]), ]); - let res = resolve_and_validated(pkg_id("root"), vec![dep("C")], ®, None).unwrap(); + let res = resolve_and_validated(vec![dep("C")], ®, None).unwrap(); assert_same( &res, &names(&[ @@ -293,7 +285,7 @@ fn public_dependency_filling_in() { pkg!("d" => [dep("c"), dep("a"), dep("b")]), ]); - let res = resolve_and_validated(pkg_id("root"), vec![dep("d")], ®, None).unwrap(); + let res = resolve_and_validated(vec![dep("d")], ®, None).unwrap(); assert_same( &res, &names(&[ @@ -328,7 +320,7 @@ fn public_dependency_filling_in_and_update() { pkg!("C" => [dep("A"),dep("B")]), pkg!("D" => [dep("B"),dep("C")]), ]); - let res = resolve_and_validated(pkg_id("root"), vec![dep("D")], ®, None).unwrap(); + let res = resolve_and_validated(vec![dep("D")], ®, None).unwrap(); assert_same( &res, &names(&[ @@ -355,7 +347,7 @@ fn public_dependency_skipping() { ]; let reg = registry(input); - resolve_and_validated(pkg_id("root"), vec![dep("c")], ®, None).unwrap(); + resolve_and_validated(vec![dep("c")], ®, None).unwrap(); } #[test] @@ -375,7 +367,7 @@ fn public_dependency_skipping_in_backtracking() { ]; let reg = registry(input); - resolve_and_validated(pkg_id("root"), vec![dep("C")], ®, None).unwrap(); + resolve_and_validated(vec![dep("C")], ®, None).unwrap(); } #[test] @@ -389,7 +381,7 @@ fn public_sat_topological_order() { ]; let reg = registry(input); - assert!(resolve_and_validated(pkg_id("root"), vec![dep("A")], ®, None).is_err()); + assert!(resolve_and_validated(vec![dep("A")], ®, None).is_err()); } #[test] @@ -403,7 +395,7 @@ fn public_sat_unused_makes_things_pub() { ]; let reg = registry(input); - resolve_and_validated(pkg_id("root"), vec![dep("c")], ®, None).unwrap(); + resolve_and_validated(vec![dep("c")], ®, None).unwrap(); } #[test] @@ -418,7 +410,7 @@ fn public_sat_unused_makes_things_pub_2() { ]; let reg = registry(input); - resolve_and_validated(pkg_id("root"), vec![dep("A")], ®, None).unwrap(); + resolve_and_validated(vec![dep("A")], ®, None).unwrap(); } #[test] @@ -430,7 +422,7 @@ fn test_dependency_with_empty_name() { #[test] fn test_resolving_empty_dependency_list() { - let res = resolve(pkg_id("root"), Vec::new(), ®istry(vec![])).unwrap(); + let res = resolve(Vec::new(), ®istry(vec![])).unwrap(); assert_eq!(res, names(&["root"])); } @@ -438,28 +430,28 @@ fn test_resolving_empty_dependency_list() { #[test] fn test_resolving_only_package() { let reg = registry(vec![pkg!("foo")]); - let res = resolve(pkg_id("root"), vec![dep("foo")], ®).unwrap(); + let res = resolve(vec![dep("foo")], ®).unwrap(); assert_same(&res, &names(&["root", "foo"])); } #[test] fn test_resolving_one_dep() { let reg = registry(vec![pkg!("foo"), pkg!("bar")]); - let res = resolve(pkg_id("root"), vec![dep("foo")], ®).unwrap(); + let res = resolve(vec![dep("foo")], ®).unwrap(); assert_same(&res, &names(&["root", "foo"])); } #[test] fn test_resolving_multiple_deps() { let reg = registry(vec![pkg!("foo"), pkg!("bar"), pkg!("baz")]); - let res = resolve(pkg_id("root"), vec![dep("foo"), dep("baz")], ®).unwrap(); + let res = resolve(vec![dep("foo"), dep("baz")], ®).unwrap(); assert_same(&res, &names(&["root", "foo", "baz"])); } #[test] fn test_resolving_transitive_deps() { let reg = registry(vec![pkg!("foo"), pkg!("bar" => ["foo"])]); - let res = resolve(pkg_id("root"), vec![dep("bar")], ®).unwrap(); + let res = resolve(vec![dep("bar")], ®).unwrap(); assert_same(&res, &names(&["root", "foo", "bar"])); } @@ -467,7 +459,7 @@ fn test_resolving_transitive_deps() { #[test] fn test_resolving_common_transitive_deps() { let reg = registry(vec![pkg!("foo" => ["bar"]), pkg!("bar")]); - let res = resolve(pkg_id("root"), vec![dep("foo"), dep("bar")], ®).unwrap(); + let res = resolve(vec![dep("foo"), dep("bar")], ®).unwrap(); assert_same(&res, &names(&["root", "foo", "bar"])); } @@ -481,7 +473,6 @@ fn test_resolving_with_same_name() { let reg = registry(list); let res = resolve( - pkg_id("root"), vec![ dep_loc("foo", "https://first.example.com"), dep_loc("bar", "https://second.example.com"), @@ -508,12 +499,7 @@ fn test_resolving_with_dev_deps() { pkg!("bat"), ]); - let res = resolve( - pkg_id("root"), - vec![dep("foo"), dep_kind("baz", Kind::Development)], - ®, - ) - .unwrap(); + let res = resolve(vec![dep("foo"), dep_kind("baz", Kind::Development)], ®).unwrap(); assert_same(&res, &names(&["root", "foo", "bar", "baz", "bat"])); } @@ -522,7 +508,7 @@ fn test_resolving_with_dev_deps() { fn resolving_with_many_versions() { let reg = registry(vec![pkg!(("foo", "1.0.1")), pkg!(("foo", "1.0.2"))]); - let res = resolve(pkg_id("root"), vec![dep("foo")], ®).unwrap(); + let res = resolve(vec![dep("foo")], ®).unwrap(); assert_same(&res, &names(&[("root", "1.0.0"), ("foo", "1.0.2")])); } @@ -531,7 +517,7 @@ fn resolving_with_many_versions() { fn resolving_with_specific_version() { let reg = registry(vec![pkg!(("foo", "1.0.1")), pkg!(("foo", "1.0.2"))]); - let res = resolve(pkg_id("root"), vec![dep_req("foo", "=1.0.1")], ®).unwrap(); + let res = resolve(vec![dep_req("foo", "=1.0.1")], ®).unwrap(); assert_same(&res, &names(&[("root", "1.0.0"), ("foo", "1.0.1")])); } @@ -546,12 +532,7 @@ fn test_resolving_maximum_version_with_transitive_deps() { pkg!("bar" => [dep_req("util", ">=1.0.1")]), ]); - let res = resolve( - pkg_id("root"), - vec![dep_req("foo", "1.0.0"), dep_req("bar", "1.0.0")], - ®, - ) - .unwrap(); + let res = resolve(vec![dep_req("foo", "1.0.0"), dep_req("bar", "1.0.0")], ®).unwrap(); assert_contains( &res, @@ -596,7 +577,6 @@ fn test_resolving_minimum_version_with_transitive_deps() { .unwrap(); let res = resolve_with_config( - pkg_id("root"), vec![dep_req("foo", "1.0.0"), dep_req("bar", "1.0.0")], ®, Some(&config), @@ -624,12 +604,7 @@ fn resolving_incompat_versions() { pkg!("bar" => [dep_req("foo", "=1.0.2")]), ]); - assert!(resolve( - pkg_id("root"), - vec![dep_req("foo", "=1.0.1"), dep("bar")], - ® - ) - .is_err()); + assert!(resolve(vec![dep_req("foo", "=1.0.1"), dep("bar")], ®).is_err()); } #[test] @@ -640,7 +615,7 @@ fn resolving_wrong_case_from_registry() { // This test documents the current behavior. let reg = registry(vec![pkg!(("foo", "1.0.0")), pkg!("bar" => ["Foo"])]); - assert!(resolve(pkg_id("root"), vec![dep("bar")], ®).is_err()); + assert!(resolve(vec![dep("bar")], ®).is_err()); } #[test] @@ -651,7 +626,7 @@ fn resolving_mis_hyphenated_from_registry() { // This test documents the current behavior. let reg = registry(vec![pkg!(("fo-o", "1.0.0")), pkg!("bar" => ["fo_o"])]); - assert!(resolve(pkg_id("root"), vec![dep("bar")], ®).is_err()); + assert!(resolve(vec![dep("bar")], ®).is_err()); } #[test] @@ -663,7 +638,7 @@ fn resolving_backtrack() { pkg!("baz"), ]); - let res = resolve(pkg_id("root"), vec![dep_req("foo", "^1")], ®).unwrap(); + let res = resolve(vec![dep_req("foo", "^1")], ®).unwrap(); assert_contains( &res, @@ -683,7 +658,7 @@ fn resolving_backtrack_features() { pkg!("bar"), ]); - let res = resolve(pkg_id("root"), vec![dep_req("foo", "^1")], ®).unwrap(); + let res = resolve(vec![dep_req("foo", "^1")], ®).unwrap(); assert_contains( &res, @@ -705,7 +680,7 @@ fn resolving_allows_multiple_compatible_versions() { pkg!("d4" => [dep_req("foo", "0.2")]), ]); - let res = resolve(pkg_id("root"), vec![dep("bar")], ®).unwrap(); + let res = resolve(vec![dep("bar")], ®).unwrap(); assert_same( &res, @@ -738,7 +713,7 @@ fn resolving_with_deep_backtracking() { pkg!(("dep_req", "2.0.0")), ]); - let res = resolve(pkg_id("root"), vec![dep_req("foo", "1")], ®).unwrap(); + let res = resolve(vec![dep_req("foo", "1")], ®).unwrap(); assert_same( &res, @@ -765,12 +740,7 @@ fn resolving_with_sys_crates() { pkg!(("r", "1.0.0") => [dep_req("l-sys", "0.9"), dep_req("l", "0.9")]), ]); - let res = resolve( - pkg_id("root"), - vec![dep_req("d", "1"), dep_req("r", "1")], - ®, - ) - .unwrap(); + let res = resolve(vec![dep_req("d", "1"), dep_req("r", "1")], ®).unwrap(); assert_same( &res, @@ -819,7 +789,7 @@ fn resolving_with_constrained_sibling_backtrack_parent() { } let reg = registry(reglist); - let res = resolve(pkg_id("root"), vec![dep_req("foo", "1")], ®).unwrap(); + let res = resolve(vec![dep_req("foo", "1")], ®).unwrap(); assert_contains( &res, @@ -854,7 +824,7 @@ fn resolving_with_many_equivalent_backtracking() { let reg = registry(reglist.clone()); - let res = resolve(pkg_id("root"), vec![dep("level0")], ®); + let res = resolve(vec![dep("level0")], ®); assert!(res.is_err()); @@ -864,7 +834,7 @@ fn resolving_with_many_equivalent_backtracking() { let reg = registry(reglist.clone()); - let res = resolve(pkg_id("root"), vec![dep("level0")], ®).unwrap(); + let res = resolve(vec![dep("level0")], ®).unwrap(); assert_contains(&res, &names(&[("root", "1.0.0"), ("level0", "1.0.0")])); @@ -877,12 +847,7 @@ fn resolving_with_many_equivalent_backtracking() { let reg = registry(reglist.clone()); - let res = resolve( - pkg_id("root"), - vec![dep("level0"), dep("constrained")], - ®, - ) - .unwrap(); + let res = resolve(vec![dep("level0"), dep("constrained")], ®).unwrap(); assert_contains( &res, @@ -895,12 +860,7 @@ fn resolving_with_many_equivalent_backtracking() { let reg = registry(reglist.clone()); - let res = resolve( - pkg_id("root"), - vec![dep_req("level0", "1.0.1"), dep("constrained")], - ®, - ) - .unwrap(); + let res = resolve(vec![dep_req("level0", "1.0.1"), dep("constrained")], ®).unwrap(); assert_contains( &res, @@ -914,7 +874,6 @@ fn resolving_with_many_equivalent_backtracking() { let reg = registry(reglist); let res = resolve( - pkg_id("root"), vec![dep_req("level0", "1.0.1"), dep_req("constrained", "1.1.0")], ®, ); @@ -956,11 +915,7 @@ fn resolving_with_deep_traps() { let reg = registry(reglist); - let res = resolve( - pkg_id("root"), - vec![dep("backtrack_trap0"), dep("cloaking")], - ®, - ); + let res = resolve(vec![dep("backtrack_trap0"), dep("cloaking")], ®); assert!(res.is_err()); } @@ -1009,7 +964,6 @@ fn resolving_with_constrained_cousins_backtrack() { // but `constrained= "2.0.1"` is already picked. // Only then to try and solve `constrained= "~1.0.0"` which is incompatible. let res = resolve( - pkg_id("root"), vec![ dep("backtrack_trap0"), dep_req("constrained", "2.0.1"), @@ -1038,20 +992,11 @@ fn resolving_with_constrained_cousins_backtrack() { let reg = registry(reglist); - let res = resolve( - pkg_id("root"), - vec![dep("level0"), dep_req("constrained", "2.0.1")], - ®, - ); + let res = resolve(vec![dep("level0"), dep_req("constrained", "2.0.1")], ®); assert!(res.is_err()); - let res = resolve( - pkg_id("root"), - vec![dep("level0"), dep_req("constrained", "2.0.0")], - ®, - ) - .unwrap(); + let res = resolve(vec![dep("level0"), dep_req("constrained", "2.0.0")], ®).unwrap(); assert_contains( &res, @@ -1091,7 +1036,7 @@ fn resolving_with_constrained_sibling_backtrack_activation() { } let reg = registry(reglist); - let res = resolve(pkg_id("root"), vec![dep_req("foo", "1")], ®).unwrap(); + let res = resolve(vec![dep_req("foo", "1")], ®).unwrap(); assert_contains( &res, @@ -1137,7 +1082,7 @@ fn resolving_with_constrained_sibling_transitive_dep_effects() { pkg!(("D", "1.0.105")), ]); - let res = resolve(pkg_id("root"), vec![dep_req("A", "1")], ®).unwrap(); + let res = resolve(vec![dep_req("A", "1")], ®).unwrap(); assert_same( &res, @@ -1183,7 +1128,7 @@ fn incomplete_information_skipping() { ]; let reg = registry(input.clone()); - let res = resolve(pkg_id("root"), vec![dep("g")], ®).unwrap(); + let res = resolve(vec![dep("g")], ®).unwrap(); let package_to_yank = "to_yank".to_pkgid(); // this package is not used in the resolution. assert!(!res.contains(&package_to_yank)); @@ -1197,7 +1142,7 @@ fn incomplete_information_skipping() { ); assert_eq!(input.len(), new_reg.len() + 1); // it should still build - assert!(resolve(pkg_id("root"), vec![dep("g")], &new_reg).is_ok()); + assert!(resolve(vec![dep("g")], &new_reg).is_ok()); } #[test] @@ -1252,7 +1197,7 @@ fn incomplete_information_skipping_2() { ]; let reg = registry(input.clone()); - let res = resolve(pkg_id("root"), vec![dep("i")], ®).unwrap(); + let res = resolve(vec![dep("i")], ®).unwrap(); let package_to_yank = ("to_yank", "8.8.1").to_pkgid(); // this package is not used in the resolution. assert!(!res.contains(&package_to_yank)); @@ -1266,7 +1211,7 @@ fn incomplete_information_skipping_2() { ); assert_eq!(input.len(), new_reg.len() + 1); // it should still build - assert!(resolve(pkg_id("root"), vec![dep("i")], &new_reg).is_ok()); + assert!(resolve(vec![dep("i")], &new_reg).is_ok()); } #[test] @@ -1302,7 +1247,7 @@ fn incomplete_information_skipping_3() { ]; let reg = registry(input.clone()); - let res = resolve(pkg_id("root"), vec![dep("b")], ®).unwrap(); + let res = resolve(vec![dep("b")], ®).unwrap(); let package_to_yank = ("to_yank", "3.0.3").to_pkgid(); // this package is not used in the resolution. assert!(!res.contains(&package_to_yank)); @@ -1316,14 +1261,14 @@ fn incomplete_information_skipping_3() { ); assert_eq!(input.len(), new_reg.len() + 1); // it should still build - assert!(resolve(pkg_id("root"), vec![dep("b")], &new_reg).is_ok()); + assert!(resolve(vec![dep("b")], &new_reg).is_ok()); } #[test] fn resolving_but_no_exists() { let reg = registry(vec![]); - let res = resolve(pkg_id("root"), vec![dep_req("foo", "1")], ®); + let res = resolve(vec![dep_req("foo", "1")], ®); assert!(res.is_err()); assert_eq!( @@ -1340,7 +1285,7 @@ fn resolving_but_no_exists() { fn resolving_cycle() { let reg = registry(vec![pkg!("foo" => ["foo"])]); - let _ = resolve(pkg_id("root"), vec![dep_req("foo", "1")], ®); + let _ = resolve(vec![dep_req("foo", "1")], ®); } #[test] @@ -1351,12 +1296,7 @@ fn hard_equality() { pkg!(("bar", "1.0.0") => [dep_req("foo", "1.0.0")]), ]); - let res = resolve( - pkg_id("root"), - vec![dep_req("bar", "1"), dep_req("foo", "=1.0.0")], - ®, - ) - .unwrap(); + let res = resolve(vec![dep_req("bar", "1"), dep_req("foo", "=1.0.0")], ®).unwrap(); assert_same( &res, @@ -1390,7 +1330,7 @@ fn large_conflict_cache() { } } let reg = registry(input); - let _ = resolve(pkg_id("root"), root_deps, ®); + let _ = resolve(root_deps, ®); } #[test] @@ -1430,7 +1370,7 @@ fn conflict_store_bug() { ]; let reg = registry(input); - let _ = resolve_and_validated(pkg_id("root"), vec![dep("j")], ®, None); + let _ = resolve_and_validated(vec![dep("j")], ®, None); } #[test] @@ -1458,5 +1398,5 @@ fn conflict_store_more_then_one_match() { ]), ]; let reg = registry(input); - let _ = resolve_and_validated(pkg_id("root"), vec![dep("nA")], ®, None); + let _ = resolve_and_validated(vec![dep("nA")], ®, None); } From f203deaa056facea535d92a856f56a328e865a17 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 19 Jun 2019 12:55:43 -0400 Subject: [PATCH 4/6] optimize `conflict_store` for looking up only older matches --- crates/resolver-tests/tests/resolve.rs | 19 +++++++++++++++++++ src/cargo/core/resolver/conflict_cache.rs | 17 ++++++++++++----- src/cargo/core/resolver/mod.rs | 6 +++--- 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index cc2fe7a345b..8be4f20aa99 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -1333,6 +1333,25 @@ fn large_conflict_cache() { let _ = resolve(root_deps, ®); } +#[test] +fn off_by_one_bug() { + let input = vec![ + pkg!(("A-sys", "0.0.1")), + pkg!(("A-sys", "0.0.4")), + pkg!(("A-sys", "0.0.6")), + pkg!(("A-sys", "0.0.7")), + pkg!(("NA", "0.0.0") => [dep_req("A-sys", "<= 0.0.5"),]), + pkg!(("NA", "0.0.1") => [dep_req("A-sys", ">= 0.0.6, <= 0.0.8"),]), + pkg!(("a", "0.0.1")), + pkg!(("a", "0.0.2")), + pkg!(("aa", "0.0.0") => [dep_req("A-sys", ">= 0.0.4, <= 0.0.6"),dep_req("NA", "<= 0.0.0"),]), + pkg!(("f", "0.0.3") => [dep("NA"),dep_req("a", "<= 0.0.2"),dep("aa"),]), + ]; + + let reg = registry(input); + let _ = resolve_and_validated(vec![dep("f")], ®, None); +} + #[test] fn conflict_store_bug() { let input = vec![ diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index 6b9d52b18d1..b88ad6825e7 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -25,6 +25,7 @@ impl ConflictStoreTrie { &self, is_active: &impl Fn(PackageId) -> Option, must_contain: Option, + mut max_age: usize, ) -> Option<(&ConflictMap, usize)> { match self { ConflictStoreTrie::Leaf(c) => { @@ -43,8 +44,12 @@ impl ConflictStoreTrie { { // If the key is active, then we need to check all of the corresponding subtrie. if let Some(age_this) = is_active(pid) { + if age_this >= max_age && must_contain != Some(pid) { + // not worth looking at, it is to old. + continue; + } if let Some((o, age_o)) = - store.find(is_active, must_contain.filter(|&f| f != pid)) + store.find(is_active, must_contain.filter(|&f| f != pid), max_age) { let age = if must_contain == Some(pid) { // all the results will include `must_contain` @@ -53,10 +58,11 @@ impl ConflictStoreTrie { } else { std::cmp::max(age_this, age_o) }; - let out_age = out.get_or_insert((o, age)).1; - if out_age > age { + if max_age > age { // we found one that can jump-back further so replace the out. out = Some((o, age)); + // and dont look at anything older + max_age = age } } } @@ -152,10 +158,11 @@ impl ConflictCache { dep: &Dependency, is_active: &impl Fn(PackageId) -> Option, must_contain: Option, + max_age: usize, ) -> Option<&ConflictMap> { self.con_from_dep .get(dep)? - .find(is_active, must_contain) + .find(is_active, must_contain, max_age) .map(|(c, _)| c) } /// Finds any known set of conflicts, if any, @@ -168,7 +175,7 @@ impl ConflictCache { dep: &Dependency, must_contain: Option, ) -> Option<&ConflictMap> { - let out = self.find(dep, &|id| cx.is_active(id), must_contain); + let out = self.find(dep, &|id| cx.is_active(id), must_contain, std::usize::MAX); if cfg!(debug_assertions) { if let Some(c) = &out { assert!(cx.is_conflicting(None, c).is_some()); diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index e4e702e9d06..26e3f48c224 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -902,12 +902,12 @@ fn generalize_conflicting( // we are imagining that we used other instead Some(backtrack_critical_age) } else { - cx.is_active(id).filter(|&age| - // we only care about things that are older then critical_age - age < backtrack_critical_age) + cx.is_active(id) } }, Some(other.package_id()), + // we only care about things that are newer then critical_age + backtrack_critical_age, ) .map(|con| (other.package_id(), con)) }) From 034c5908d823d5797f4eefc731b936fb1e3fd80d Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 19 Jun 2019 16:53:09 -0400 Subject: [PATCH 5/6] check that the SAT solver exempts the result from the resolver --- crates/resolver-tests/src/lib.rs | 179 ++++++++++++++++--------- crates/resolver-tests/tests/resolve.rs | 4 +- 2 files changed, 119 insertions(+), 64 deletions(-) diff --git a/crates/resolver-tests/src/lib.rs b/crates/resolver-tests/src/lib.rs index 63750c28564..73a26bae9cd 100644 --- a/crates/resolver-tests/src/lib.rs +++ b/crates/resolver-tests/src/lib.rs @@ -1,7 +1,10 @@ +use std::cell::RefCell; use std::cmp::PartialEq; use std::cmp::{max, min}; use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::fmt; +use std::fmt::Write; +use std::rc::Rc; use std::time::Instant; use cargo::core::dependency::Kind; @@ -24,66 +27,80 @@ pub fn resolve(deps: Vec, registry: &[Summary]) -> CargoResult, registry: &[Summary], - sat_resolve: Option<&mut SatResolve>, + sat_resolve: Option, ) -> CargoResult> { - let should_resolve = if let Some(sat) = sat_resolve { - sat.sat_resolve(&deps) - } else { - SatResolve::new(registry).sat_resolve(&deps) - }; - let resolve = resolve_with_config_raw(deps, registry, None); - assert_eq!(resolve.is_ok(), should_resolve); - - let resolve = resolve?; - let mut stack = vec![pkg_id("root")]; - let mut used = HashSet::new(); - let mut links = HashSet::new(); - while let Some(p) = stack.pop() { - assert!(resolve.contains(&p)); - if used.insert(p) { - // in the tests all `links` crates end in `-sys` - if p.name().ends_with("-sys") { - assert!(links.insert(p.name())); + let resolve = resolve_with_config_raw(deps.clone(), registry, None); + + match resolve { + Err(e) => { + let sat_resolve = sat_resolve.unwrap_or_else(|| SatResolve::new(registry)); + if sat_resolve.sat_resolve(&deps) { + panic!( + "the resolve err but the sat_resolve thinks this will work:\n{}", + sat_resolve.use_packages().unwrap() + ); } - stack.extend(resolve.deps(p).map(|(dp, deps)| { - for d in deps { - assert!(d.matches_id(dp)); - } - dp - })); + Err(e) } - } - let out = resolve.sort(); - assert_eq!(out.len(), used.len()); - - let mut pub_deps: HashMap> = HashMap::new(); - for &p in out.iter() { - // make the list of `p` public dependencies - let mut self_pub_dep = HashSet::new(); - self_pub_dep.insert(p); - for (dp, deps) in resolve.deps(p) { - if deps.iter().any(|d| d.is_public()) { - self_pub_dep.extend(pub_deps[&dp].iter().cloned()) + Ok(resolve) => { + let mut stack = vec![pkg_id("root")]; + let mut used = HashSet::new(); + let mut links = HashSet::new(); + while let Some(p) = stack.pop() { + assert!(resolve.contains(&p)); + if used.insert(p) { + // in the tests all `links` crates end in `-sys` + if p.name().ends_with("-sys") { + assert!(links.insert(p.name())); + } + stack.extend(resolve.deps(p).map(|(dp, deps)| { + for d in deps { + assert!(d.matches_id(dp)); + } + dp + })); + } } - } - pub_deps.insert(p, self_pub_dep); + let out = resolve.sort(); + assert_eq!(out.len(), used.len()); + + let mut pub_deps: HashMap> = HashMap::new(); + for &p in out.iter() { + // make the list of `p` public dependencies + let mut self_pub_dep = HashSet::new(); + self_pub_dep.insert(p); + for (dp, deps) in resolve.deps(p) { + if deps.iter().any(|d| d.is_public()) { + self_pub_dep.extend(pub_deps[&dp].iter().cloned()) + } + } + pub_deps.insert(p, self_pub_dep); - // check if `p` has a public dependencies conflicts - let seen_dep: BTreeSet<_> = resolve - .deps(p) - .flat_map(|(dp, _)| pub_deps[&dp].iter().cloned()) - .collect(); - let seen_dep: Vec<_> = seen_dep.iter().collect(); - for a in seen_dep.windows(2) { - if a[0].name() == a[1].name() { + // check if `p` has a public dependencies conflicts + let seen_dep: BTreeSet<_> = resolve + .deps(p) + .flat_map(|(dp, _)| pub_deps[&dp].iter().cloned()) + .collect(); + let seen_dep: Vec<_> = seen_dep.iter().collect(); + for a in seen_dep.windows(2) { + if a[0].name() == a[1].name() { + panic!( + "the package {:?} can publicly see {:?} and {:?}", + p, a[0], a[1] + ) + } + } + } + let sat_resolve = sat_resolve.unwrap_or_else(|| SatResolve::new(registry)); + if !sat_resolve.sat_is_valid_solution(&out) { panic!( - "the package {:?} can publicly see {:?} and {:?}", - p, a[0], a[1] - ) + "the sat_resolve err but the resolve thinks this will work:\n{:?}", + resolve + ); } + Ok(out) } } - Ok(out) } pub fn resolve_with_config( @@ -234,7 +251,9 @@ fn sat_at_most_one_by_key( /// /// The SAT library dose not optimize for the newer version, /// so the selected packages may not match the real resolver. -pub struct SatResolve { +#[derive(Clone)] +pub struct SatResolve(Rc>); +struct SatResolveInner { solver: varisat::Solver<'static>, var_for_is_packages_used: HashMap, by_name: HashMap<&'static str, Vec>, @@ -397,27 +416,27 @@ impl SatResolve { solver .solve() .expect("docs say it can't error in default config"); - - SatResolve { + SatResolve(Rc::new(RefCell::new(SatResolveInner { solver, var_for_is_packages_used, by_name, - } + }))) } - pub fn sat_resolve(&mut self, deps: &[Dependency]) -> bool { + pub fn sat_resolve(&self, deps: &[Dependency]) -> bool { + let mut s = self.0.borrow_mut(); let mut assumption = vec![]; let mut this_call = None; // the starting `deps` need to be satisfied for dep in deps.iter() { let empty_vec = vec![]; - let matches: Vec = self + let matches: Vec = s .by_name .get(dep.package_name().as_str()) .unwrap_or(&empty_vec) .iter() .filter(|&p| dep.matches_id(*p)) - .map(|p| self.var_for_is_packages_used[p].positive()) + .map(|p| s.var_for_is_packages_used[p].positive()) .collect(); if matches.is_empty() { return false; @@ -425,22 +444,58 @@ impl SatResolve { assumption.extend_from_slice(&matches) } else { if this_call.is_none() { - let new_var = self.solver.new_var(); + let new_var = s.solver.new_var(); this_call = Some(new_var); assumption.push(new_var.positive()); } let mut matches = matches; matches.push(this_call.unwrap().negative()); - self.solver.add_clause(&matches); + s.solver.add_clause(&matches); + } + } + + s.solver.assume(&assumption); + + s.solver + .solve() + .expect("docs say it can't error in default config") + } + pub fn sat_is_valid_solution(&self, pids: &[PackageId]) -> bool { + let mut s = self.0.borrow_mut(); + for p in pids { + if p.name().as_str() != "root" && !s.var_for_is_packages_used.contains_key(p) { + return false; } } + let assumption: Vec<_> = s + .var_for_is_packages_used + .iter() + .map(|(p, v)| v.lit(pids.contains(p))) + .collect(); - self.solver.assume(&assumption); + s.solver.assume(&assumption); - self.solver + s.solver .solve() .expect("docs say it can't error in default config") } + fn use_packages(&self) -> Option { + self.0.borrow().solver.model().map(|lits| { + let lits: HashSet<_> = lits + .iter() + .filter(|l| l.is_positive()) + .map(|l| l.var()) + .collect(); + let mut out = String::new(); + out.push_str("used:\n"); + for (p, v) in self.0.borrow().var_for_is_packages_used.iter() { + if lits.contains(v) { + writeln!(&mut out, " {}", p).unwrap(); + } + } + out + }) + } } pub trait ToDep { diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index 8be4f20aa99..5f2790c6afc 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -39,7 +39,7 @@ proptest! { PrettyPrintRegistry(input) in registry_strategy(50, 20, 60) ) { let reg = registry(input.clone()); - let mut sat_resolve = SatResolve::new(®); + let sat_resolve = SatResolve::new(®); // there is only a small chance that any one // crate will be interesting. // So we try some of the most complicated. @@ -47,7 +47,7 @@ proptest! { let _ = resolve_and_validated( vec![dep_req(&this.name(), &format!("={}", this.version()))], ®, - Some(&mut sat_resolve), + Some(sat_resolve.clone()), ); } } From cb95f5ef7c7203213a82e7b29b6612c1ae4ca440 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 20 Jun 2019 13:58:51 -0400 Subject: [PATCH 6/6] check is_public vs Kind more carefully --- src/cargo/core/dependency.rs | 4 ++++ src/cargo/core/resolver/resolve.rs | 16 ++++------------ 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/cargo/core/dependency.rs b/src/cargo/core/dependency.rs index f1a87e0a6f9..11f626e9ef9 100644 --- a/src/cargo/core/dependency.rs +++ b/src/cargo/core/dependency.rs @@ -328,6 +328,10 @@ impl Dependency { } pub fn set_kind(&mut self, kind: Kind) -> &mut Dependency { + if self.is_public() { + // Setting 'public' only makes sense for normal dependencies + assert_eq!(kind, Kind::Normal); + } Rc::make_mut(&mut self.inner).kind = kind; self } diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index 1684dc7fdff..82d4a61ec12 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -49,19 +49,11 @@ impl Resolve { .map(|p| { let public_deps = graph .edges(p) - .flat_map(|(dep_package, deps)| { - let id_opt: Option = deps - .iter() - .find(|d| d.kind() == Kind::Normal) - .and_then(|d| { - if d.is_public() { - Some(*dep_package) - } else { - None - } - }); - id_opt + .filter(|(_, deps)| { + deps.iter() + .any(|d| d.kind() == Kind::Normal && d.is_public()) }) + .map(|(dep_package, _)| *dep_package) .collect::>(); (*p, public_deps)