From a5f8bc94f5d38539dd127f735ea4d3a515c230fd Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 23 Aug 2021 11:36:42 +0800 Subject: [PATCH] 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)`