diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 02a26a9f79c..7977e958aa2 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -745,9 +745,16 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, Ok(cx) } -// Searches up `backtrack_stack` until it finds a dependency with remaining -// candidates. Resets `cx` and `remaining_deps` to that level and returns the +// Looks through the states in `backtrack_stack` for dependencies with +// remaining candidates. For each one, also checks if rolling back +// could change the outcome of the failed resolution that caused backtracking +// in the first place - namely, if we've backtracked past the parent of the +// failed dep, or the previous number of activations of the failed dep has +// changed (possibly relaxing version constraints). If the outcome could differ, +// resets `cx` and `remaining_deps` to that level and returns the // next candidate. If all candidates have been exhausted, returns None. +// Read https://github.com/rust-lang/cargo/pull/4834#issuecomment-362871537 for +// a more detailed explanation of the logic here. fn find_candidate<'a>(backtrack_stack: &mut Vec>, cx: &mut Context<'a>, remaining_deps: &mut BinaryHeap, @@ -755,12 +762,21 @@ fn find_candidate<'a>(backtrack_stack: &mut Vec>, cur: &mut usize, dep: &mut Dependency, features: &mut Rc>) -> Option { + let num_dep_prev_active = cx.prev_active(dep).len(); while let Some(mut frame) = backtrack_stack.pop() { let (next, has_another) = { let prev_active = frame.context_backup.prev_active(&frame.dep); (frame.remaining_candidates.next(prev_active), frame.remaining_candidates.clone().next(prev_active).is_some()) }; + let cur_num_dep_prev_active = frame.context_backup.prev_active(dep).len(); + // Activations should monotonically decrease during backtracking + assert!(cur_num_dep_prev_active <= num_dep_prev_active); + let maychange = !frame.context_backup.is_active(parent) || + cur_num_dep_prev_active != num_dep_prev_active; + if !maychange { + continue + } if let Some(candidate) = next { *cur = frame.cur; if has_another { @@ -1178,6 +1194,14 @@ impl<'a> Context<'a> { .unwrap_or(&[]) } + fn is_active(&mut self, summary: &Summary) -> bool { + let id = summary.package_id(); + self.activations.get(id.name()) + .and_then(|v| v.get(id.source_id())) + .map(|v| v.iter().any(|s| s == summary)) + .unwrap_or(false) + } + /// Return all dependencies and the features we want from them. fn resolve_features<'b>(&mut self, s: &'b Summary, diff --git a/tests/resolve.rs b/tests/resolve.rs index 42a67dd37d0..6e43528fa3f 100644 --- a/tests/resolve.rs +++ b/tests/resolve.rs @@ -74,6 +74,13 @@ impl ToPkgId for (&'static str, &'static str) { } } +impl ToPkgId for (&'static str, String) { + fn to_pkgid(&self) -> PackageId { + let (name, ref vers) = *self; + PackageId::new(name, vers, ®istry_loc()).unwrap() + } +} + macro_rules! pkg { ($pkgid:expr => [$($deps:expr),+]) => ({ let d: Vec = vec![$($deps.to_dep()),+]; @@ -364,6 +371,137 @@ fn resolving_with_deep_backtracking() { ("baz", "1.0.1")]))); } +#[test] +fn resolving_with_constrained_sibling_backtrack_parent() { + // There is no point in considering all of the backtrack_trap{1,2} + // candidates since they can't change the result of failing to + // resolve 'constrained'. Cargo should (ideally) skip past them and resume + // resolution once the activation of the parent, 'bar', is rolled back. + // Note that the traps are slightly more constrained to make sure they + // get picked first. + let mut reglist = vec![ + pkg!(("foo", "1.0.0") => [dep_req("bar", "1.0"), + dep_req("constrained", "=1.0.0")]), + + pkg!(("bar", "1.0.0") => [dep_req("backtrack_trap1", "1.0.2"), + dep_req("backtrack_trap2", "1.0.2"), + dep_req("constrained", "1.0.0")]), + pkg!(("constrained", "1.0.0")), + pkg!(("backtrack_trap1", "1.0.0")), + pkg!(("backtrack_trap2", "1.0.0")), + ]; + // Bump this to make the test harder - it adds more versions of bar that will + // fail to resolve, and more versions of the traps to consider. + const NUM_BARS_AND_TRAPS: usize = 50; // minimum 2 + for i in 1..NUM_BARS_AND_TRAPS { + let vsn = format!("1.0.{}", i); + reglist.push(pkg!(("bar", vsn.clone()) => [dep_req("backtrack_trap1", "1.0.2"), + dep_req("backtrack_trap2", "1.0.2"), + dep_req("constrained", "1.0.1")])); + reglist.push(pkg!(("backtrack_trap1", vsn.clone()))); + reglist.push(pkg!(("backtrack_trap2", vsn.clone()))); + reglist.push(pkg!(("constrained", vsn.clone()))); + } + let reg = registry(reglist); + + let res = resolve(&pkg_id("root"), vec![ + dep_req("foo", "1"), + ], ®).unwrap(); + + assert_that(&res, contains(names(&[("root", "1.0.0"), + ("foo", "1.0.0"), + ("bar", "1.0.0"), + ("constrained", "1.0.0")]))); +} + +#[test] +fn resolving_with_constrained_sibling_backtrack_activation() { + // It makes sense to resolve most-constrained deps first, but + // with that logic the backtrack traps here come between the two + // attempted resolutions of 'constrained'. When backtracking, + // cargo should skip past them and resume resolution once the + // number of activations for 'constrained' changes. + let mut reglist = vec![ + pkg!(("foo", "1.0.0") => [dep_req("bar", "=1.0.0"), + dep_req("backtrack_trap1", "1.0"), + dep_req("backtrack_trap2", "1.0"), + dep_req("constrained", "<=1.0.60")]), + pkg!(("bar", "1.0.0") => [dep_req("constrained", ">=1.0.60")]), + ]; + // Bump these to make the test harder, but you'll also need to + // change the version constraints on `constrained` above. To correctly + // exercise Cargo, the relationship between the values is: + // NUM_CONSTRAINED - vsn < NUM_TRAPS < vsn + // to make sure the traps are resolved between `constrained`. + const NUM_TRAPS: usize = 45; // min 1 + const NUM_CONSTRAINED: usize = 100; // min 1 + for i in 0..NUM_TRAPS { + let vsn = format!("1.0.{}", i); + reglist.push(pkg!(("backtrack_trap1", vsn.clone()))); + reglist.push(pkg!(("backtrack_trap2", vsn.clone()))); + } + for i in 0..NUM_CONSTRAINED { + let vsn = format!("1.0.{}", i); + reglist.push(pkg!(("constrained", vsn.clone()))); + } + let reg = registry(reglist); + + let res = resolve(&pkg_id("root"), vec![ + dep_req("foo", "1"), + ], ®).unwrap(); + + assert_that(&res, contains(names(&[("root", "1.0.0"), + ("foo", "1.0.0"), + ("bar", "1.0.0"), + ("constrained", "1.0.60")]))); +} + +#[test] +fn resolving_with_constrained_sibling_transitive_dep_effects() { + // When backtracking due to a failed dependency, if Cargo is + // trying to be clever and skip irrelevant dependencies, care must + // be taken to not miss the transitive effects of alternatives. E.g. + // in the right-to-left resolution of the graph below, B may + // affect whether D is successfully resolved. + // + // A + // / | \ + // B C D + // | | + // C D + let reg = registry(vec![ + pkg!(("A", "1.0.0") => [dep_req("B", "1.0"), + dep_req("C", "1.0"), + dep_req("D", "1.0.100")]), + + pkg!(("B", "1.0.0") => [dep_req("C", ">=1.0.0")]), + pkg!(("B", "1.0.1") => [dep_req("C", ">=1.0.1")]), + + pkg!(("C", "1.0.0") => [dep_req("D", "1.0.0")]), + pkg!(("C", "1.0.1") => [dep_req("D", ">=1.0.1,<1.0.100")]), + pkg!(("C", "1.0.2") => [dep_req("D", ">=1.0.2,<1.0.100")]), + + pkg!(("D", "1.0.0")), + pkg!(("D", "1.0.1")), + pkg!(("D", "1.0.2")), + pkg!(("D", "1.0.100")), + pkg!(("D", "1.0.101")), + pkg!(("D", "1.0.102")), + pkg!(("D", "1.0.103")), + pkg!(("D", "1.0.104")), + pkg!(("D", "1.0.105")), + ]); + + let res = resolve(&pkg_id("root"), vec![ + dep_req("A", "1"), + ], ®).unwrap(); + + assert_that(&res, contains(names(&[("A", "1.0.0"), + ("B", "1.0.0"), + ("C", "1.0.0"), + ("D", "1.0.105")]))); +} + #[test] fn resolving_but_no_exists() { let reg = registry(vec![