Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nathaniel-daniel opened this issue Jul 5, 2024 · 4 comments · May be fixed by #14464
Open
Labels
A-environment-variables Area: environment variables A-rebuild-detection Area: rebuild detection and fingerprinting C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@nathaniel-daniel
Copy link

nathaniel-daniel commented Jul 5, 2024

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();
}
  1. 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]
@nathaniel-daniel nathaniel-daniel added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Jul 5, 2024
@weihanglo
Copy link
Member

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.

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
Copy link
Member

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

@weihanglo
Copy link
Member

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.

@weihanglo weihanglo added A-rebuild-detection Area: rebuild detection and fingerprinting A-environment-variables Area: environment variables S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-triage Status: This issue is waiting on initial triage. labels Jul 5, 2024
@linyihai
Copy link
Contributor

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-environment-variables Area: environment variables A-rebuild-detection Area: rebuild detection and fingerprinting C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
3 participants