From 4b4dc0a4797d4847a5375785028afbee47fa3683 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 14 Jan 2021 08:40:12 -0800 Subject: [PATCH] Fix a bug in Cargo's cyclic dep graph detection Cargo's cyclic dependency graph detection turns out to have had a bug for quite a long time as surfaced by #9073. The bug in Cargo has to do with how dev-dependencies are handled. Cycles are "allowed" through dev-dependencies because the dev-dependency can depend on the original crate. Our cyclic graph detection, however, was too eagerly flagging a package as known to not have a cycle before we had processed everything about it. The fix here was basically to just simplify the graph traversal. Instead of traversing the raw `Resolve` this instead creates an alternate in-memory graph which has the actual edges we care about for cycle detection (e.g. every edge that wasn't induced via a dev-dependency). With this simplified graph we then apply the exact same algorithm, but this time it should be less buggy because we're not trying to do funky things about switching sets about what's visited halfway through a traversal. Closes #9073 --- src/cargo/core/resolver/mod.rs | 60 ++++++++++++++-------------- tests/testsuite/path.rs | 72 ++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 31 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 094c64065b1..0531f3dba89 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -47,7 +47,7 @@ //! that we're implementing something that probably shouldn't be allocating all //! over the place. -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::mem; use std::rc::Rc; use std::time::{Duration, Instant}; @@ -1001,28 +1001,46 @@ fn find_candidate( } 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(); + // Create a simple graph representation alternative of `resolve` which has + // only the edges we care about. Note that `BTree*` is used to produce + // deterministic error messages here. Also note that the main reason for + // this copy of the resolve graph is to avoid edges between a crate and its + // dev-dependency since that doesn't count for cycles. + let mut graph = BTreeMap::new(); + for id in resolve.iter() { + let set = graph.entry(id).or_insert_with(BTreeSet::new); + for (dep, listings) in resolve.deps_not_replaced(id) { + let is_transitive = listings.iter().any(|d| d.is_transitive()); + + if is_transitive { + set.insert(dep); + set.extend(resolve.replacement(dep)); + } + } + } + + // After we have the `graph` that we care about, perform a simple cycle + // check by visiting all nodes. We visit each node at most once and we keep + // track of the path through the graph as we walk it. If we walk onto the + // same node twice that's a cycle. let mut checked = HashSet::new(); let mut path = Vec::new(); let mut visited = HashSet::new(); - for pkg in all_packages { - if !checked.contains(&pkg) { - visit(resolve, pkg, &mut visited, &mut path, &mut checked)? + for pkg in graph.keys() { + if !checked.contains(pkg) { + visit(&graph, *pkg, &mut visited, &mut path, &mut checked)? } } return Ok(()); fn visit( - resolve: &Resolve, + graph: &BTreeMap>, id: PackageId, visited: &mut HashSet, path: &mut Vec, checked: &mut HashSet, ) -> CargoResult<()> { path.push(id); - // See if we visited ourselves if !visited.insert(id) { anyhow::bail!( "cyclic package dependency: package `{}` depends on itself. Cycle:\n{}", @@ -1031,32 +1049,12 @@ fn check_cycles(resolve: &Resolve) -> CargoResult<()> { ); } - // If we've already checked this node no need to recurse again as we'll - // just conclude the same thing as last time, so we only execute the - // recursive step if we successfully insert into `checked`. - // - // Note that if we hit an intransitive dependency then we clear out the - // visitation list as we can't induce a cycle through transitive - // dependencies. if checked.insert(id) { - let mut empty_set = HashSet::new(); - let mut empty_vec = Vec::new(); - for (dep, listings) in resolve.deps_not_replaced(id) { - let is_transitive = listings.iter().any(|d| d.is_transitive()); - let (visited, path) = if is_transitive { - (&mut *visited, &mut *path) - } else { - (&mut empty_set, &mut empty_vec) - }; - visit(resolve, dep, visited, path, checked)?; - - if let Some(id) = resolve.replacement(dep) { - visit(resolve, id, visited, path, checked)?; - } + for dep in graph[&id].iter() { + visit(graph, *dep, visited, path, checked)?; } } - // Ok, we're done, no longer visiting our node any more path.pop(); visited.remove(&id); Ok(()) diff --git a/tests/testsuite/path.rs b/tests/testsuite/path.rs index d0f380156f2..34cdffb3e13 100644 --- a/tests/testsuite/path.rs +++ b/tests/testsuite/path.rs @@ -1063,3 +1063,75 @@ Caused by: ) .run(); } + +#[cargo_test] +fn catch_tricky_cycle() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "message" + version = "0.1.0" + + [dev-dependencies] + test = { path = "test" } + "#, + ) + .file("src/lib.rs", "") + .file( + "tangle/Cargo.toml", + r#" + [package] + name = "tangle" + version = "0.1.0" + + [dependencies] + message = { path = ".." } + snapshot = { path = "../snapshot" } + "#, + ) + .file("tangle/src/lib.rs", "") + .file( + "snapshot/Cargo.toml", + r#" + [package] + name = "snapshot" + version = "0.1.0" + + [dependencies] + ledger = { path = "../ledger" } + "#, + ) + .file("snapshot/src/lib.rs", "") + .file( + "ledger/Cargo.toml", + r#" + [package] + name = "ledger" + version = "0.1.0" + + [dependencies] + tangle = { path = "../tangle" } + "#, + ) + .file("ledger/src/lib.rs", "") + .file( + "test/Cargo.toml", + r#" + [package] + name = "test" + version = "0.1.0" + + [dependencies] + snapshot = { path = "../snapshot" } + "#, + ) + .file("test/src/lib.rs", "") + .build(); + + p.cargo("test") + .with_stderr_contains("[..]cyclic package dependency[..]") + .with_status(101) + .run(); +}