Improve message for multiple links to native lib #4515
Conversation
If a native library is linked multiple times, present the user with a clear error message, indicating the offending packages and their dependency-chain. Fixes rust-lang#1006
|
(rust_highfive has picked a reviewer for you, use r? to override) |
src/cargo/ops/cargo_rustc/links.rs
Outdated
| let mut pkg_path_desc = String::from("(Dependency via "); | ||
| let mut dep_path_iter = dep_path.into_iter().peekable(); | ||
| while let Some(dep) = dep_path_iter.next() { | ||
| write!(pkg_path_desc, "{}", dep).unwrap(); |
There was a problem hiding this comment.
Perhaps this could have some newlines in it? I could imagine that the dependencies here are sometimes pretty deep, so this may take up a lot of terminal space
src/cargo/ops/cargo_rustc/links.rs
Outdated
| while let Some(dep) = dep_path_iter.next() { | ||
| write!(pkg_path_desc, "{}", dep).unwrap(); | ||
| if dep_path_iter.peek().is_some() { | ||
| pkg_path_desc.push_str(" => "); |
There was a problem hiding this comment.
Should this point the other way? The directionality here would entice me to interpret this as "the left depends on the right", but I think it's the other way?
We could also expand this a bit more perhaps like:
Package `foo v0.4.0` also links to native library `{}`.
... which is depended on by `bar v0.4.0`
... which is depended on by `test v0.3.0`
src/cargo/ops/cargo_rustc/links.rs
Outdated
| let describe_path = |pkgid: &PackageId| -> String { | ||
| let dep_path = resolve.path_to_top(pkgid); | ||
| if dep_path.is_empty() { | ||
| String::from("(This is the root-package)") |
There was a problem hiding this comment.
I think in this case the root package should likely be pretty recognizable, so perhaps the output could be suppressed here?
|
While compiling
Is the last sentence the best piece of advice we can give? |
|
That looks like a great message to me! I think it's fine to jettison the last clause about changing deps with this improvement |
|
The message is now
or
|
|
@bors: r+ Looks great, thanks! |
|
📌 Commit 45b4a55 has been approved by |
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.
|
☀️ Test successful - status-appveyor, status-travis |
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
In case the root-package itself is an offender:
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.