Cargo set environment variables provided by build scripts randomly when running tests

Discovered in rust-lang/miri#1661 (comment) by @RalfJung.

Steps

1. Setup a workspace like this:

cargo new --lib cargo-bug --vcs none
cd cargo-bug/
echo 'proc-macro-crate = { path = "proc-macro-crate" }
[lib]
doctest = false
[workspace]' >> Cargo.toml
cargo new --lib proc-macro-crate --vcs none
cd proc-macro-crate/
echo '[lib]
proc-macro = true
[dev-dependencies]
cargo-bug = { path = ".." }' >> Cargo.toml
cd ..
echo 'fn main() { println!("cargo:rustc-env=CRATE_TARGET={}", std::env::var("TARGET").unwrap()) }' > build.rs
echo '#[test]
fn target() {
    assert_eq!(env!("CRATE_TARGET"), "i686-unknown-linux-musl");
    assert_eq!(std::env::var_os("CRATE_TARGET"), None);
}' > src/lib.rs

2. Run cargo clean && cargo test --workspace --target i686-unknown-linux-musl -vv (with --jobs 1 optionally).
3. The test always fails with (the random) one of the following messages (on x86_64-unknown-linux-gnu):

thread 'target' panicked at 'assertion failed: `(left == right)`
  left: `Some("i686-unknown-linux-musl")`,
 right: `None`', src/lib.rs:4:5

thread 'target' panicked at 'assertion failed: `(left == right)`
  left: `Some("x86_64-unknown-linux-gnu")`,
 right: `None`', src/lib.rs:4:5

Version info

cargo 1.51.0-nightly (783bc43c6 2021-01-20)
release: 1.51.0
commit-hash: 783bc43c660bf39c1e562c8c429b32078ad3099b
commit-date: 2021-01-20

cc @ehuss

[ehuss]

The environment variable differing between host and target is working as expected. (EDIT, was wrong, see below.) The build script is being run twice, once for host and once for target (in parallel, so that's what it seems inconsistent, there's a race which one runs first). The reason it does it twice is that proc-macro tests always run on the host. With --workspace it is also running the cargo_bug tests, which run on target. So it needs to do both.

As for the environment variables being set during cargo test and cargo run, that's a surprise to me. It looks like it has behaved that way ever since it was introduced in #3929.

@alexcrichton To clarify, the issue is that cargo:rustc-env in a build script will set that environment variable not only when compiling with rustc, but also when running the process with cargo run or cargo test. That's a surprise to me, and I was wondering if that was intended. If not, do you think it would be feasible to change that? I fear there are probably tests which rely on that, in which case should we just update the documentation? Should the documentation discourage doing that?

[ehuss]

Hm, I take back what I said. It looks like it is randomly picking environment variables. The issue is that these lines do not differentiate based on the correct parameters. I'll try to take a look at that soonish.

[RalfJung]

As for the environment variables being set during cargo test and cargo run, that's a surprise to me. It looks like it has behaved that way ever since it was introduced in #3929.

The thing is (as you noted in your last comment) -- cargo is inconsistent wrt. which env vars it sets during test+run; sometimes it's the host build script, sometimes the target build script.

The issue is that these lines do not differentiate based on the correct parameters.

Yeah, looks like all build script env vars are put into that HashMap and then it's a game of chance which one "wins" during test/`run?

[alexcrichton]

Hm that's probably a bug that it's set at runtime for the test in addition to compile-time for the test executable. But yeah I agree with @ehuss's assessment that we probably can't back this out now and the bug is around those lines since the targete-process-execution is much less precise than the build itself.