From 201fdba8f349ce8face9274050ef75594263a0fe Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sun, 11 Feb 2018 16:52:58 -0500 Subject: [PATCH 1/3] More directly track conflicts in backtracking, hopefully this will be easier to extend. --- src/cargo/core/resolver/mod.rs | 86 ++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 34 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 05fa4f5a0aa..3e983a23baa 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -536,15 +536,18 @@ struct BacktrackFrame<'a> { parent: Summary, dep: Dependency, features: Rc>, + conflicting_activations: HashSet, } #[derive(Clone)] struct RemainingCandidates { remaining: RcVecIter, + // note: change to RcList or something if clone is to expensive + conflicting_prev_active: HashSet, } impl RemainingCandidates { - fn next(&mut self, prev_active: &[Summary]) -> Option { + fn next(&mut self, prev_active: &[Summary]) -> Result> { // Filter the set of candidates based on the previously activated // versions for this dependency. We can actually use a version if it // precisely matches an activated version or if it is otherwise @@ -552,12 +555,20 @@ impl RemainingCandidates { // define "compatible" here in terms of the semver sense where if // the left-most nonzero digit is the same they're considered // compatible. - self.remaining.by_ref().map(|p| p.1).find(|b| { - prev_active.iter().any(|a| *a == b.summary) || - prev_active.iter().all(|a| { - !compatible(a.version(), b.summary.version()) - }) - }) + // + // When we are done we return the set of previously activated + // that conflicted with the ones we tried. If any of these change + // then we would have considered different candidates. + for (_, b) in self.remaining.by_ref() { + if let Some(a) = prev_active.iter().find(|a| compatible(a.version(), b.summary.version())) { + if *a != b.summary { + self.conflicting_prev_active.insert(a.package_id().clone()); + continue + } + } + return Ok(b); + } + Err(self.conflicting_prev_active.clone()) } } @@ -580,6 +591,7 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, // use (those with more candidates). let mut backtrack_stack = Vec::new(); let mut remaining_deps = BinaryHeap::new(); + let mut conflicting_activations; for &(ref summary, ref method) in summaries { debug!("initial activation: {}", summary.package_id()); let candidate = Candidate { summary: summary.clone(), replace: None }; @@ -650,9 +662,11 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, dep.name(), prev_active.len()); let mut candidates = RemainingCandidates { remaining: RcVecIter::new(Rc::clone(&candidates)), + conflicting_prev_active: HashSet::new(), }; + conflicting_activations = HashSet::new(); (candidates.next(prev_active), - candidates.clone().next(prev_active).is_some(), + candidates.clone().next(prev_active).is_ok(), candidates) }; @@ -669,7 +683,7 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, // turn. We could possibly fail to activate each candidate, so we try // each one in turn. let candidate = match next { - Some(candidate) => { + Ok(candidate) => { // We have a candidate. Add an entry to the `backtrack_stack` so // we can try the next one if this one fails. if has_another { @@ -681,11 +695,13 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, parent: Summary::clone(&parent), dep: Dependency::clone(&dep), features: Rc::clone(&features), + conflicting_activations: conflicting_activations.clone(), }); } candidate } - None => { + Err(mut conflicting) => { + conflicting_activations.extend(conflicting.drain()); // This dependency has no valid candidate. Backtrack until we // find a dependency that does have a candidate to try, and try // to activate that one. This resets the `remaining_deps` to @@ -698,7 +714,8 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, &mut parent, &mut cur, &mut dep, - &mut features) { + &mut features, + &mut conflicting_activations) { None => return Err(activation_error(&cx, registry, &parent, &dep, cx.prev_active(&dep), @@ -725,39 +742,39 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, Ok(cx) } -// 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. +/// 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 any of the packages flagged as giving us trouble in conflicting_activations. +/// Read https://github.com/rust-lang/cargo/pull/4834 +/// For several more detailed explanations of the logic here. +/// +/// 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. fn find_candidate<'a>(backtrack_stack: &mut Vec>, cx: &mut Context<'a>, remaining_deps: &mut BinaryHeap, parent: &mut Summary, cur: &mut usize, dep: &mut Dependency, - features: &mut Rc>) -> Option { - let num_dep_prev_active = cx.prev_active(dep).len(); + features: &mut Rc>, + conflicting_activations: &mut HashSet) -> Option { while let Some(mut frame) = backtrack_stack.pop() { + conflicting_activations.insert(parent.package_id().clone()); 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()) + frame.remaining_candidates.clone().next(prev_active).is_ok()) }; - 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 { + if conflicting_activations + .iter() + // note: a lot of redundant work in is_active for similar debs + .all(|con| frame.context_backup.is_active(con)) { continue } - if let Some(candidate) = next { + if let Ok(candidate) = next { *cur = frame.cur; if has_another { *cx = frame.context_backup.clone(); @@ -765,6 +782,7 @@ fn find_candidate<'a>(backtrack_stack: &mut Vec>, *parent = frame.parent.clone(); *dep = frame.dep.clone(); *features = Rc::clone(&frame.features); + *conflicting_activations = frame.conflicting_activations.clone(); backtrack_stack.push(frame); } else { *cx = frame.context_backup; @@ -772,6 +790,7 @@ fn find_candidate<'a>(backtrack_stack: &mut Vec>, *parent = frame.parent; *dep = frame.dep; *features = frame.features; + *conflicting_activations = frame.conflicting_activations } return Some(candidate) } @@ -1172,11 +1191,10 @@ impl<'a> Context<'a> { .unwrap_or(&[]) } - fn is_active(&mut self, summary: &Summary) -> bool { - let id = summary.package_id(); + fn is_active(&mut self, id: &PackageId) -> bool { self.activations.get(id.name()) .and_then(|v| v.get(id.source_id())) - .map(|v| v.iter().any(|s| s == summary)) + .map(|v| v.iter().any(|s| s.package_id() == id)) .unwrap_or(false) } From a0282214781d67826e98d11405a2f6879d1968cf Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Mon, 12 Feb 2018 20:54:52 -0500 Subject: [PATCH 2/3] Use the conflict tracking in the error messages to only show the versions that are in conflict --- src/cargo/core/resolver/mod.rs | 69 +++++++++++++++++++--------------- tests/build.rs | 45 +++++++++++++++++++--- 2 files changed, 78 insertions(+), 36 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 3e983a23baa..e354d6029c1 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -718,7 +718,7 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, &mut conflicting_activations) { None => return Err(activation_error(&cx, registry, &parent, &dep, - cx.prev_active(&dep), + conflicting_activations, &candidates, config)), Some(candidate) => candidate, } @@ -753,26 +753,31 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, /// 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. -fn find_candidate<'a>(backtrack_stack: &mut Vec>, - cx: &mut Context<'a>, - remaining_deps: &mut BinaryHeap, - parent: &mut Summary, - cur: &mut usize, - dep: &mut Dependency, - features: &mut Rc>, - conflicting_activations: &mut HashSet) -> Option { +fn find_candidate<'a>( + backtrack_stack: &mut Vec>, + cx: &mut Context<'a>, + remaining_deps: &mut BinaryHeap, + parent: &mut Summary, + cur: &mut usize, + dep: &mut Dependency, + features: &mut Rc>, + conflicting_activations: &mut HashSet, +) -> Option { while let Some(mut frame) = backtrack_stack.pop() { - conflicting_activations.insert(parent.package_id().clone()); 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_ok()) + ( + frame.remaining_candidates.next(prev_active), + frame.remaining_candidates.clone().next(prev_active).is_ok(), + ) }; - if conflicting_activations - .iter() - // note: a lot of redundant work in is_active for similar debs - .all(|con| frame.context_backup.is_active(con)) { - continue + if frame.context_backup.is_active(parent.package_id()) + && conflicting_activations + .iter() + // note: a lot of redundant work in is_active for similar debs + .all(|con| frame.context_backup.is_active(con)) + { + continue; } if let Ok(candidate) = next { *cur = frame.cur; @@ -792,7 +797,7 @@ fn find_candidate<'a>(backtrack_stack: &mut Vec>, *features = frame.features; *conflicting_activations = frame.conflicting_activations } - return Some(candidate) + return Some(candidate); } } None @@ -802,7 +807,7 @@ fn activation_error(cx: &Context, registry: &mut Registry, parent: &Summary, dep: &Dependency, - prev_active: &[Summary], + conflicting_activations: HashSet, candidates: &[Candidate], config: Option<&Config>) -> CargoError { let graph = cx.graph(); @@ -818,29 +823,31 @@ fn activation_error(cx: &Context, dep_path_desc }; if !candidates.is_empty() { - let mut msg = format!("failed to select a version for `{0}`\n\ + let mut msg = format!("failed to select a version for `{}`.\n\ all possible versions conflict with \ - previously selected versions of `{0}`\n", + previously selected packages.\n", dep.name()); msg.push_str("required by "); msg.push_str(&describe_path(parent.package_id())); - for v in prev_active.iter() { + let mut conflicting_activations: Vec<_> = conflicting_activations.iter().collect(); + conflicting_activations.sort_unstable(); + for v in conflicting_activations.iter().rev() { msg.push_str("\n previously selected "); - msg.push_str(&describe_path(v.package_id())); + msg.push_str(&describe_path(v)); } - msg.push_str(&format!("\n possible versions to select: {}", - candidates.iter() - .map(|v| v.summary.version()) - .map(|v| v.to_string()) - .collect::>() - .join(", "))); + msg.push_str("\n possible versions to select: "); + msg.push_str(&candidates.iter() + .map(|v| v.summary.version()) + .map(|v| v.to_string()) + .collect::>() + .join(", ")); return format_err!("{}", msg) } // Once we're all the way down here, we're definitely lost in the - // weeds! We didn't actually use any candidates above, so we need to + // weeds! We didn't actually find any candidates, so we need to // give an error message that nothing was found. // // Note that we re-query the registry with a new dependency that @@ -853,7 +860,7 @@ fn activation_error(cx: &Context, Ok(candidates) => candidates, Err(e) => return e, }; - candidates.sort_by(|a, b| { + candidates.sort_unstable_by(|a, b| { b.version().cmp(a.version()) }); diff --git a/tests/build.rs b/tests/build.rs index a39ea600505..7f3e887be49 100644 --- a/tests/build.rs +++ b/tests/build.rs @@ -1026,19 +1026,54 @@ fn incompatible_dependencies() { assert_that(p.cargo("build"), execs().with_status(101) .with_stderr_contains("\ -error: failed to select a version for `bad` -all possible versions conflict with previously selected versions of `bad` +error: failed to select a version for `bad`. +all possible versions conflict with previously selected packages. required by package `baz v0.1.0` ... which is depended on by `incompatible_dependencies v0.0.1 ([..])` - previously selected package `bad v0.1.0` - ... which is depended on by `foo v0.1.0` - ... which is depended on by `incompatible_dependencies v0.0.1 ([..])` previously selected package `bad v1.0.0` ... which is depended on by `bar v0.1.0` ... which is depended on by `incompatible_dependencies v0.0.1 ([..])` possible versions to select: 1.0.2, 1.0.1")); } +#[test] +fn incompatible_dependencies_with_multi_semver() { + Package::new("bad", "1.0.0").publish(); + Package::new("bad", "1.0.1").publish(); + Package::new("bad", "2.0.0").publish(); + Package::new("bad", "2.0.1").publish(); + Package::new("bar", "0.1.0").dep("bad", "=1.0.0").publish(); + Package::new("baz", "0.1.0").dep("bad", ">=2.0.1").publish(); + + let p = project("transitive_load_test") + .file("Cargo.toml", r#" + [project] + name = "incompatible_dependencies" + version = "0.0.1" + + [dependencies] + bar = "0.1.0" + baz = "0.1.0" + bad = ">=1.0.1, <=2.0.0" + "#) + .file("src/main.rs", "fn main(){}") + .build(); + + assert_that(p.cargo("build"), + execs().with_status(101) + .with_stderr_contains("\ +error: failed to select a version for `bad`. +all possible versions conflict with previously selected packages. +required by package `incompatible_dependencies v0.0.1 ([..])` + previously selected package `bad v2.0.1` + ... which is depended on by `baz v0.1.0` + ... which is depended on by `incompatible_dependencies v0.0.1 ([..])` + previously selected package `bad v1.0.0` + ... which is depended on by `bar v0.1.0` + ... which is depended on by `incompatible_dependencies v0.0.1 ([..])` + possible versions to select: 2.0.0, 1.0.1")); +} + #[test] fn compile_offline_while_transitive_dep_not_cached() { let bar = Package::new("bar", "1.0.0"); From 889909342cd9342babe2ca738d2d81c639a38050 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 14 Feb 2018 11:22:02 -0500 Subject: [PATCH 3/3] Don't store conflicting_activations for backtracking. If we need it later we can always add it back in. --- src/cargo/core/resolver/mod.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index e354d6029c1..633ae256033 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -536,7 +536,6 @@ struct BacktrackFrame<'a> { parent: Summary, dep: Dependency, features: Rc>, - conflicting_activations: HashSet, } #[derive(Clone)] @@ -591,7 +590,6 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, // use (those with more candidates). let mut backtrack_stack = Vec::new(); let mut remaining_deps = BinaryHeap::new(); - let mut conflicting_activations; for &(ref summary, ref method) in summaries { debug!("initial activation: {}", summary.package_id()); let candidate = Candidate { summary: summary.clone(), replace: None }; @@ -664,7 +662,6 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, remaining: RcVecIter::new(Rc::clone(&candidates)), conflicting_prev_active: HashSet::new(), }; - conflicting_activations = HashSet::new(); (candidates.next(prev_active), candidates.clone().next(prev_active).is_ok(), candidates) @@ -695,13 +692,11 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, parent: Summary::clone(&parent), dep: Dependency::clone(&dep), features: Rc::clone(&features), - conflicting_activations: conflicting_activations.clone(), }); } candidate } Err(mut conflicting) => { - conflicting_activations.extend(conflicting.drain()); // This dependency has no valid candidate. Backtrack until we // find a dependency that does have a candidate to try, and try // to activate that one. This resets the `remaining_deps` to @@ -715,10 +710,10 @@ fn activate_deps_loop<'a>(mut cx: Context<'a>, &mut cur, &mut dep, &mut features, - &mut conflicting_activations) { + &mut conflicting) { None => return Err(activation_error(&cx, registry, &parent, &dep, - conflicting_activations, + conflicting, &candidates, config)), Some(candidate) => candidate, } @@ -787,7 +782,6 @@ fn find_candidate<'a>( *parent = frame.parent.clone(); *dep = frame.dep.clone(); *features = Rc::clone(&frame.features); - *conflicting_activations = frame.conflicting_activations.clone(); backtrack_stack.push(frame); } else { *cx = frame.context_backup; @@ -795,7 +789,6 @@ fn find_candidate<'a>( *parent = frame.parent; *dep = frame.dep; *features = frame.features; - *conflicting_activations = frame.conflicting_activations } return Some(candidate); }