rust-lang / cargo

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

Cargo appends paths to `PATH` while building without checking if those paths already exist #14194

Open nathaniel-daniel opened 2 months ago

nathaniel-daniel commented 2 months ago

Problem

cargo appears to be appending paths to the PATH environment variable without checking if those paths are already present in it, causing unneeded rebuilds in some situations. I would expect cargo to not add to the path if it doesn't have to.

Steps

This issue can be reduced to a single file.

  1. Create a new cargo project.
  2. Enter the following in main.rs:
    
    use std::process::Command;

fn main() { for path in std::env::split_paths(&std::env::var("PATH").unwrap()) { println!("{}", path.to_string_lossy()); }

if std::env::var("EXIT").is_ok() {
    return;
}

Command::new("cargo")
    .arg("run")
    .env("EXIT", "1")
    .status()
    .unwrap();

}

3. Inspect the printed `PATH` variables. The beginning of the path of the first cargo invocation should be:

C:\Users\natha\OneDrive\Desktop\cargo-path-duplicate\target\debug\deps C:\Users\natha\OneDrive\Desktop\cargo-path-duplicate\target\debug C:\Users\natha\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\x86_64-pc-windows-msvc\lib ...rest of PATH

While the nested cargo invocation should be:

C:\Users\natha\OneDrive\Desktop\cargo-path-duplicate\target\debug\deps C:\Users\natha\OneDrive\Desktop\cargo-path-duplicate\target\debug C:\Users\natha\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\x86_64-pc-windows-msvc\lib C:\Users\natha\OneDrive\Desktop\cargo-path-duplicate\target\debug\deps C:\Users\natha\OneDrive\Desktop\cargo-path-duplicate\target\debug C:\Users\natha\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\x86_64-pc-windows-msvc\lib ...rest of PATH

As can be seen, cargo has added the same 3 paths again to the path, making crates that watch the PATH variable for changes be rebuilt.

### Possible Solution(s)

I'm not sure where cargo inserts these paths, but perhaps just avoiding adding a path if the path occurs within the first 3 paths is sufficient? Another solution could be a hashset over all paths in the `PATH`, but that seems excessive.

### Notes

I'm using an [xtask](https://github.com/matklad/cargo-xtask) setup to run `cargo build`, while running clippy through cargo directly. Switching between xtask and cargo causes a decent number of crates to be rebuilt. This is because `cc`, which a few of my crates use, emits a `cargo:rerun-if-env-changed` directive for `PATH`, which is dirty between cargo and xtask. I would expect a `cargo build` run from within xtask and a `cargo build` run outside of xtask to not need to rebuild each other's artifacts.

As an alternative, I could just add a new command to my xtask setup for clippy. However, I would prefer if I didn't have to wrap each cargo build-like command and `cargo install`-ed subcommand that I want to use.

### Version

```text
cargo 1.79.0 (ffa9cf99a 2024-06-03)
release: 1.79.0
commit-hash: ffa9cf99a594e59032757403d4c780b46dc2c43a
commit-date: 2024-06-03
host: x86_64-pc-windows-msvc
libgit2: 1.7.2 (sys:0.18.3 vendored)
libcurl: 8.6.0-DEV (sys:0.4.72+curl-8.6.0 vendored ssl:Schannel)
os: Windows 10.0.22631 (Windows 11 Professional) [64-bit]
weihanglo commented 2 months ago

Thanks for the report.

By looking at the duplicate env vars, I am guessing that this is a Windows specific issue. Cargo prepend paths to dylib search paths to ensure its own executable to be searched first. On Windows unfortunately the environment variable of dylib search pats is PATH. And that is why.

https://github.com/rust-lang/cargo/blob/c1fe2bdf40cc222408ee117930d5289f6b893551/src/cargo/core/compiler/compilation.rs#L337-L339

perhaps just avoiding adding a path if the path occurs within the first 3 paths is sufficient?

Checking if PATH contains our paths-to-prepend in the desired order is sufficient I believe.

weihanglo commented 2 months ago

Hmm… not really Windows specific. If a build script checks LD_LIBRARY_PATH in rerun-if-env-changed, same thing happens on Unix.

weihanglo commented 2 months ago

BTW, for recursive call into cargo, the recommended way is using std::env!("CARGO") so that you don't need to re-resolve the rustup proxy again. Cargo set CARGO env when building each crate.

linyihai commented 1 week ago

FYI, I tried to deduplicate the path in https://github.com/rust-lang/cargo/pull/14464/commits/b7c18805e6a96ef89eeb4c1344ec4b855a683ff8, but it still recomplie.

(The commit can pass on linux platform since it hard the path output)