From: Giles Cope <gilescope@gmail.com>
Date: Wed, 7 Mar 2018 22:56:47 +0000 (+0000)
Subject: As pointed out by Jacob, we can display the ordered cycle in the error message.
X-Git-Tag: archive/raspbian/0.35.0-2+rpi1~3^2^2^2^2^2^2^2~22^2~2^2~52^2~2
X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=32c7ac144b9de23688e880694b40f8b90d5709f0;p=cargo.git

As pointed out by Jacob, we can display the ordered cycle in the error message.
---

diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index 234646b3a..dfcdb71fb 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -985,6 +985,19 @@ fn find_candidate(
     None
 }
 
+/// Returns String representation of dependency chain for a particular `pkgid`.
+fn describe_path(graph: &Graph<PackageId>, pkgid: &PackageId) -> String {
+    use std::fmt::Write;
+    let dep_path = graph.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
+}
+
 fn activation_error(cx: &Context,
                     registry: &mut Registry,
                     parent: &Summary,
@@ -993,21 +1006,10 @@ fn activation_error(cx: &Context,
                     candidates: &[Candidate],
                     config: Option<&Config>) -> CargoError {
     let graph = cx.graph();
-    let describe_path = |pkgid: &PackageId| -> String {
-        use std::fmt::Write;
-        let dep_path = graph.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
-    };
     if !candidates.is_empty() {
         let mut msg = format!("failed to select a version for `{}`.", dep.name());
         msg.push_str("\n    ... required by ");
-        msg.push_str(&describe_path(parent.package_id()));
+        msg.push_str(&describe_path(&graph, parent.package_id()));
 
         msg.push_str("\nversions that meet the requirements `");
         msg.push_str(&dep.version_req().to_string());
@@ -1032,7 +1034,7 @@ fn activation_error(cx: &Context,
                 msg.push_str(link);
                 msg.push_str("` as well:\n");
             }
-            msg.push_str(&describe_path(p));
+            msg.push_str(&describe_path(&graph, p));
         }
 
         let (features_errors, other_errors): (Vec<_>, Vec<_>) = other_errors.drain(..).partition(|&(_, r)| r.is_missing_features());
@@ -1059,7 +1061,7 @@ fn activation_error(cx: &Context,
 
         for &(p, _) in other_errors.iter() {
             msg.push_str("\n\n  previously selected ");
-            msg.push_str(&describe_path(p));
+            msg.push_str(&describe_path(&graph, p));
         }
 
         msg.push_str("\n\nfailed to select a version for `");
@@ -1108,7 +1110,7 @@ fn activation_error(cx: &Context,
                               dep.source_id(),
                               versions);
         msg.push_str("required by ");
-        msg.push_str(&describe_path(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
@@ -1125,7 +1127,7 @@ fn activation_error(cx: &Context,
                  location searched: {}\n",
                 dep.name(), dep.source_id());
         msg.push_str("required by ");
-        msg.push_str(&describe_path(parent.package_id()));
+        msg.push_str(&describe_path(&graph, parent.package_id()));
 
         msg
     };
@@ -1491,12 +1493,8 @@ fn check_cycles(resolve: &Resolve, activations: &Activations)
                  -> CargoResult<()> {
         // See if we visited ourselves
         if !visited.insert(id) {
-            let mut cycle = String::new();
-            for package_id in visited.iter() {
-                 cycle += &format!("\n    {}", package_id);
-            }
-            bail!("cyclic package dependency: package `{}` depends on itself. Cycle (not in order):{}",
-                  id, cycle);
+            bail!("cyclic package dependency: package `{}` depends on itself. Cycle:\n{}",
+                  id, describe_path(&resolve.graph, id));
         }
 
         // If we've already checked this node no need to recurse again as we'll
diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs
index f7f2d532b..9e5dead5e 100644
--- a/tests/testsuite/build.rs
+++ b/tests/testsuite/build.rs
@@ -1532,9 +1532,8 @@ fn self_dependency() {
     assert_that(p.cargo("build"),
                 execs().with_status(101)
                        .with_stderr_contains("\
-[ERROR] cyclic package dependency: package `test v0.0.0 ([..])` depends on itself. Cycle (not in order):
-    test v0.0.0 ([..][/]foo)
-"));
+[ERROR] cyclic package dependency: package `test v0.0.0 ([..])` depends on itself. Cycle:
+package `test v0.0.0 ([..]foo)`"));
 }
 
 #[test]
@@ -2653,12 +2652,10 @@ fn cyclic_deps_rejected() {
 
     assert_that(p.cargo("build").arg("-v"),
                 execs().with_status(101)
-                       .with_stderr_contains("\
-[ERROR] cyclic package dependency: package `a v0.0.1 ([..]")
-                       .with_stderr_contains("[..]depends on itself[..]")
-                       .with_stderr_contains("    foo v0.0.1 ([..][/]foo)")
-                       .with_stderr_contains("    a v0.0.1 ([..][/]a)"));
-
+                       .with_stderr_contains(
+r#"[ERROR] cyclic package dependency: package `a v0.0.1 ([..])` depends on itself. Cycle:
+package `a v0.0.1 ([..]a)`
+    ... which is depended on by `foo v0.0.1 ([..]foo)`[..]"#));
 }
 
 #[test]