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

[nathaniel-daniel]

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 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

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]

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.

cargo/src/cargo/core/compiler/compilation.rs

Lines 337 to 339 in c1fe2bd

	let search_path = paths::join_paths(&search_path, paths::dylib_path_envvar())?;
	cmd.env(paths::dylib_path_envvar(), &search_path);

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]

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

[weihanglo]

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]

FYI, I tried to deduplicate the path in b7c1880, but it still recomplie.

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

[weihanglo]

#14464 fixes only half of the issue (don't append already-appended env vars), the other half shares a similar bug with #10358 (see #14464 (comment) for details).