Wrong PATH override on Windows

[sinkuu]

EDIT: See #7475 (comment) for better description. Thank you @ehuss!

Problem

This is a regression introduced in the recent cargo update in rust repo.

On windows, within test processes run by cargo test, PATH variable now looks like:

C:\projects\rust-clippy\clippy_dev\target\debug\deps;C:\projects\rust-clippy\clippy_dev\target\debug;C:\Users\appveyor\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\bin;C:\Users\appveyor\.cargo\bin;C:\Users\appveyor\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\bin;.....

The first three paths are added by cargo. Note that nightly-x86_64-pc-windows-msvc\bin is now preferred than .cargo\bin.

In clippy CI, cargo +nightly fmt is invoked during integration test. This have been worked until the recent update. Today, the plain cargo binary from nightly-x86_64-pc-windows-msvc\bin is run instead of the rustup's one in .cargo\bin that supports +toolchain_name argument.

Steps

See recent failed builds in https://ci.appveyor.com/project/rust-lang-libs/rust-clippy

Clippy's CI involves two versions of toolchains: nightly and master.

1. cargo test (master) runs tests/fmt.rs integration test.
2. tests/fmt.rs invokes cargo +nightly run on clippy_dev. This succeeds because master does not have cargo. Now we are in nightly environment.
3. clippy_dev invokes cargo +nightly fmt. This fails because nightly has cargo.

Possible Solution(s)

.cargo\bin should always be preferred (come first in PATH variable).

Notes

Output of cargo version:

cargo 1.40.0-nightly (8b0561d68 2019-09-30)

[matthiaskrgr]

It seems the bug is present since the latest update of cargo in the rustc repo:
rust-lang/rust@e1d9f82

[Eh2406]

Is there some cargo test we can add to make sure this doesn't happen again? If so we can use it to bisec to find the what broke it.

[mati865]

It's reproducible on Windows (both msvc and gnu) by running cargo +nightly-2019-10-02 test on rust-lang/rust-clippy@737f0a6

It's far from being minimal though.

[ehuss]

The issue is due to #7425, though I'm not yet sure I understand it fully.

[ehuss]

@alexcrichton I'm running out of energy for today, but I'll try to distill what is happening and see if you have some ideas.

Problem: Due to #7425, on Windows, cargo test now sets the PATH with …\toolchains\$HOST\bin. Previously it would be …\toolchains\$HOST\lib\rustlib\$HOST\lib. This causes a problem for anyone running commands like cargo +nightly from within a test because the bin path is the actual cargo, not the rustup one in ~/.cargo/bin. I also imagine there might be issues with dylib linking to target dylibs, but I'm not sure how to trigger that. I believe the old behavior is more correct, although the difference is subtle.

Cause: Previously, BuildContext::target_info would be populated as-if --target $HOST was set. Now, BuildContext::target_info is an empty HashMap. The result is that Compilation::target_dylib_path is set to the host dylib path (…/bin). Tests use target_process and this dylib path is populated here.

Possible solution? I was thinking maybe BuildContext could populate target_info with a $HOST entry if --target is not specified. Compilation::target_dylib_path would need to use this host entry. It feels kinda messy, so there's probably a more elegant solution.

[alexcrichton]

Thanks for the investigation @ehuss, that sounds spot-on to me! I've posted #7482 which has the same shape of solution