diff --git a/Cargo.toml b/Cargo.toml index 59b7427b8b2..394e8070d6c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,7 @@ crates-io = { path = "src/crates-io", version = "0.18" } crossbeam-utils = "0.5" crypto-hash = "0.3.1" curl = "0.4.13" -env_logger = "0.5.4" +env_logger = "0.5.11" failure = "0.1.2" filetime = "0.2" flate2 = "1.0" diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index 0f592cc3e23..b35813bc35a 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -1,8 +1,8 @@ use std::collections::{HashMap, HashSet}; -use core::{Dependency, PackageId}; -use core::resolver::Context; use super::types::ConflictReason; +use core::resolver::Context; +use core::{Dependency, PackageId}; pub(super) struct ConflictCache { // `con_from_dep` is a cache of the reasons for each time we @@ -77,11 +77,17 @@ impl ConflictCache { /// `dep` is known to be unresolvable if /// all the `PackageId` entries are activated pub fn insert(&mut self, dep: &Dependency, con: &HashMap) { - let past = self.con_from_dep + let past = self + .con_from_dep .entry(dep.clone()) .or_insert_with(Vec::new); if !past.contains(con) { - trace!("{} adding a skip {:?}", dep.package_name(), con); + trace!( + "{} = \"{}\" adding a skip {:?}", + dep.package_name(), + dep.version_req(), + con + ); past.push(con.clone()); for c in con.keys() { self.dep_from_pid diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 3957976d3cb..328c34dab56 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -305,7 +305,12 @@ fn activate_deps_loop( // It's our job here to backtrack, if possible, and find a // different candidate to activate. If we can't find any // candidates whatsoever then it's time to bail entirely. - trace!("{}[{}]>{} -- no candidates", parent.name(), cur, dep.package_name()); + trace!( + "{}[{}]>{} -- no candidates", + parent.name(), + cur, + dep.package_name() + ); // Use our list of `conflicting_activations` to add to our // global list of past conflicting activations, effectively @@ -325,7 +330,12 @@ fn activate_deps_loop( past_conflicting_activations.insert(&dep, &conflicting_activations); } - match find_candidate(&mut backtrack_stack, &parent, &conflicting_activations) { + match find_candidate( + &mut backtrack_stack, + &parent, + backtracked, + &conflicting_activations, + ) { Some((candidate, has_another, frame)) => { // Reset all of our local variables used with the // contents of `frame` to complete our backtrack. @@ -432,8 +442,7 @@ fn activate_deps_loop( .clone() .filter_map(|(_, (ref new_dep, _, _))| { past_conflicting_activations.conflicting(&cx, new_dep) - }) - .next() + }).next() { // If one of our deps is known unresolvable // then we will not succeed. @@ -467,18 +476,14 @@ fn activate_deps_loop( .iter() .flat_map(|other| other.flatten()) // for deps related to us - .filter(|&(_, ref other_dep)| - known_related_bad_deps.contains(other_dep)) - .filter_map(|(other_parent, other_dep)| { + .filter(|&(_, ref other_dep)| { + known_related_bad_deps.contains(other_dep) + }).filter_map(|(other_parent, other_dep)| { past_conflicting_activations - .find_conflicting( - &cx, - &other_dep, - |con| con.contains_key(&pid) - ) - .map(|con| (other_parent, con)) - }) - .next() + .find_conflicting(&cx, &other_dep, |con| { + con.contains_key(&pid) + }).map(|con| (other_parent, con)) + }).next() { let rel = conflict.get(&pid).unwrap().clone(); @@ -518,6 +523,7 @@ fn activate_deps_loop( find_candidate( &mut backtrack_stack.clone(), &parent, + backtracked, &conflicting_activations, ).is_none() } @@ -809,6 +815,7 @@ fn compatible(a: &semver::Version, b: &semver::Version) -> bool { fn find_candidate( backtrack_stack: &mut Vec, parent: &Summary, + backtracked: bool, conflicting_activations: &HashMap, ) -> Option<(Candidate, bool, BacktrackFrame)> { while let Some(mut frame) = backtrack_stack.pop() { @@ -830,11 +837,20 @@ fn find_candidate( // active in this back up we know that we're guaranteed to not actually // make any progress. As a result if we hit this condition we can // completely skip this backtrack frame and move on to the next. - if frame - .context_backup - .is_conflicting(Some(parent.package_id()), conflicting_activations) - { - continue; + if !backtracked { + if frame + .context_backup + .is_conflicting(Some(parent.package_id()), conflicting_activations) + { + trace!( + "{} = \"{}\" skip as not solving {}: {:?}", + frame.dep.package_name(), + frame.dep.version_req(), + parent.package_id(), + conflicting_activations + ); + continue; + } } return Some((candidate, has_another, frame)); diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index b71f9c07571..bc456ec20d4 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -104,7 +104,7 @@ impl<'a> ToPkgId for (&'a str, String) { } macro_rules! pkg { - ($pkgid:expr => [$($deps:expr),+]) => ({ + ($pkgid:expr => [$($deps:expr),+ $(,)* ]) => ({ let d: Vec = vec![$($deps.to_dep()),+]; let pkgid = $pkgid.to_pkgid(); let link = if pkgid.name().ends_with("-sys") {Some(pkgid.name().as_str())} else {None}; @@ -963,6 +963,174 @@ fn resolving_with_constrained_sibling_transitive_dep_effects() { ); } +#[test] +fn incomplete_information_skiping() { + // 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. + // Fuzzing discovered that for some reason cargo was skiping based + // on incomplete information in the following case: + // minimized bug found in: + // https://github.com/rust-lang/cargo/commit/003c29b0c71e5ea28fbe8e72c148c755c9f3f8d9 + let input = vec![ + pkg!(("a", "1.0.0")), + pkg!(("a", "1.1.0")), + pkg!("b" => [dep("a")]), + pkg!(("c", "1.0.0")), + pkg!(("c", "1.1.0")), + pkg!("d" => [dep_req("c", "=1.0")]), + pkg!(("e", "1.0.0")), + pkg!(("e", "1.1.0") => [dep_req("c", "1.1")]), + pkg!("to_yank"), + pkg!(("f", "1.0.0") => [ + dep("to_yank"), + dep("d"), + ]), + pkg!(("f", "1.1.0") => [dep("d")]), + pkg!("g" => [ + dep("b"), + dep("e"), + dep("f"), + ]), + ]; + let reg = registry(input.clone()); + + let res = resolve(&pkg_id("root"), 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)); + // so when we yank it + let new_reg = registry( + input + .iter() + .cloned() + .filter(|x| &package_to_yank != x.package_id()) + .collect(), + ); + assert_eq!(input.len(), new_reg.len() + 1); + // it should still build + assert!(resolve(&pkg_id("root"), vec![dep("g")], &new_reg).is_ok()); +} + +#[test] +fn incomplete_information_skiping_2() { + // 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. + // Fuzzing discovered that for some reason cargo was skiping based + // on incomplete information in the following case: + // https://github.com/rust-lang/cargo/commit/003c29b0c71e5ea28fbe8e72c148c755c9f3f8d9 + let input = vec![ + pkg!(("b", "3.8.10")), + pkg!(("b", "8.7.4")), + pkg!(("b", "9.4.6")), + pkg!(("c", "1.8.8")), + pkg!(("c", "10.2.5")), + pkg!(("d", "4.1.2") => [ + dep_req("bad", "=6.10.9"), + ]), + pkg!(("d", "5.5.6")), + pkg!(("d", "5.6.10")), + pkg!(("to_yank", "8.0.1")), + pkg!(("to_yank", "8.8.1")), + pkg!(("e", "4.7.8") => [ + dep_req("d", ">=5.5.6, <=5.6.10"), + dep_req("to_yank", "=8.0.1"), + ]), + pkg!(("e", "7.4.9") => [ + dep_req("bad", "=4.7.5"), + ]), + pkg!("f" => [ + dep_req("d", ">=4.1.2, <=5.5.6"), + ]), + pkg!("g" => [ + dep("bad"), + ]), + pkg!(("h", "3.8.3") => [ + dep_req("g", "*"), + ]), + pkg!(("h", "6.8.3") => [ + dep("f"), + ]), + pkg!(("h", "8.1.9") => [ + dep_req("to_yank", "=8.8.1"), + ]), + pkg!("i" => [ + dep_req("b", "*"), + dep_req("c", "*"), + dep_req("e", "*"), + dep_req("h", "*"), + ]), + ]; + let reg = registry(input.clone()); + + let res = resolve(&pkg_id("root"), 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)); + // so when we yank it + let new_reg = registry( + input + .iter() + .cloned() + .filter(|x| &package_to_yank != x.package_id()) + .collect(), + ); + assert_eq!(input.len(), new_reg.len() + 1); + // it should still build + assert!(resolve(&pkg_id("root"), vec![dep("i")], &new_reg).is_ok()); +} + +#[test] +fn incomplete_information_skiping_3() { + // 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. + // Fuzzing discovered that for some reason cargo was skiping based + // on incomplete information in the following case: + // minimized bug found in: + // https://github.com/rust-lang/cargo/commit/003c29b0c71e5ea28fbe8e72c148c755c9f3f8d9 + let input = vec![ + pkg!{("to_yank", "3.0.3")}, + pkg!{("to_yank", "3.3.0")}, + pkg!{("to_yank", "3.3.1")}, + pkg!{("a", "3.3.0") => [ + dep_req("to_yank", "=3.0.3"), + ] }, + pkg!{("a", "3.3.2") => [ + dep_req("to_yank", "<=3.3.0"), + ] }, + pkg!{("b", "0.1.3") => [ + dep_req("a", "=3.3.0"), + ] }, + pkg!{("b", "2.0.2") => [ + dep_req("to_yank", "3.3.0"), + dep_req("a", "*"), + ] }, + pkg!{("b", "2.3.3") => [ + dep_req("to_yank", "3.3.0"), + dep_req("a", "=3.3.0"), + ] }, + ]; + let reg = registry(input.clone()); + + let res = resolve(&pkg_id("root"), vec![dep_req("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)); + // so when we yank it + let new_reg = registry( + input + .iter() + .cloned() + .filter(|x| &package_to_yank != x.package_id()) + .collect(), + ); + assert_eq!(input.len(), new_reg.len() + 1); + // it should still build + assert!(resolve(&pkg_id("root"), vec![dep_req("b", "*")], &new_reg).is_ok()); +} + #[test] fn resolving_but_no_exists() { let reg = registry(vec![]);