From 15c757aca9c3f2381c72195488f641f9563492a2 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 14 Mar 2018 20:02:06 -0400 Subject: [PATCH 1/8] Clean the `backtrack_stack` so we don't backtrack to a place with cashed bad activations --- src/cargo/core/resolver/mod.rs | 54 ++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 377a20236db..dbb86a14a25 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -952,10 +952,7 @@ fn activate_deps_loop( .get(&dep) .and_then(|past_bad| { past_bad.iter().find(|conflicting| { - conflicting - .iter() - // note: a lot of redundant work in is_active for similar debs - .all(|(con, _)| cx.is_active(con)) + cx.is_conflicting(None, conflicting) }) }) .is_some(); @@ -1085,21 +1082,14 @@ fn activate_deps_loop( // but we may want to scrap it if it is not going to end well let mut has_past_conflicting_dep = just_here_for_the_error_messages; if !has_past_conflicting_dep { - if let Some(conflicting) = frame - .remaining_siblings - .clone() - .filter_map(|(_, (deb, _, _))| { - past_conflicting_activations.get(&deb).and_then(|past_bad| { - // for each dependency check all of its cashed conflicts - past_bad.iter().find(|conflicting| { - conflicting - .iter() - // note: a lot of redundant work in is_active for similar debs - .all(|(con, _)| cx.is_active(con)) - }) + if let Some(conflicting) = frame.remaining_siblings.clone().filter_map(|(_, (deb, _, _))| { + past_conflicting_activations.get(&deb).and_then(|past_bad| { + // for each dependency check all of its cashed conflicts + past_bad.iter().find(|conflicting| { + cx.is_conflicting(None, conflicting) }) }) - .next() + }).next() { // if any of them match than it will just backtrack to us // so let's save the effort. @@ -1123,6 +1113,9 @@ fn activate_deps_loop( conflicting_activations ); past.push(conflicting_activations.clone()); + // also clean the `backtrack_stack` so we don't + // backtrack to a place where we will try this again. + backtrack_stack.retain(|bframe| !bframe.context_backup.is_conflicting(Some(parent.package_id()), &conflicting_activations)); } } // if not has_another we we activate for the better error messages @@ -1191,15 +1184,10 @@ fn find_candidate<'a>( conflicting_activations: &HashMap, ) -> Option<(Candidate, bool, BacktrackFrame)> { while let Some(mut frame) = backtrack_stack.pop() { - let next = frame.remaining_candidates.next( - frame.context_backup.prev_active(&frame.dep), - &frame.context_backup.links, - ); - 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)) + let next= frame.remaining_candidates.next(frame.context_backup.prev_active(&frame.dep), &frame.context_backup.links,); + if frame.context_backup.is_conflicting(Some(parent.package_id()) + , conflicting_activations + ) { continue; } @@ -1643,6 +1631,20 @@ impl Context { .unwrap_or(false) } + /// checks whether all of `parent` and the keys of `conflicting activations` + /// are still active + fn is_conflicting( + &self, + parent: Option<&PackageId>, + conflicting_activations: &HashMap, + ) -> bool { + parent.map(|p| self.is_active(p)).unwrap_or(true) && + conflicting_activations + .keys() + // note: a lot of redundant work in is_active for similar debs + .all(|con| self.is_active(con)) + } + /// Return all dependencies and the features we want from them. fn resolve_features<'b>( &mut self, From 617856ec2b700133a7238102f8a7ec1ea3e106f0 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 15 Mar 2018 13:30:42 -0400 Subject: [PATCH 2/8] add a test --- tests/testsuite/resolve.rs | 39 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 5b3538c0224..90f69ec09b6 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -608,6 +608,45 @@ fn resolving_with_many_equivalent_backtracking() { assert!(res.is_err()); } +#[test] +fn resolving_with_deep_traps() { + let mut reglist = Vec::new(); + + const DEPTH: usize = 200; + const BRANCHING_FACTOR: usize = 100; + + // Each backtrack_trap depends on the next, and adds a backtrack frame. + // None of witch is going to help with `bad`. + for l in 0..DEPTH { + let name = format!("backtrack_trap{}", l); + let next = format!("backtrack_trap{}", l + 1); + for i in 1..BRANCHING_FACTOR { + let vsn = format!("1.0.{}", i); + reglist.push(pkg!((name.as_str(), vsn.as_str()) => [dep_req(next.as_str(), "*")])); + } + } + { + let name = format!("backtrack_trap{}", DEPTH); + for i in 1..BRANCHING_FACTOR { + let vsn = format!("1.0.{}", i); + reglist.push(pkg!((name.as_str(), vsn.as_str()))); + } + } + { + // slightly less constrained to make sure `cloaking` gets picked last. + for i in 1..(BRANCHING_FACTOR + 10) { + let vsn = format!("1.0.{}", i); + reglist.push(pkg!(("cloaking", vsn.as_str()) => [dep_req("bad", "1.0.1")])); + } + } + + let reg = registry(reglist.clone()); + + let res = resolve(&pkg_id("root"), vec![dep_req("backtrack_trap0", "*"), dep_req("cloaking", "*")], ®); + + assert!(res.is_err()); +} + #[test] fn resolving_with_constrained_sibling_backtrack_activation() { // It makes sense to resolve most-constrained deps first, but From 1291c50e86ed4b31db0c76de03a47a5d0074bbd7 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 15 Mar 2018 15:02:40 -0400 Subject: [PATCH 3/8] cargo +stable fmt --- src/cargo/core/resolver/mod.rs | 44 +++++++++++++++++++++------------- tests/testsuite/resolve.rs | 6 ++++- 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index dbb86a14a25..8f02830c83f 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -951,9 +951,9 @@ fn activate_deps_loop( && past_conflicting_activations .get(&dep) .and_then(|past_bad| { - past_bad.iter().find(|conflicting| { - cx.is_conflicting(None, conflicting) - }) + past_bad + .iter() + .find(|conflicting| cx.is_conflicting(None, conflicting)) }) .is_some(); @@ -1082,14 +1082,18 @@ fn activate_deps_loop( // but we may want to scrap it if it is not going to end well let mut has_past_conflicting_dep = just_here_for_the_error_messages; if !has_past_conflicting_dep { - if let Some(conflicting) = frame.remaining_siblings.clone().filter_map(|(_, (deb, _, _))| { - past_conflicting_activations.get(&deb).and_then(|past_bad| { - // for each dependency check all of its cashed conflicts - past_bad.iter().find(|conflicting| { - cx.is_conflicting(None, conflicting) + if let Some(conflicting) = frame + .remaining_siblings + .clone() + .filter_map(|(_, (deb, _, _))| { + past_conflicting_activations.get(&deb).and_then(|past_bad| { + // for each dependency check all of its cashed conflicts + past_bad + .iter() + .find(|conflicting| cx.is_conflicting(None, conflicting)) }) }) - }).next() + .next() { // if any of them match than it will just backtrack to us // so let's save the effort. @@ -1115,7 +1119,12 @@ fn activate_deps_loop( past.push(conflicting_activations.clone()); // also clean the `backtrack_stack` so we don't // backtrack to a place where we will try this again. - backtrack_stack.retain(|bframe| !bframe.context_backup.is_conflicting(Some(parent.package_id()), &conflicting_activations)); + backtrack_stack.retain(|bframe| { + !bframe.context_backup.is_conflicting( + Some(parent.package_id()), + &conflicting_activations, + ) + }); } } // if not has_another we we activate for the better error messages @@ -1184,10 +1193,13 @@ fn find_candidate<'a>( conflicting_activations: &HashMap, ) -> Option<(Candidate, bool, BacktrackFrame)> { while let Some(mut frame) = backtrack_stack.pop() { - let next= frame.remaining_candidates.next(frame.context_backup.prev_active(&frame.dep), &frame.context_backup.links,); - if frame.context_backup.is_conflicting(Some(parent.package_id()) - , conflicting_activations - ) + let next = frame.remaining_candidates.next( + frame.context_backup.prev_active(&frame.dep), + &frame.context_backup.links, + ); + if frame + .context_backup + .is_conflicting(Some(parent.package_id()), conflicting_activations) { continue; } @@ -1638,8 +1650,8 @@ impl Context { parent: Option<&PackageId>, conflicting_activations: &HashMap, ) -> bool { - parent.map(|p| self.is_active(p)).unwrap_or(true) && - conflicting_activations + parent.map(|p| self.is_active(p)).unwrap_or(true) + && conflicting_activations .keys() // note: a lot of redundant work in is_active for similar debs .all(|con| self.is_active(con)) diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index 90f69ec09b6..85297652303 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -642,7 +642,11 @@ fn resolving_with_deep_traps() { let reg = registry(reglist.clone()); - let res = resolve(&pkg_id("root"), vec![dep_req("backtrack_trap0", "*"), dep_req("cloaking", "*")], ®); + let res = resolve( + &pkg_id("root"), + vec![dep_req("backtrack_trap0", "*"), dep_req("cloaking", "*")], + ®, + ); assert!(res.is_err()); } From 68e9577001570617034c7c4731e24ce7ce926a51 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Thu, 15 Mar 2018 23:06:19 -0400 Subject: [PATCH 4/8] suggestions --- src/cargo/core/resolver/mod.rs | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 8f02830c83f..33ba172b23d 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -1078,22 +1078,16 @@ fn activate_deps_loop( match res { Ok(Some((mut frame, dur))) => { - // at this point we have technical already activated + // at this point we have technically already activated // but we may want to scrap it if it is not going to end well let mut has_past_conflicting_dep = just_here_for_the_error_messages; if !has_past_conflicting_dep { if let Some(conflicting) = frame .remaining_siblings .clone() - .filter_map(|(_, (deb, _, _))| { - past_conflicting_activations.get(&deb).and_then(|past_bad| { - // for each dependency check all of its cashed conflicts - past_bad - .iter() - .find(|conflicting| cx.is_conflicting(None, conflicting)) - }) - }) - .next() + .filter_map(|(_, (new_dep, _, _))| past_conflicting_activations.get(&new_dep)) + .flat_map(|x| x) + .find(|con| cx.is_conflicting(None, con)) { // if any of them match than it will just backtrack to us // so let's save the effort. @@ -1650,11 +1644,7 @@ impl Context { parent: Option<&PackageId>, conflicting_activations: &HashMap, ) -> bool { - parent.map(|p| self.is_active(p)).unwrap_or(true) - && conflicting_activations - .keys() - // note: a lot of redundant work in is_active for similar debs - .all(|con| self.is_active(con)) + conflicting_activations.keys().chain(parent).all(|id| self.is_active(id)) } /// Return all dependencies and the features we want from them. From 59249d1f4e09a93d78d13dadb83064690aeef391 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 16 Mar 2018 12:31:30 -0400 Subject: [PATCH 5/8] remove duplicated adding to the cache --- src/cargo/core/resolver/mod.rs | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 33ba172b23d..4174cc0b4bc 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -1096,30 +1096,16 @@ fn activate_deps_loop( } } if !has_another && has_past_conflicting_dep && !backtracked { - // we have not activated ANY candidates and - // we are out of choices so add it to the cache - // so our parent will know that we don't work - let past = past_conflicting_activations - .entry(dep.clone()) - .or_insert_with(Vec::new); - if !past.contains(&conflicting_activations) { - trace!( - "{}[{}]>{} adding a meta-skip {:?}", - parent.name(), - cur, - dep.name(), - conflicting_activations - ); - past.push(conflicting_activations.clone()); - // also clean the `backtrack_stack` so we don't - // backtrack to a place where we will try this again. - backtrack_stack.retain(|bframe| { - !bframe.context_backup.is_conflicting( - Some(parent.package_id()), - &conflicting_activations, - ) - }); - } + // we have not activated ANY candidates. + // So clean the `backtrack_stack` so we don't + // backtrack to a place where we will try this again. + backtrack_stack.retain(|bframe| { + // Maybe we could just not add them in the first place, but for now this works + !bframe.context_backup.is_conflicting( + Some(parent.package_id()), + &conflicting_activations, + ) + }); } // if not has_another we we activate for the better error messages frame.just_for_error_messages = has_past_conflicting_dep; From f5a0f28a7c4cf6fb678c715a92fa3f9036254f65 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 16 Mar 2018 15:10:58 -0400 Subject: [PATCH 6/8] Revert "Clean the `backtrack_stack` so we don't backtrack to a place with cashed bad activations" --- src/cargo/core/resolver/mod.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 4174cc0b4bc..fe9a8c85dad 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -1095,18 +1095,6 @@ fn activate_deps_loop( has_past_conflicting_dep = true; } } - if !has_another && has_past_conflicting_dep && !backtracked { - // we have not activated ANY candidates. - // So clean the `backtrack_stack` so we don't - // backtrack to a place where we will try this again. - backtrack_stack.retain(|bframe| { - // Maybe we could just not add them in the first place, but for now this works - !bframe.context_backup.is_conflicting( - Some(parent.package_id()), - &conflicting_activations, - ) - }); - } // if not has_another we we activate for the better error messages frame.just_for_error_messages = has_past_conflicting_dep; if !has_past_conflicting_dep From dd9ff1f23040e7994129e318ed02b194151f0265 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 16 Mar 2018 21:40:13 -0400 Subject: [PATCH 7/8] When test backtracking include conflicts in `remaining_candidates` --- src/cargo/core/resolver/mod.rs | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index fe9a8c85dad..13054aa4f5a 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -1085,7 +1085,9 @@ fn activate_deps_loop( if let Some(conflicting) = frame .remaining_siblings .clone() - .filter_map(|(_, (new_dep, _, _))| past_conflicting_activations.get(&new_dep)) + .filter_map(|(_, (new_dep, _, _))| { + past_conflicting_activations.get(&new_dep) + }) .flat_map(|x| x) .find(|con| cx.is_conflicting(None, con)) { @@ -1098,14 +1100,15 @@ fn activate_deps_loop( // if not has_another we we activate for the better error messages frame.just_for_error_messages = has_past_conflicting_dep; if !has_past_conflicting_dep - || (!has_another - && (just_here_for_the_error_messages - || find_candidate( - &mut backtrack_stack.clone(), - &parent, - &conflicting_activations, - ).is_none())) - { + || (!has_another && (just_here_for_the_error_messages || { + conflicting_activations + .extend(remaining_candidates.conflicting_prev_active.clone()); + find_candidate( + &mut backtrack_stack.clone(), + &parent, + &conflicting_activations, + ).is_none() + })) { remaining_deps.push(frame); } else { trace!( @@ -1618,7 +1621,10 @@ impl Context { parent: Option<&PackageId>, conflicting_activations: &HashMap, ) -> bool { - conflicting_activations.keys().chain(parent).all(|id| self.is_active(id)) + conflicting_activations + .keys() + .chain(parent) + .all(|id| self.is_active(id)) } /// Return all dependencies and the features we want from them. From c771a4c6b0e24523552fc0db3e8c584957c5ada4 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Sat, 17 Mar 2018 10:07:13 -0400 Subject: [PATCH 8/8] When activating for the better error messages don't waste time on the other backtrack frames --- src/cargo/core/resolver/mod.rs | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 13054aa4f5a..90042e0b22e 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -1097,18 +1097,34 @@ fn activate_deps_loop( has_past_conflicting_dep = true; } } - // if not has_another we we activate for the better error messages - frame.just_for_error_messages = has_past_conflicting_dep; - if !has_past_conflicting_dep - || (!has_another && (just_here_for_the_error_messages || { + let activate_for_error_message = has_past_conflicting_dep && !has_another && { + // has_past_conflicting_dep -> One of this candidate deps will fail. + // !has_another -> If the candidate is not accepted we will backtrack. + + // So the question is will the user see that we skipped this candidate? + just_here_for_the_error_messages || { + // make sure we know about all the possible conflicts conflicting_activations .extend(remaining_candidates.conflicting_prev_active.clone()); + // test if backtracking will get to the user find_candidate( &mut backtrack_stack.clone(), &parent, &conflicting_activations, ).is_none() - })) { + } + }; + if activate_for_error_message { + // We know one of this candidate deps will fail, + // which means we will fail, + // and that none of the backtrack frames will + // find a candidate that will help. + // So lets clean up the useless backtrack frames. + backtrack_stack.clear(); + } + // if not has_another we we activate for the better error messages + frame.just_for_error_messages = has_past_conflicting_dep; + if !has_past_conflicting_dep || activate_for_error_message { remaining_deps.push(frame); } else { trace!(