search

rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.61k stars 2.4k forks source link

Cargo wrongly links `std` to `no_std` crate with conditional dependency on `std`, even when that dependency is not selected #6571

Closed cbeck88 closed 4 years ago

cbeck88 commented 5 years ago

Problem

In the minimal test case (https://github.com/cbeck88/rust-cargo-bug), a library is marked no_std and which has no dependency on std is nevertheless linked to std by cargo, wrongly. This breaks the build, because it conflicts with provided lang items.

Yet, when a conditional dependency which is not even selected or built by cargo is commented out in Cargo.toml, cargo stops linking std to the first target, and the build succeeds.

This indicates that the logic that determines whether a crate transitively depends on std is broken and doesn't take into account conditional dependencies properly.

Steps

To reproduce the bug, use the minimal verifiable example provided here: https://github.com/cbeck88/rust-cargo-bug

  1. clone the repository
  2. cargo build, or cargo build --target=x86_64-unknown-linux-gnu if you are not on an x86_64 machine. Observe build failure
  3. git apply the patch, which comments out conditional dependency on rand
  4. Build again and observe that it succeeds.

Possible Solution(s)

I suspect that the logic that determines whether std is needed, ignores, or doesn't properly take into account, whether or not a conditional dependency was actually selected.

Notes

Output of cargo version:

Using stable cargo and rust on Ubuntu 18.04:

$ cargo --version
cargo 1.30.0
$ rustc --version
rustc 1.30.0
dwijnand commented 5 years ago

Duplicate of #4361

and if it's not exactly #4361, then it's probably one of:

cbeck88 commented 5 years ago

@dwijnand I don't see that any of those tickets has to do with my issue -- they are all about features, and there are no cfg features in my example, I am using cfg of target-arch

This issue specifically has to do with this syntax:

[target.'cfg(not(target_arch = "x86_64"))'.dependencies]
rand = { version = "0.6" }

Cargo is giving me std on x86_64 arch because these two lines are present, which doesn't make any sense, and when we comment them out, it stops doing that.

This has nothing to do with default features, or features being unioned across the build plan. rand is not a dependency of my crate on x86_64, nevertheless std gets linked in when I build. That's the bug.

dwijnand commented 5 years ago

Let me premise by saying we can reopen if I'm wrong, but I think that's already covered by these comments in those issues:

cbeck88 commented 5 years ago

So, I'm skeptical that this is really the same issue, because when we change it from using target_arch in the cfg expressions to simply adding a feature, and making rand an optional dependency gated on that feature, it simply works, in our actual project in tree.

I can try to add such a demonstration in the minimal verifiable example.

This suggests that there is more to the story than e.g. @alexcrichton 's response to this comment: https://github.com/rust-lang/cargo/issues/2589#issuecomment-224697180

When we build the resolution graph today it contains information about all targets, and the filtering per platform only happens at the very end when we're compiling.

otherwise why doesn't the same issue occur when we branch on features instead of target_arch?

Contrary to what is written there in 2016, It seems that cargo is successfully able to avoid building rand in x86_64, AND when using the cfg(not(target_arch = ...)) expression. The discrepancy in my example is specific to std and not any other crate.

dwijnand commented 5 years ago

A minimal verificable example would be lovely, thank you.

cbeck88 commented 5 years ago

@dwijnand done, see here: https://github.com/cbeck88/rust-cargo-bug#distinguishing-this-issue-from-cargo-issue-2589-httpsgithubcomrust-langcargoissues2589issuecomment-224697180

lijinpei commented 5 years ago

A little bit of digging into it shows the problem seems to be cargo decided to pass --cfg 'feature="std"' when compiling rand_core, see the following different calls of rustc(you can generate then with cargo build -vv)

LD_LIBRARY_PATH='/C/Development/rust-cargo-bug/target/debug/deps:/home/lijinpei/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib:/home/lijinpei/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib' CARGO_PKG_NAME=rand_core CARGO_PKG_DESCRIPTION='Core random number generator traits and tools for implementation. ' CARGO_PKG_VERSION_MINOR=4 CARGO_PKG_HOMEPAGE='https://crates.io/crates/rand_core' CARGO=/home/lijinpei/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo CARGO_PKG_AUTHORS='The Rand Project Developers:The Rust Project Developers' CARGO_MANIFEST_DIR=/home/lijinpei/.cargo/registry/src/github.com-1ecc6299db9ec823/rand_core-0.4.0 CARGO_PKG_VERSION_MAJOR=0 CARGO_PKG_VERSION_PATCH=0 CARGO_PKG_VERSION_PRE= CARGO_PKG_VERSION=0.4.0 CARGO_PKG_REPOSITORY='https://github.com/rust-random/rand' rustc --crate-name rand_core /home/lijinpei/.cargo/registry/src/github.com-1ecc6299db9ec823/rand_core-0.4.0/src/lib.rs --color always --crate-type lib --emit=dep-info,link -C debuginfo=2 -C metadata=afbd3cacdc17ebd3 -C extra-filename=-afbd3cacdc17ebd3 --out-dir /C/Development/rust-cargo-bug/target/debug/deps -L dependency=/C/Development/rust-cargo-bug/target/debug/deps --cap-lints warn

LD_LIBRARY_PATH='/C/Development/rust-cargo-bug/target/debug/deps:/home/lijinpei/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib:/home/lijinpei/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib' CARGO_PKG_DESCRIPTION='Core random number generator traits and tools for implementation. ' CARGO_PKG_AUTHORS='The Rand Project Developers:The Rust Project Developers' CARGO_PKG_NAME=rand_core CARGO_PKG_VERSION_MINOR=4 CARGO_MANIFEST_DIR=/home/lijinpei/.cargo/registry/src/github.com-1ecc6299db9ec823/rand_core-0.4.0 CARGO_PKG_VERSION=0.4.0 CARGO_PKG_REPOSITORY='https://github.com/rust-random/rand' CARGO_PKG_VERSION_PATCH=0 CARGO_PKG_HOMEPAGE='https://crates.io/crates/rand_core' CARGO_PKG_VERSION_PRE= CARGO=/home/lijinpei/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo CARGO_PKG_VERSION_MAJOR=0 rustc --crate-name rand_core /home/lijinpei/.cargo/registry/src/github.com-1ecc6299db9ec823/rand_core-0.4.0/src/lib.rs --color always --crate-type lib --emit=dep-info,link -C debuginfo=2 --cfg 'feature="alloc"' --cfg 'feature="std"' -C metadata=fac15169af3958da -C extra-filename=-fac15169af3958da --out-dir /C/Development/rust-cargo-bug/target/debug/deps -L dependency=/C/Development/rust-cargo-bug/target/debug/deps --cap-lints warn

(the first one is the correct one).

lijinpei commented 5 years ago

I think the problem is, when cargo computes Resolve(a data-structure contains which package are dependent by which, and which version and features), it treat take targets-specific dependencies as they are always needed. So rand's mere appearance (even through not later linked in) cause it's std feature, and therefore rand_core's std feature to be included, which cause problem when rand_core is linked with myenslave. For example, when the crate doesn't compiles, Resolve contains the followings: pkg "rand_core" version 0.3.1 features: pkg "rand_isaac" version 0.1.1 features: pkg "cloudabi" version 0.0.3 features: bitflags default pkg "rand_os" version 0.1.2 features: pkg "rand_chacha" version 0.1.1 features: pkg "winapi-x86_64-pc-windows-gnu" version 0.4.0 features: pkg "myenclave" version 1.0.0 features: pkg "semver" version 0.9.0 features: default pkg "autocfg" version 0.1.2 features: pkg "rdrand" version 0.4.0 features: std default pkg "winapi" version 0.3.6 features: ntsecapi minwindef profileapi winnt pkg "winapi-i686-pc-windows-gnu" version 0.4.0 features: pkg "semver-parser" version 0.7.0 features: pkg "fuchsia-cprng" version 0.1.1 features: pkg "rand_jitter" version 0.1.3 features: std rand_core pkg "bitflags" version 1.0.4 features: default pkg "rand" version 0.6.5 features: std alloc default rand_jitter rand_core rand_os pkg "mcrand" version 1.0.0 features: pkg "libc" version 0.2.48 features: use_std default pkg "rand_pcg" version 0.1.1 features: pkg "rand_hc" version 0.1.0 features: pkg "rustc_version" version 0.2.3 features: pkg "rand_core" version 0.4.0 features: alloc std pkg "rand_xorshift" version 0.1.1 features: If we write the following in Cargo.toml: [target.'cfg(not(target_arch = "x86_64"))'.dependencies] rand = { version = "0.6", default-features = false } then Resolve would be: pkg "winapi" version 0.3.6 features: profileapi minwindef winnt ntsecapi pkg "rand_xorshift" version 0.1.1 features: pkg "rand_pcg" version 0.1.1 features: pkg "rand_isaac" version 0.1.1 features: pkg "winapi-i686-pc-windows-gnu" version 0.4.0 features: pkg "rand_hc" version 0.1.0 features: pkg "libc" version 0.2.48 features: pkg "rand_core" version 0.4.0 features: pkg "rand_jitter" version 0.1.3 features: pkg "mcrand" version 1.0.0 features: pkg "rand_chacha" version 0.1.1 features: pkg "semver-parser" version 0.7.0 features: pkg "rustc_version" version 0.2.3 features: pkg "rand_core" version 0.3.1 features: pkg "winapi-x86_64-pc-windows-gnu" version 0.4.0 features: pkg "rand" version 0.6.5 features: pkg "semver" version 0.9.0 features: default pkg "autocfg" version 0.1.2 features: pkg "myenclave" version 1.0.0 features:

and the project compiles.

cbeck88 commented 5 years ago

@lijinpei thank you for digging into this!

So my takeaway from this is that making something optional is somehow not as strong as making it `cfg(target_arch(...))`` selected -- that's confusing to me but if you think this behavior is reliable it's really helpful!

Do you think that this a "bug" or a design decision of the cargo team? (Or simply not clear which?)

lijinpei commented 5 years ago

First, theoretically, whether core_rand's std feature enabled or not, it meets all the dependency requirements. A requirements problem may be satisfied by multiple version of a set of packages, or multiple sets of feature of packages, I don't know if cargo guarantees to return a specific set.

Secondly, currently, when computing dependencies, cargo treat all platform-specific dependencies as if they are always needed, it's only when the actual build starts the platform-specifics are filtered out. I personally thought this is a bug. But that needs to be cleared be cargo team. (I can see one reason for cargo to work this way is it needs to support cross-build while share the same Cargo.lock. But maybe constraints for different platforms may not be satisfied together?)

(A third problem would be features like std needs consensus between rustc and cargo, but cargo won't look into a .rs file's contents, it even doesn't know which set of files are needed to build a crate without rustc's help, while rustc doesn't know package dependencies, it needs cargo to provides them on the command line)

RalfJung commented 5 years ago

Might be related to https://github.com/rust-lang/cargo/issues/2524?

cbeck88 commented 5 years ago

@lijinpei in any other build system like, scons, cmake, make, if unnecessary stuff is being built, that is a bug. All of these other systems are highly configurable so that you can make it build exactly what you need with exactly the flags you need.

It's crazy that in cargo you can have bugs where the wrong stuff is built, it takes hours to track down the reason, and then there's no fix, or maybe a hack requiring changes to everything, and the cargo devs question whether it's a bug! Building unnecessary stuff can mean slow builds or broken builds. Bugs like this in cargo (and there are many of them!), and the difficulty of debugging them, are a serious reason not to use rust at all, esp. in embedded development. In our company we have worked around it with a hack but it's pretty dirty.

You can choose not to define it as a bug, but this just means you have set the bar for cargo way below other similar tools.

I can see one reason for cargo to work this way is it needs to support cross-build while share the same Cargo.lock

I don't see a reason for this constraint, any more than it would make sense to require Cargo.lock to be the same no matter what features are enabled.

ehuss commented 4 years ago

I'm going to close this as I believe it is resolved by #7914.