Bail out when trying to link to a library that is not linkable.

[lukaslueg]

There are more subtleties here than expected, as we can have situations where it is actually Ok to have no linkable targets: Build scripts are a common case, yet benchmark tests started to also fail. I have to say I'm not convinced if the situation "not one target is linkable, yet at least one target is a library (and therefor at least something should be linked)" is actually correct. All tests pass, however, including the one that checks for #3169

[rust-highfive]

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

Looks great, thanks! I would hesitate here, however, mostly due to backwards compatibility. For example today while this is guaranteed to fail for extern crate it may be used to ensure that set of dylibs are compiled, for example.

That may point to this wanting to be a warning rather than an error, but if we do that then we'd also need to provide some assurances that the warning can be suppressed as well. I'm curious, what do you think along this front?

[lukaslueg]

I wondered about that as well as the original test did not include the extern crate and passed just fine, so strictly speaking this change turns into a hard error what was previously accepted behaviour. Does it make sense to depend on a package that is_lib() but is not linkable() ? It could be acceptable behaviour to depend on something "just so it's there" without ever using it: If there is no extern crate everything is fine...

I did a quick survey of all ~71.000 crates on crates.io and siphoned the crate-type in the manifests:

 60 crate-type = ["dylib", "rlib"]
 33 crate-type = ["rlib"]
 32 crate-type = ["dylib"]
 11 crate-type = ["rlib", "dylib"]
  9 crate-type = ["rlib", "dylib", "staticlib"]
  6 crate-type = [ "dylib" ]
  6 crate-type = ["cdylib", "rlib"]
  4 crate-type = ["cdylib"]
  2 crate-type = ["staticlib"]
  2 crate-type=["rlib", "dylib"]
  2 crate-type = ["dylib"]
  1 crate-type = ["proc-macro"]
  1 crate-type = ["bin"]

So staticlib is exceedingly rare and probably a misstake in their respective first place. However I'd go with a Warning and decide wether to be able to suppress it or turn it into a hard error if the issue comes up.

[alexcrichton]

Thanks for the analysis!

Yeah I don't currently know of a use case of where you'd depend on a cdylib/staticlib via [dependencies]. That being said I've learned time and time again that just because I can't think of something doesn't mean someone else won't!

Maybe we can start with a warning (saying it'll become a hard error if you don't report an issue) and go from there?

[lukaslueg]

I've force-pushed the Warning. Notice that the test now ensures that cargo succeeds when there is no extern crate.

[lukaslueg]

failing travis seems unrelated

[alexcrichton]

@bors: r+

Thanks!

[bors]

📌 Commit 554f333 has been approved by alexcrichton

[bors]

⌛ Testing commit 554f333 with merge 314b172029108336d758516efd55362dcc3b3de0...

[bors]

💔 Test failed - status-travis

[alexcrichton]

@bors: retry

…

On Mon, Dec 11, 2017 at 6:52 AM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/cargo/builds/314751856?utm_source=github_status&utm_medium=notification> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4797 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95P0vllXffqAKoAdZESfXfpZcqKa2ks5s_SV1gaJpZM4Q8HHK> .

[bors]

⌛ Testing commit 554f333 with merge 77d44e7...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 77d44e7 to master...

[novafacing]

@alexcrichton and @lukaslueg I have a use case for which this becoming a hard error would be a blocker: fuzzing (actual use case: sanitizers). When building a fuzzer, we want to only sanitize the target, so I very frequently will make my target (the software under test) crate-type = ["staticlib"]. I'll then use a build script like this:

use std::{env::var, process::Command};

fn main() {
    let status = Command::new("cargo")
        .arg("rustc")
        .arg("-p")
        .arg("first-target")
        .arg("--target-dir")
        .arg("target/first-target/")
        .arg("--")
        .arg("-C")
        .arg(format!(
            "opt-level={}",
            var("OPT_LEVEL").expect("No OPT_LEVEL variable set")
        ))
        .arg("-C")
        .arg("prefer-dynamic")
        .arg("-C")
        .arg("passes=sancov-module")
        .arg("-C")
        .arg("llvm-args=-sanitizer-coverage-level=3")
        .arg("-C")
        .arg("llvm-args=-sanitizer-coverage-inline-8bit-counters")
        .arg("-C")
        .arg("llvm-args=-sanitizer-coverage-prune-blocks=0")
        .arg("-C")
        .arg("link-dead-code")
        .arg("-C")
        .arg("lto=no")
        .arg("--emit=dep-info,link")
        .status()
        .expect("Failed to spawn Cargo");

    assert!(status.success(), "Target build command failed");

    println!("cargo:rustc-link-lib=first_target");
    println!(
        "cargo:rustc-link-search={}/target/first-target/debug/",
        env!("CARGO_MANIFEST_DIR")
    );
}

To produce a .a staticlib from that crate (which is a dependency, because that lets me avoid having to do a bunch of explicit path nonsense), so that I can call into the coverage sanitized library like:

extern "Rust" {
    fn decode(encoded_input: &[u8]) -> Vec<u8>;
}

fn main() {
  unsafe { decode("AAAAA".as_bytes()) };
}

I beg you, please don't make this a hard error, I'm doing it on purpose!

BTW, none of these are on crates.io because I (and most other security folks) don't generally publish our fuzz targets online because nobody else really needs to use them.

[weihanglo]

@novafacing, I don't think commenting on a 5-year-old pull request would help. I'd recommend filing a new issue to raise the visibility to the current maintainers.

[novafacing]

Sure, this PR is linked in a lot of places so I figured it's what people have watched, but I'll make a new issue.