Fix an infinite loop in error reporting

The `path_to_root` function unfortunately didn't account for cycles in the
dependency graph introduced through dev-dependencies, so if a cycle was present
then the function would infinitely loop pushing items onto a vector.

This commit fixes the infinite loop and also touches up the reporting to be a
little more consistent with the rest of Cargo

diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index 9ce0a9e2f47cd6c66c6be9e2e8351bca80fd8326..712808391dc0428f201d3b581c5ba7f4fcb76c05 100644 (file)
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -118,21 +118,26 @@ struct Candidate {
 impl Resolve {
     /// Resolves one of the paths from the given dependent package up to
     /// the root.
-    pub fn path_to_top(&self, pkg: &PackageId) -> Vec<&PackageId> {
-        let mut result = Vec::new();
-        let mut pkg = pkg;
-        while let Some(pulling) = self.graph
-                                    .get_nodes()
-                                    .iter()
-                                    .filter_map(|(pulling, pulled)|
-                                                if pulled.contains(pkg) {
-                                                    Some(pulling)
-                                                } else {
-                                                    None
-                                                })
-                                    .nth(0) {
-            result.push(pulling);
-            pkg = pulling;
+    pub fn path_to_top<'a>(&'a self, mut pkg: &'a PackageId) -> Vec<&'a PackageId> {
+        // 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: &PackageId| {
+            self.graph.get_nodes()
+                .iter()
+                .filter(|&(_node, adjacent)| adjacent.contains(pkg))
+                .next()
+                .map(|p| p.0)
+        };
+        while let Some(p) = first_pkg_depending_on(pkg) {
+            // Note that we can have "cycles" introduced through dev-dependency
+            // edges, so make sure we don't loop infinitely.
+            if result.contains(&p) {
+                break
+            }
+            result.push(p);
+            pkg = p;
         }
         result
     }

diff --git a/src/cargo/ops/cargo_rustc/links.rs b/src/cargo/ops/cargo_rustc/links.rs
index 79bb240cf82b38cc9197afb5d3e0169e0ede4fab..5484a68b9a3df6c565048e0c316ba1707fd1f737 100644 (file)
--- a/src/cargo/ops/cargo_rustc/links.rs
+++ b/src/cargo/ops/cargo_rustc/links.rs
@@ -31,25 +31,21 @@ impl<'a> Links<'a> {
 
             let describe_path = |pkgid: &PackageId| -> String {
                 let dep_path = resolve.path_to_top(pkgid);
-                if dep_path.is_empty() {
-                    String::from("The root-package ")
-                } else {
-                    let mut dep_path_desc = format!("Package `{}`\n", pkgid);
-                    for dep in dep_path {
-                        write!(dep_path_desc,
-                               "    ... which is depended on by `{}`\n",
-                               dep).unwrap();
-                    }
-                    dep_path_desc
+                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
             };
 
-            bail!("Multiple packages link to native library `{}`. \
-                   A native library can be linked only once.\n\
+            bail!("multiple packages link to native library `{}`, \
+                   but a native library can be linked only once\n\
                    \n\
-                   {}links to native library `{}`.\n\
+                   {}\nlinks to native library `{}`\n\
                    \n\
-                   {}also links to native library `{}`.",
+                   {}\nalso links to native library `{}`",
                   lib,
                   describe_path(prev), lib,
                   describe_path(pkg), lib)

diff --git a/tests/build-script.rs b/tests/build-script.rs
index 3644235a3e6ffe2e4adcfddb40ec1a07bc262a48..145db690277fd22b577c645d32173c9910afd0f4 100644 (file)
--- a/tests/build-script.rs
+++ b/tests/build-script.rs
@@ -254,14 +254,15 @@ fn links_duplicates() {
     assert_that(p.cargo("build"),
                 execs().with_status(101)
                        .with_stderr("\
-[ERROR] Multiple packages link to native library `a`. A native library can be \
-linked only once.
+[ERROR] multiple packages link to native library `a`, but a native library can \
+be linked only once
 
-The root-package links to native library `a`.
+package `foo v0.5.0 ([..])`
+links to native library `a`
 
-Package `a-sys v0.5.0 (file://[..])`
-    ... which is depended on by `foo v0.5.0 (file://[..])`
-also links to native library `a`.
+package `a-sys v0.5.0 ([..])`
+    ... which is depended on by `foo v0.5.0 ([..])`
+also links to native library `a`
 "));
 }
 
@@ -308,15 +309,16 @@ fn links_duplicates_deep_dependency() {
     assert_that(p.cargo("build"),
                 execs().with_status(101)
                        .with_stderr("\
-[ERROR] Multiple packages link to native library `a`. A native library can be \
-linked only once.
+[ERROR] multiple packages link to native library `a`, but a native library can \
+be linked only once
 
-The root-package links to native library `a`.
+package `foo v0.5.0 ([..])`
+links to native library `a`
 
-Package `a-sys v0.5.0 (file://[..])`
-    ... which is depended on by `a v0.5.0 (file://[..])`
-    ... which is depended on by `foo v0.5.0 (file://[..])`
-also links to native library `a`.
+package `a-sys v0.5.0 ([..])`
+    ... which is depended on by `a v0.5.0 ([..])`
+    ... which is depended on by `foo v0.5.0 ([..])`
+also links to native library `a`
 "));
 }
 
@@ -2734,3 +2736,61 @@ fn deterministic_rustc_dependency_flags() {
 -L native=test3 -L native=test4`
 "));
 }
+
+#[test]
+fn links_duplicates_with_cycle() {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.5.0"
+            authors = []
+            links = "a"
+            build = "build.rs"
+
+            [dependencies.a]
+            path = "a"
+
+            [dev-dependencies]
+            b = { path = "b" }
+        "#)
+        .file("src/lib.rs", "")
+        .file("build.rs", "")
+        .file("a/Cargo.toml", r#"
+            [project]
+            name = "a"
+            version = "0.5.0"
+            authors = []
+            links = "a"
+            build = "build.rs"
+        "#)
+        .file("a/src/lib.rs", "")
+        .file("a/build.rs", "")
+        .file("b/Cargo.toml", r#"
+            [project]
+            name = "b"
+            version = "0.5.0"
+            authors = []
+
+            [dependencies]
+            foo = { path = ".." }
+        "#)
+        .file("b/src/lib.rs", "")
+        .build();
+
+    assert_that(p.cargo("build"),
+                execs().with_status(101)
+                       .with_stderr("\
+[ERROR] multiple packages link to native library `a`, but a native library can \
+be linked only once
+
+package `foo v0.5.0 ([..])`
+    ... which is depended on by `b v0.5.0 ([..])`
+links to native library `a`
+
+package `a v0.5.0 (file://[..])`
+    ... which is depended on by `foo v0.5.0 ([..])`
+    ... which is depended on by `b v0.5.0 ([..])`
+also links to native library `a`
+"));
+}