Auto merge of #4515 - lukaslueg:issue1006, r=alexcrichton

Improve message for multiple links to native lib

Proposal for a fix to #1006, as advertised in my comment; as discussed briefly with alexcrichton on IRC.

In case multiple packages link to the same library, the error message is now

> error: More than one package links to native library `a`, which can only be linked once.
>
> Package a-sys v0.5.0 (file:///home/lukas/dev/issue1006test/a) links to native library `a`.
> (Dependency via foo v0.5.0 (file:///home/lukas/dev/issue1006test))
>
> Package b-sys v0.5.0 (file:///home/lukas/dev/issue1006test/a/b) also links to native library `a`.
> (Dependency via a-sys v0.5.0 (file:///home/lukas/dev/issue1006test/a) => foo v0.5.0 (file:///home/lukas/dev/issue1006test))
>
> Try updating or pinning your dependencies to ensure that native library `a` is only linked once.
>

In case the root-package itself is an offender:

> Package foo v0.5.0 (file:///home/lukas/dev/issue1006test) links to native library `a`.
> (This is the root-package)
>

IMHO the wording is much clearer now (the term "native library" and "package" are repeated on purpose); printing the whole dependency-chain from the offending package up to the root allows the user to at least figure out where the native library actually comes in.

Added a unit-test, which all pass. Please scrutinize this carefully, it's my first PR for cargo.

Author: bors

src/cargo/core/resolver/mod.rs

@@ -114,6 +114,31 @@ 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;
+        loop {
+            match self.graph
+                  .get_nodes()
+                  .iter()
+                  .filter_map(|(pulling, pulled)|
+                              if pulled.contains(pkg) {
+                                  Some(pulling)
+                              } else {
+                                  None
+                              })
+                  .nth(0) {
+                Some(pulling) => {
+                    result.push(pulling);
+                    pkg = pulling;
+                },
+                None => break
+            }
+        }
+        result
+    }
     pub fn register_used_patches(&mut self,
                                  patches: &HashMap<Url, Vec<Summary>>) {
         for summary in patches.values().flat_map(|v| v) {

src/cargo/ops/cargo_rustc/links.rs

@@ -1,6 +1,7 @@
 use std::collections::{HashMap, HashSet};
+use std::fmt::Write;
 
-use core::PackageId;
+use core::{Resolve, PackageId};
 use util::CargoResult;
 use super::Unit;
 
@@ -17,7 +18,7 @@ impl<'a> Links<'a> {
         }
     }
 
-    pub fn validate(&mut self, unit: &Unit<'a>) -> CargoResult<()> {
+    pub fn validate(&mut self, resolve: &Resolve, unit: &Unit<'a>) -> CargoResult<()> {
         if !self.validated.insert(unit.pkg.package_id()) {
             return Ok(())
         }
@@ -27,17 +28,31 @@ impl<'a> Links<'a> {
         };
         if let Some(prev) = self.links.get(lib) {
             let pkg = unit.pkg.package_id();
-            if prev.name() == pkg.name() && prev.source_id() == pkg.source_id() {
-                bail!("native library `{}` is being linked to by more \
-                       than one version of the same package, but it can \
-                       only be linked once; try updating or pinning your \
-                       dependencies to ensure that this package only shows \
-                       up once\n\n  {}\n  {}", lib, prev, pkg)
-            } else {
-                bail!("native library `{}` is being linked to by more than \
-                       one package, and can only be linked to by one \
-                       package\n\n  {}\n  {}", lib, prev, pkg)
-            }
+
+            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
+                }
+            };
+
+            bail!("Multiple packages link to native library `{}`. \
+                   A native library can be linked only once.\n\
+                   \n\
+                   {}links to native library `{}`.\n\
+                   \n\
+                   {}also links to native library `{}`.",
+                  lib,
+                  describe_path(prev), lib,
+                  describe_path(pkg), lib)
         }
         if !unit.pkg.manifest().targets().iter().any(|t| t.is_custom_build()) {
             bail!("package `{}` specifies that it links to `{}` but does not \

src/cargo/ops/cargo_rustc/mod.rs

@@ -237,7 +237,7 @@ fn compile<'a, 'cfg: 'a>(cx: &mut Context<'a, 'cfg>,
     let p = profile::start(format!("preparing: {}/{}", unit.pkg,
                                    unit.target.name()));
     fingerprint::prepare_init(cx, unit)?;
-    cx.links.validate(unit)?;
+    cx.links.validate(&cx.resolve, unit)?;
 
     let (dirty, fresh, freshness) = if unit.profile.run_custom_build {
         custom_build::prepare(cx, unit)?

tests/build-script.rs

@@ -220,6 +220,47 @@ not have a custom build script
 
 #[test]
 fn links_duplicates() {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.5.0"
+            authors = []
+            links = "a"
+            build = "build.rs"
+
+            [dependencies.a-sys]
+            path = "a-sys"
+        "#)
+        .file("src/lib.rs", "")
+        .file("build.rs", "")
+        .file("a-sys/Cargo.toml", r#"
+            [project]
+            name = "a-sys"
+            version = "0.5.0"
+            authors = []
+            links = "a"
+            build = "build.rs"
+        "#)
+        .file("a-sys/src/lib.rs", "")
+        .file("a-sys/build.rs", "");
+
+    assert_that(p.cargo_process("build"),
+                execs().with_status(101)
+                       .with_stderr("\
+[ERROR] Multiple packages link to native library `a`. A native library can be \
+linked only once.
+
+The root-package 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`.
+"));
+}
+
+#[test]
+fn links_duplicates_deep_dependency() {
     let p = project("foo")
         .file("Cargo.toml", r#"
             [project]
@@ -239,20 +280,36 @@ fn links_duplicates() {
             name = "a"
             version = "0.5.0"
             authors = []
-            links = "a"
             build = "build.rs"
+
+            [dependencies.a-sys]
+            path = "a-sys"
         "#)
         .file("a/src/lib.rs", "")
-        .file("a/build.rs", "");
+        .file("a/build.rs", "")
+        .file("a/a-sys/Cargo.toml", r#"
+            [project]
+            name = "a-sys"
+            version = "0.5.0"
+            authors = []
+            links = "a"
+            build = "build.rs"
+        "#)
+        .file("a/a-sys/src/lib.rs", "")
+        .file("a/a-sys/build.rs", "");
 
     assert_that(p.cargo_process("build"),
                 execs().with_status(101)
                        .with_stderr("\
-[ERROR] native library `a` is being linked to by more than one package, and can only be \
-linked to by one package
+[ERROR] Multiple packages link to native library `a`. A native library can be \
+linked only once.
+
+The root-package links to native library `a`.
 
-  [..] v0.5.0 (file://[..])
-  [..] v0.5.0 (file://[..])
+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`.
 "));
 }