From 0bd1d34cae6b1c21bee9cd217b35fb8939992d84 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 24 Jul 2019 07:31:42 -0700 Subject: [PATCH] Fix detection of cyclic dependencies through `[patch]` This commit fixes detection of cyclic dependencies through the use of `[patch]` by ensuring that `matches_id` isn't used because it returns a false negative for registry dependencies when the dependency specifications don't match but the resolve edges are still correct. Closes #7163 --- src/cargo/core/resolver/encode.rs | 2 +- src/cargo/core/resolver/mod.rs | 26 ++++------- src/cargo/core/resolver/resolve.rs | 14 +++--- src/cargo/ops/cargo_generate_lockfile.rs | 2 +- src/cargo/ops/resolve.rs | 8 +++- tests/testsuite/patch.rs | 55 ++++++++++++++++++++++++ 6 files changed, 80 insertions(+), 27 deletions(-) diff --git a/src/cargo/core/resolver/encode.rs b/src/cargo/core/resolver/encode.rs index 84a92c5645e..d605b5e0c33 100644 --- a/src/cargo/core/resolver/encode.rs +++ b/src/cargo/core/resolver/encode.rs @@ -582,7 +582,7 @@ fn encodable_resolve_node( None => { let mut deps = resolve .deps_not_replaced(id) - .map(|id| encodable_package_id(id, state)) + .map(|(id, _)| encodable_package_id(id, state)) .collect::>(); deps.sort(); (None, Some(deps)) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index d6c9bcadb35..b7fa8b213fb 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -60,7 +60,7 @@ use crate::util::config::Config; use crate::util::errors::CargoResult; use crate::util::profile; -use self::context::{Activations, Context}; +use self::context::Context; use self::dep_cache::RegistryQueryer; use self::types::{ConflictMap, ConflictReason, DepsFrame}; use self::types::{FeaturesSet, RcVecIter, RemainingDeps, ResolverProgress}; @@ -154,7 +154,7 @@ pub fn resolve( ResolveVersion::default(), ); - check_cycles(&resolve, &cx.activations)?; + check_cycles(&resolve)?; check_duplicate_pkgs_in_lockfile(&resolve)?; trace!("resolved: {:?}", resolve); @@ -1048,19 +1048,14 @@ fn find_candidate( None } -fn check_cycles(resolve: &Resolve, activations: &Activations) -> CargoResult<()> { - let summaries: HashMap = activations - .values() - .map(|(s, _)| (s.package_id(), s)) - .collect(); - +fn check_cycles(resolve: &Resolve) -> CargoResult<()> { // Sort packages to produce user friendly deterministic errors. let mut all_packages: Vec<_> = resolve.iter().collect(); all_packages.sort_unstable(); let mut checked = HashSet::new(); for pkg in all_packages { if !checked.contains(&pkg) { - visit(resolve, pkg, &summaries, &mut HashSet::new(), &mut checked)? + visit(resolve, pkg, &mut HashSet::new(), &mut checked)? } } return Ok(()); @@ -1068,7 +1063,6 @@ fn check_cycles(resolve: &Resolve, activations: &Activations) -> CargoResult<()> fn visit( resolve: &Resolve, id: PackageId, - summaries: &HashMap, visited: &mut HashSet, checked: &mut HashSet, ) -> CargoResult<()> { @@ -1089,22 +1083,18 @@ fn check_cycles(resolve: &Resolve, activations: &Activations) -> CargoResult<()> // visitation list as we can't induce a cycle through transitive // dependencies. if checked.insert(id) { - let summary = summaries[&id]; - for dep in resolve.deps_not_replaced(id) { - let is_transitive = summary - .dependencies() - .iter() - .any(|d| d.matches_id(dep) && d.is_transitive()); + for (dep, listings) in resolve.deps_not_replaced(id) { + let is_transitive = listings.iter().any(|d| d.is_transitive()); let mut empty = HashSet::new(); let visited = if is_transitive { &mut *visited } else { &mut empty }; - visit(resolve, dep, summaries, visited, checked)?; + visit(resolve, dep, visited, checked)?; if let Some(id) = resolve.replacement(dep) { - visit(resolve, id, summaries, visited, checked)?; + visit(resolve, id, visited, checked)?; } } } diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index 3838ee3c36e..c1353e6299e 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -229,13 +229,17 @@ unable to verify that `{0}` is the same as when the lockfile was generated } pub fn deps(&self, pkg: PackageId) -> impl Iterator { - self.graph - .edges(&pkg) - .map(move |&(id, ref deps)| (self.replacement(id).unwrap_or(id), deps.as_slice())) + self.deps_not_replaced(pkg) + .map(move |(id, deps)| (self.replacement(id).unwrap_or(id), deps)) } - pub fn deps_not_replaced<'a>(&'a self, pkg: PackageId) -> impl Iterator + 'a { - self.graph.edges(&pkg).map(|&(id, _)| id) + pub fn deps_not_replaced( + &self, + pkg: PackageId, + ) -> impl Iterator { + self.graph + .edges(&pkg) + .map(|(id, deps)| (*id, deps.as_slice())) } pub fn replacement(&self, pkg: PackageId) -> Option { diff --git a/src/cargo/ops/cargo_generate_lockfile.rs b/src/cargo/ops/cargo_generate_lockfile.rs index 16495a41cda..1055e52f713 100644 --- a/src/cargo/ops/cargo_generate_lockfile.rs +++ b/src/cargo/ops/cargo_generate_lockfile.rs @@ -154,7 +154,7 @@ pub fn update_lockfile(ws: &Workspace<'_>, opts: &UpdateOptions<'_>) -> CargoRes return; } set.insert(dep); - for dep in resolve.deps_not_replaced(dep) { + for (dep, _) in resolve.deps_not_replaced(dep) { fill_with_deps(resolve, dep, set, visited); } } diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index 14da2061179..b660be8b6ca 100644 --- a/src/cargo/ops/resolve.rs +++ b/src/cargo/ops/resolve.rs @@ -572,7 +572,11 @@ fn register_previous_locks( let keep = |id: &PackageId| keep(id) && !avoid_locking.contains(id); for node in resolve.iter().filter(keep) { - let deps = resolve.deps_not_replaced(node).filter(keep).collect(); + let deps = resolve + .deps_not_replaced(node) + .map(|p| p.0) + .filter(keep) + .collect(); registry.register_lock(node, deps); } @@ -582,7 +586,7 @@ fn register_previous_locks( return; } debug!("ignoring any lock pointing directly at {}", node); - for dep in resolve.deps_not_replaced(node) { + for (dep, _) in resolve.deps_not_replaced(node) { add_deps(resolve, dep, set); } } diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs index 408f2b7e0d0..e3d699cc812 100644 --- a/tests/testsuite/patch.rs +++ b/tests/testsuite/patch.rs @@ -1079,3 +1079,58 @@ fn patch_older() { ) .run(); } + +#[cargo_test] +fn cycle() { + Package::new("a", "1.0.0").publish(); + Package::new("b", "1.0.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["a", "b"] + + [patch.crates-io] + a = {path="a"} + b = {path="b"} + "#, + ) + .file( + "a/Cargo.toml", + r#" + [package] + name = "a" + version = "1.0.0" + + [dependencies] + b = "1.0" + "#, + ) + .file("a/src/lib.rs", "") + .file( + "b/Cargo.toml", + r#" + [package] + name = "b" + version = "1.0.0" + + [dependencies] + a = "1.0" + "#, + ) + .file("b/src/lib.rs", "") + .build(); + + p.cargo("check") + .with_status(101) + .with_stderr( + "\ +[UPDATING] [..] +error: cyclic package dependency: [..] +package `[..]` + ... which is depended on by `[..]` +", + ) + .run(); +}