From 7d22a309d7262a0556ca6b3535cb92075f4dd21f Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 23 Aug 2021 00:04:41 +0800 Subject: [PATCH 1/4] Add dep requirement info for dep-chain display --- src/cargo/core/resolver/dep_cache.rs | 4 +- src/cargo/core/resolver/errors.rs | 72 +++++++++++++++++++++------- src/cargo/util/graph.rs | 15 ++++-- 3 files changed, 68 insertions(+), 23 deletions(-) diff --git a/src/cargo/core/resolver/dep_cache.rs b/src/cargo/core/resolver/dep_cache.rs index 82aec0b8faa..32bc6507670 100644 --- a/src/cargo/core/resolver/dep_cache.rs +++ b/src/cargo/core/resolver/dep_cache.rs @@ -10,7 +10,7 @@ //! This module impl that cache in all the gory details use crate::core::resolver::context::Context; -use crate::core::resolver::errors::describe_path; +use crate::core::resolver::errors::describe_path_in_context; use crate::core::resolver::types::{ConflictReason, DepInfo, FeaturesSet}; use crate::core::resolver::{ ActivateError, ActivateResult, CliFeatures, RequestedFeatures, ResolveOpts, VersionOrdering, @@ -216,7 +216,7 @@ impl<'a> RegistryQueryer<'a> { format!( "failed to get `{}` as a dependency of {}", dep.package_name(), - describe_path(&cx.parents.path_to_bottom(&candidate.package_id())), + describe_path_in_context(cx, &candidate.package_id()), ) })?; Ok((dep, candidates, features)) diff --git a/src/cargo/core/resolver/errors.rs b/src/cargo/core/resolver/errors.rs index 72b190ee05b..d39029ce91a 100644 --- a/src/cargo/core/resolver/errors.rs +++ b/src/cargo/core/resolver/errors.rs @@ -82,6 +82,7 @@ pub(super) fn activation_error( cx.parents .path_to_bottom(&parent.package_id()) .into_iter() + .map(|(node, _)| node) .cloned() .collect(), ) @@ -90,9 +91,7 @@ pub(super) fn activation_error( if !candidates.is_empty() { let mut msg = format!("failed to select a version for `{}`.", dep.package_name()); msg.push_str("\n ... required by "); - msg.push_str(&describe_path( - &cx.parents.path_to_bottom(&parent.package_id()), - )); + msg.push_str(&describe_path_in_context(cx, &parent.package_id())); msg.push_str("\nversions that meet the requirements `"); msg.push_str(&dep.version_req().to_string()); @@ -128,7 +127,7 @@ pub(super) fn activation_error( msg.push_str("`, but it conflicts with a previous package which links to `"); msg.push_str(link); msg.push_str("` as well:\n"); - msg.push_str(&describe_path(&cx.parents.path_to_bottom(p))); + msg.push_str(&describe_path_in_context(cx, p)); msg.push_str("\nOnly one package in the dependency graph may specify the same links value. This helps ensure that only one copy of a native library is linked in the final binary. "); msg.push_str("Try to adjust your dependencies so that only one package uses the links ='"); msg.push_str(&*dep.package_name()); @@ -197,7 +196,7 @@ pub(super) fn activation_error( for (p, r) in &conflicting_activations { if let ConflictReason::Semver = r { msg.push_str("\n\n previously selected "); - msg.push_str(&describe_path(&cx.parents.path_to_bottom(p))); + msg.push_str(&describe_path_in_context(cx, p)); } } } @@ -250,9 +249,7 @@ pub(super) fn activation_error( registry.describe_source(dep.source_id()), ); msg.push_str("required by "); - msg.push_str(&describe_path( - &cx.parents.path_to_bottom(&parent.package_id()), - )); + msg.push_str(&describe_path_in_context(cx, &parent.package_id())); // If we have a path dependency with a locked version, then this may // indicate that we updated a sub-package and forgot to run `cargo @@ -330,9 +327,7 @@ pub(super) fn activation_error( } msg.push_str(&format!("location searched: {}\n", dep.source_id())); msg.push_str("required by "); - msg.push_str(&describe_path( - &cx.parents.path_to_bottom(&parent.package_id()), - )); + msg.push_str(&describe_path_in_context(cx, &parent.package_id())); msg }; @@ -351,12 +346,57 @@ pub(super) fn activation_error( to_resolve_err(anyhow::format_err!("{}", msg)) } +/// Returns String representation of dependency chain for a particular `pkgid` +/// within given context. +pub(super) fn describe_path_in_context(cx: &Context, id: &PackageId) -> String { + let iter = cx + .parents + .path_to_bottom(id) + .into_iter() + .map(|(p, d)| (p, d.and_then(|d| d.iter().next()))); + describe_path(iter) +} + /// Returns String representation of dependency chain for a particular `pkgid`. -pub(super) fn describe_path(path: &[&PackageId]) -> String { +/// +/// Note that all elements of `path` iterator should have `Some` dependency +/// except the first one. It would look like: +/// +/// (pkg0, None) +/// -> (pkg1, dep from pkg1 satisfied by pkg0) +/// -> (pkg2, dep from pkg2 satisfied by pkg1) +/// -> ... +pub(super) fn describe_path<'a>( + mut path: impl Iterator)>, +) -> String { use std::fmt::Write; - let mut dep_path_desc = format!("package `{}`", path[0]); - for dep in path[1..].iter() { - write!(dep_path_desc, "\n ... which is depended on by `{}`", dep).unwrap(); + + if let Some(p) = path.next() { + let mut dep_path_desc = format!("package `{}`", p.0); + for (pkg, dep) in path { + let dep = dep.unwrap(); + let source_kind = if dep.source_id().is_path() { + "path " + } else if dep.source_id().is_git() { + "git " + } else { + "" + }; + let requirement = if source_kind.is_empty() { + format!("{} = \"{}\"", dep.name_in_toml(), dep.version_req()) + } else { + dep.name_in_toml().to_string() + }; + write!( + dep_path_desc, + "\n ... which satisfies {}dependency `{}` of package `{}`", + source_kind, requirement, pkg + ) + .unwrap(); + } + + return dep_path_desc; } - dep_path_desc + + String::new() } diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index e5db1319e57..16c5f82e838 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -89,17 +89,22 @@ impl Graph { /// Resolves one of the paths from the given dependent package down to /// a leaf. - pub fn path_to_bottom<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> { - let mut result = vec![pkg]; + /// + /// Each element contains a node along with an edge except the first one. + /// The representation would look like: + /// + /// (Node0,) -> (Node1, Edge01) -> (Node2, Edge12)... + pub fn path_to_bottom<'a>(&'a self, mut pkg: &'a N) -> Vec<(&'a N, Option<&'a E>)> { + let mut result = vec![(pkg, None)]; while let Some(p) = self.nodes.get(pkg).and_then(|p| { p.iter() // Note that we can have "cycles" introduced through dev-dependency // edges, so make sure we don't loop infinitely. - .find(|(node, _)| !result.contains(node)) - .map(|(p, _)| p) + .find(|&(node, _)| result.iter().all(|p| p.0 != node)) + .map(|(node, edge)| (node, Some(edge))) }) { result.push(p); - pkg = p; + pkg = p.0; } result } From 70a3ccb885a7a7e573d5027fa5ed945524668d78 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 23 Aug 2021 00:08:02 +0800 Subject: [PATCH 2/4] Display dep requirement info for cyclic dependencies --- src/cargo/core/resolver/mod.rs | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 8f947594de7..4e1d2b80ec8 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, BTreeSet, HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::mem; use std::rc::Rc; use std::time::{Duration, Instant}; @@ -1007,13 +1007,15 @@ fn check_cycles(resolve: &Resolve) -> CargoResult<()> { // 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)); + let map = graph.entry(id).or_insert_with(BTreeMap::new); + for (dep_id, listings) in resolve.deps_not_replaced(id) { + let transitive_dep = listings.iter().find(|d| d.is_transitive()); + + if let Some(transitive_dep) = transitive_dep.cloned() { + map.insert(dep_id, transitive_dep.clone()); + resolve + .replacement(dep_id) + .map(|p| map.insert(p, transitive_dep)); } } } @@ -1033,7 +1035,7 @@ fn check_cycles(resolve: &Resolve) -> CargoResult<()> { return Ok(()); fn visit( - graph: &BTreeMap>, + graph: &BTreeMap>, id: PackageId, visited: &mut HashSet, path: &mut Vec, @@ -1041,15 +1043,21 @@ fn check_cycles(resolve: &Resolve) -> CargoResult<()> { ) -> CargoResult<()> { path.push(id); if !visited.insert(id) { + let iter = path.iter().rev().skip(1).scan(id, |child, parent| { + let dep = graph.get(parent).and_then(|adjacent| adjacent.get(child)); + *child = *parent; + Some((parent, dep)) + }); + let iter = std::iter::once((&id, None)).chain(iter); anyhow::bail!( "cyclic package dependency: package `{}` depends on itself. Cycle:\n{}", id, - errors::describe_path(&path.iter().rev().collect::>()), + errors::describe_path(iter), ); } if checked.insert(id) { - for dep in graph[&id].iter() { + for dep in graph[&id].keys() { visit(graph, *dep, visited, path, checked)?; } } From 0afd40b4de17a5c45145a0762beb4ef001720fe1 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 23 Aug 2021 00:10:15 +0800 Subject: [PATCH 3/4] Update tests to display dep-req info for dep-chain --- crates/resolver-tests/tests/resolve.rs | 4 ++-- tests/testsuite/build.rs | 20 ++++++++++---------- tests/testsuite/build_script.rs | 2 +- tests/testsuite/patch.rs | 6 +++--- tests/testsuite/path.rs | 4 ++-- tests/testsuite/registry.rs | 2 +- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index aa9e78c76e0..6a065b93800 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/crates/resolver-tests/tests/resolve.rs @@ -1498,7 +1498,7 @@ fn cyclic_good_error_message() { assert_eq!("\ cyclic package dependency: package `A v0.0.0 (registry `https://example.com/`)` depends on itself. Cycle: package `A v0.0.0 (registry `https://example.com/`)` - ... which is depended on by `C v0.0.0 (registry `https://example.com/`)` - ... which is depended on by `A v0.0.0 (registry `https://example.com/`)`\ + ... which satisfies dependency `A = \"*\"` of package `C v0.0.0 (registry `https://example.com/`)` + ... which satisfies dependency `C = \"*\"` of package `A v0.0.0 (registry `https://example.com/`)`\ ", error.to_string()); } diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 0e0b59fcda1..8aff41bb96b 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -1096,14 +1096,14 @@ fn incompatible_dependencies() { "\ error: failed to select a version for `bad`. ... required by package `qux v0.1.0` - ... which is depended on by `foo v0.0.1 ([..])` + ... which satisfies dependency `qux = \"^0.1.0\"` of package `foo v0.0.1 ([..])` versions that meet the requirements `>=1.0.1` are: 1.0.2, 1.0.1 all possible versions conflict with previously selected packages. previously selected package `bad v1.0.0` - ... which is depended on by `baz v0.1.0` - ... which is depended on by `foo v0.0.1 ([..])` + ... which satisfies dependency `bad = \"=1.0.0\"` of package `baz v0.1.0` + ... which satisfies dependency `baz = \"^0.1.0\"` of package `foo v0.0.1 ([..])` failed to select a version for `bad` which could resolve this conflict", ) @@ -1147,12 +1147,12 @@ versions that meet the requirements `>=1.0.1, <=2.0.0` are: 2.0.0, 1.0.1 all possible versions conflict with previously selected packages. previously selected package `bad v2.0.1` - ... which is depended on by `baz v0.1.0` - ... which is depended on by `foo v0.0.1 ([..])` + ... which satisfies dependency `bad = \">=2.0.1\"` of package `baz v0.1.0` + ... which satisfies dependency `baz = \"^0.1.0\"` of package `foo v0.0.1 ([..])` previously selected package `bad v1.0.0` - ... which is depended on by `bar v0.1.0` - ... which is depended on by `foo v0.0.1 ([..])` + ... which satisfies dependency `bad = \"=1.0.0\"` of package `bar v0.1.0` + ... which satisfies dependency `bar = \"^0.1.0\"` of package `foo v0.0.1 ([..])` failed to select a version for `bad` which could resolve this conflict", ) @@ -1662,7 +1662,7 @@ fn self_dependency() { "\ [ERROR] cyclic package dependency: package `test v0.0.0 ([CWD])` depends on itself. Cycle: package `test v0.0.0 ([CWD])` - ... which is depended on by `test v0.0.0 ([..])`", + ... which satisfies path dependency `test` of package `test v0.0.0 ([..])`", ) .run(); } @@ -2808,8 +2808,8 @@ fn cyclic_deps_rejected() { .with_stderr( "[ERROR] cyclic package dependency: package `a v0.0.1 ([CWD]/a)` depends on itself. Cycle: package `a v0.0.1 ([CWD]/a)` - ... which is depended on by `foo v0.0.1 ([CWD])` - ... which is depended on by `a v0.0.1 ([..])`", + ... which satisfies path dependency `a` of package `foo v0.0.1 ([CWD])` + ... which satisfies path dependency `foo` of package `a v0.0.1 ([..])`", ).run(); } diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index 5a8b5ea28f9..def40fb38c3 100644 --- a/tests/testsuite/build_script.rs +++ b/tests/testsuite/build_script.rs @@ -1064,7 +1064,7 @@ fn links_duplicates_deep_dependency() { .with_stderr("\ error: failed to select a version for `a-sys`. ... required by package `a v0.5.0 ([..])` - ... which is depended on by `foo v0.5.0 ([..])` + ... which satisfies path dependency `a` of package `foo v0.5.0 ([..])` versions that meet the requirements `*` are: 0.5.0 the package `a-sys` links to the native library `a`, but it conflicts with a previous package which links to `a` as well: diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs index 181802b6a3c..9f00544e417 100644 --- a/tests/testsuite/patch.rs +++ b/tests/testsuite/patch.rs @@ -1618,10 +1618,10 @@ fn cycle() { .with_stderr( "\ [UPDATING] [..] -error: cyclic package dependency: [..] +[ERROR] cyclic package dependency: [..] package `[..]` - ... which is depended on by `[..]` - ... which is depended on by `[..]` + ... which satisfies dependency `[..]` of package `[..]` + ... which satisfies dependency `[..]` of package `[..]` ", ) .run(); diff --git a/tests/testsuite/path.rs b/tests/testsuite/path.rs index 3043a85e067..9bcd220ef77 100644 --- a/tests/testsuite/path.rs +++ b/tests/testsuite/path.rs @@ -1046,8 +1046,8 @@ fn deep_path_error() { .with_stderr( "\ [ERROR] failed to get `c` as a dependency of package `b v0.1.0 [..]` - ... which is depended on by `a v0.1.0 [..]` - ... which is depended on by `foo v0.1.0 [..]` + ... which satisfies path dependency `b` of package `a v0.1.0 [..]` + ... which satisfies path dependency `a` of package `foo v0.1.0 [..]` Caused by: failed to load source for dependency `c` diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index f46effb0962..c62037012d4 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -547,7 +547,7 @@ error: failed to select a version for the requirement `baz = \"=0.0.2\"` candidate versions found which didn't match: 0.0.1 location searched: `[..]` index (which is replacing registry `[..]`) required by package `bar v0.0.1` - ... which is depended on by `foo [..]` + ... which satisfies dependency `bar = \"*\"` of package `foo [..]` ", ) .run(); From a5f8bc94f5d38539dd127f735ea4d3a515c230fd Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 23 Aug 2021 11:36:42 +0800 Subject: [PATCH 4/4] Improve resolver message for validate_links --- src/cargo/core/compiler/links.rs | 24 +++++++++++------------- src/cargo/core/resolver/errors.rs | 2 +- src/cargo/core/resolver/mod.rs | 2 +- src/cargo/core/resolver/resolve.rs | 5 ++++- src/cargo/util/graph.rs | 17 +++++++++++------ tests/testsuite/build_script.rs | 2 +- 6 files changed, 29 insertions(+), 23 deletions(-) diff --git a/src/cargo/core/compiler/links.rs b/src/cargo/core/compiler/links.rs index 8faa831ef4d..34c021f9309 100644 --- a/src/cargo/core/compiler/links.rs +++ b/src/cargo/core/compiler/links.rs @@ -1,8 +1,8 @@ use super::unit_graph::UnitGraph; +use crate::core::resolver::errors::describe_path; use crate::core::{PackageId, Resolve}; use crate::util::errors::CargoResult; use std::collections::{HashMap, HashSet}; -use std::fmt::Write; /// Validate `links` field does not conflict between packages. pub fn validate_links(resolve: &Resolve, unit_graph: &UnitGraph) -> CargoResult<()> { @@ -28,17 +28,15 @@ pub fn validate_links(resolve: &Resolve, unit_graph: &UnitGraph) -> CargoResult< None => continue, }; if let Some(&prev) = links.get(lib) { + let prev_path = resolve + .path_to_top(&prev) + .into_iter() + .map(|(p, d)| (p, d.and_then(|d| d.iter().next()))); let pkg = unit.pkg.package_id(); - - let describe_path = |pkgid: PackageId| -> String { - let dep_path = resolve.path_to_top(&pkgid); - let mut dep_path_desc = format!("package `{}`", dep_path[0]); - for dep in dep_path.iter().skip(1) { - write!(dep_path_desc, "\n ... which is depended on by `{}`", dep).unwrap(); - } - dep_path_desc - }; - + let path = resolve + .path_to_top(&pkg) + .into_iter() + .map(|(p, d)| (p, d.and_then(|d| d.iter().next()))); anyhow::bail!( "multiple packages link to native library `{}`, \ but a native library can be linked only once\n\ @@ -47,9 +45,9 @@ pub fn validate_links(resolve: &Resolve, unit_graph: &UnitGraph) -> CargoResult< \n\ {}\nalso links to native library `{}`", lib, - describe_path(prev), + describe_path(prev_path), lib, - describe_path(pkg), + describe_path(path), lib ) } diff --git a/src/cargo/core/resolver/errors.rs b/src/cargo/core/resolver/errors.rs index d39029ce91a..63d58392dc2 100644 --- a/src/cargo/core/resolver/errors.rs +++ b/src/cargo/core/resolver/errors.rs @@ -366,7 +366,7 @@ pub(super) fn describe_path_in_context(cx: &Context, id: &PackageId) -> String { /// -> (pkg1, dep from pkg1 satisfied by pkg0) /// -> (pkg2, dep from pkg2 satisfied by pkg1) /// -> ... -pub(super) fn describe_path<'a>( +pub(crate) fn describe_path<'a>( mut path: impl Iterator)>, ) -> String { use std::fmt::Write; diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 4e1d2b80ec8..44fa79575a9 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -78,7 +78,7 @@ mod conflict_cache; mod context; mod dep_cache; mod encode; -mod errors; +pub(crate) mod errors; pub mod features; mod resolve; mod types; diff --git a/src/cargo/core/resolver/resolve.rs b/src/cargo/core/resolver/resolve.rs index d024959b9e9..28fdb07300f 100644 --- a/src/cargo/core/resolver/resolve.rs +++ b/src/cargo/core/resolver/resolve.rs @@ -117,7 +117,10 @@ impl Resolve { /// Resolves one of the paths from the given dependent package up to /// the root. - pub fn path_to_top<'a>(&'a self, pkg: &'a PackageId) -> Vec<&'a PackageId> { + pub fn path_to_top<'a>( + &'a self, + pkg: &'a PackageId, + ) -> Vec<(&'a PackageId, Option<&'a HashSet>)> { self.graph.path_to_top(pkg) } diff --git a/src/cargo/util/graph.rs b/src/cargo/util/graph.rs index 16c5f82e838..ff4018201fe 100644 --- a/src/cargo/util/graph.rs +++ b/src/cargo/util/graph.rs @@ -111,23 +111,28 @@ impl Graph { /// Resolves one of the paths from the given dependent package up to /// the root. - pub fn path_to_top<'a>(&'a self, mut pkg: &'a N) -> Vec<&'a N> { + /// + /// Each element contains a node along with an edge except the first one. + /// The representation would look like: + /// + /// (Node0,) -> (Node1, Edge01) -> (Node2, Edge12)... + pub fn path_to_top<'a>(&'a self, mut pkg: &'a N) -> Vec<(&'a N, Option<&'a E>)> { // Note that this implementation isn't the most robust per se, we'll // likely have to tweak this over time. For now though it works for what // it's used for! - let mut result = vec![pkg]; - let first_pkg_depending_on = |pkg: &N, res: &[&N]| { + let mut result = vec![(pkg, None)]; + let first_pkg_depending_on = |pkg, res: &[(&N, Option<&E>)]| { self.nodes .iter() .filter(|(_, adjacent)| adjacent.contains_key(pkg)) // Note that we can have "cycles" introduced through dev-dependency // edges, so make sure we don't loop infinitely. - .find(|(node, _)| !res.contains(node)) - .map(|(p, _)| p) + .find(|&(node, _)| !res.iter().any(|p| p.0 == node)) + .map(|(p, adjacent)| (p, adjacent.get(pkg))) }; while let Some(p) = first_pkg_depending_on(pkg, &result) { result.push(p); - pkg = p; + pkg = p.0; } result } diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index def40fb38c3..9fffdc5cc39 100644 --- a/tests/testsuite/build_script.rs +++ b/tests/testsuite/build_script.rs @@ -1000,7 +1000,7 @@ fn links_duplicates_old_registry() { but a native library can be linked only once package `bar v0.1.0` - ... which is depended on by `foo v0.1.0 ([..]foo)` + ... which satisfies dependency `bar = \"=0.1.0\"` of package `foo v0.1.0 ([..]foo)` links to native library `a` package `foo v0.1.0 ([..]foo)`