Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resolve error messages #6374

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -565,10 +565,19 @@ impl Links {
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();
let dep_path = resolve.graph().path_to_top(&pkgid);
let mut dep_path_desc = format!("package `{}`", pkgid);
for &(dep, req) in dep_path.iter() {
let req = req.first().unwrap();
write!(
dep_path_desc,
"\n ... selected to fulfill the requirement \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"\n ... selected to fulfill the requirement \
"\n ... selected to satisfy the requirement \

"Satisfying" dependencies is already a term that a lot of people are familiar with,
even (or especially) when English isn't their first language.
Using this would probably make it more clear.

(just randomly found this PR, sorry for sort of necromancing, in case this has been resolved elsewhere)

`{} = \"{}\"` from package `{}`",
req.name_in_toml(),
req.version_req(),
dep
)
.unwrap();
}
dep_path_desc
};
Expand Down
49 changes: 35 additions & 14 deletions src/cargo/core/resolver/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,14 @@ pub(super) fn activation_error(
let to_resolve_err = |err| {
ResolveError::new(
err,
graph
.path_to_top(&parent.package_id())
.into_iter()
Some(parent.package_id())
.iter()
.chain(
graph
.path_to_top(&parent.package_id())
.into_iter()
.map(|x| x.0),
)
.cloned()
.collect(),
)
Expand All @@ -92,7 +97,11 @@ 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(&graph.path_to_top(&parent.package_id())));
msg.push_str(&describe_path(&graph, parent.package_id()));
msg.push_str("which requires ");
msg.push_str(&dep.package_name());
msg.push_str(" ");
msg.push_str(&dep.version_req().to_string());

msg.push_str("\nversions that meet the requirements `");
msg.push_str(&dep.version_req().to_string());
Expand All @@ -113,7 +122,7 @@ pub(super) fn activation_error(
.rev()
.partition(|&(_, r)| r.is_links());

for &(p, r) in links_errors.iter() {
for &(&p, r) in links_errors.iter() {
if let ConflictReason::Links(ref link) = *r {
msg.push_str("\n\nthe package `");
msg.push_str(&*dep.package_name());
Expand All @@ -123,7 +132,7 @@ pub(super) fn activation_error(
msg.push_str(link);
msg.push_str("` as well:\n");
}
msg.push_str(&describe_path(&graph.path_to_top(p)));
msg.push_str(&describe_path(&graph, p));
}

let (features_errors, other_errors): (Vec<_>, Vec<_>) = other_errors
Expand Down Expand Up @@ -152,9 +161,9 @@ pub(super) fn activation_error(
);
}

for &(p, _) in other_errors.iter() {
for &(&p, _) in other_errors.iter() {
msg.push_str("\n\n previously selected ");
msg.push_str(&describe_path(&graph.path_to_top(p)));
msg.push_str(&describe_path(&graph, p));
}

msg.push_str("\n\nfailed to select a version for `");
Expand Down Expand Up @@ -204,7 +213,7 @@ pub(super) fn activation_error(
registry.describe_source(dep.source_id()),
);
msg.push_str("required by ");
msg.push_str(&describe_path(&graph.path_to_top(&parent.package_id())));
msg.push_str(&describe_path(&graph, 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
Expand Down Expand Up @@ -258,7 +267,7 @@ pub(super) fn activation_error(
msg.push_str("\n");
}
msg.push_str("required by ");
msg.push_str(&describe_path(&graph.path_to_top(&parent.package_id())));
msg.push_str(&describe_path(&graph, parent.package_id()));

msg
};
Expand All @@ -278,11 +287,23 @@ pub(super) fn activation_error(
}

/// Returns String representation of dependency chain for a particular `pkgid`.
pub(super) fn describe_path(path: &[&PackageId]) -> String {
pub(super) fn describe_path(
graph: &::util::graph::Graph<PackageId, Vec<Dependency>>,
this: PackageId,
) -> 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();
let path = &graph.path_to_top(&this);
let mut dep_path_desc = format!("package `{}`", this);
for &(dep, req) in path.iter() {
let req = req.first().unwrap();
write!(
dep_path_desc,
"\n ... selected to fulfill the requirement `{} = \"{}\"` from package `{}`",
req.name_in_toml(),
req.version_req(),
dep
)
.unwrap();
}
dep_path_desc
}
2 changes: 1 addition & 1 deletion src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -848,7 +848,7 @@ fn check_cycles(resolve: &Resolve, activations: &Activations) -> CargoResult<()>
bail!(
"cyclic package dependency: package `{}` depends on itself. Cycle:\n{}",
id,
errors::describe_path(&resolve.path_to_top(&id))
errors::describe_path(&resolve.graph(), id)
);
}

Expand Down
10 changes: 4 additions & 6 deletions src/cargo/core/resolver/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,6 @@ 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> {
self.graph.path_to_top(pkg)
}

pub fn register_used_patches(&mut self, patches: &HashMap<Url, Vec<Summary>>) {
for summary in patches.values().flat_map(|v| v) {
if self.iter().any(|id| id == summary.package_id()) {
Expand Down Expand Up @@ -194,6 +188,10 @@ unable to verify that `{0}` is the same as when the lockfile was generated
&self.replacements
}

pub fn graph(&self) -> &Graph<PackageId, Vec<Dependency>> {
&self.graph
}

pub fn features(&self, pkg: PackageId) -> &HashSet<String> {
self.features.get(&pkg).unwrap_or(&self.empty_features)
}
Expand Down
13 changes: 7 additions & 6 deletions src/cargo/util/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,23 +72,24 @@ impl<N: Eq + Hash + Clone, E: Default> Graph<N, E> {

/// 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> {
pub fn path_to_top<'s: 'q, 'q>(&'s self, mut pkg: &'q N) -> Vec<(&'s N, &'s 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<(&'s N, &'s E)> = vec![];
let first_pkg_depending_on = |pkg: &N, res: &[(&'s N, &'s 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.0)
.find(|&(node, _)| res.iter().find(|p| p.0 == node).is_none())
// TODO: find_map would be clearer
.map(|(node, adjacent)| (node, adjacent.get(pkg).unwrap()))
};
while let Some(p) = first_pkg_depending_on(pkg, &result) {
result.push(p);
pkg = p;
pkg = p.0;
}
result
}
Expand Down
Loading