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 should not prepend $HOME/.cargo/bin to PATH for subcommands when it's already in PATH, to avoid unnecessary recompilations #7431

Closed
Boscop opened this issue Sep 25, 2019 · 6 comments · Fixed by rust-lang/rustup#2978
Labels
A-build-scripts Area: build.rs scripts A-environment-variables Area: environment variables C-bug Category: bug

Comments

@Boscop
Copy link

Boscop commented Sep 25, 2019

cargo is prepending $HOME/.cargo/bin/%HOME%\.cargo\bin to PATH, even if PATH already contains this dir.
Cargo should only prepend it if it's not already in PATH!
Currently this behavior causes unnecessarily recompilations when any dependency uses rerun-if-env-changed=PATH, such as libsqlite3-sys.
(I have to wait several minutes every time when switching between cargo check in my editor and having cargo watch -x run running in cmd.exe, because cargo passes an unnecessarily modified PATH to cargo-watch, even though my PATH already contains %HOME%\.cargo\bin.)

@jonhoo
Copy link
Contributor

jonhoo commented Apr 14, 2022

I've been doing some digging, and I can't actually find where this is added. From what I can tell, nothing in Cargo sets PATH. The closest I can find is

cargo/src/bin/cargo/main.rs

Lines 206 to 210 in 65c8266

fn search_directories(config: &Config) -> Vec<PathBuf> {
let mut dirs = vec![config.home().clone().into_path_unlocked().join("bin")];
if let Some(val) = env::var_os("PATH") {
dirs.extend(env::split_paths(&val));
}

which does search $CARGO_HOME/bin first and then the rest of PATH, but it only ever seems to be iterated over and not actually used to set anything that would affect PATH.

Instead, I wonder if what's going on is that something in the call hierarchy is invoking sh or bash, which in turn sources ~/.profile, which tends to contain

export PATH="$HOME/.cargo/bin:$PATH"

or

source "$HOME/.cargo/env"

which in turn contains

#!/bin/sh
# rustup shell setup
# affix colons on either side of $PATH to simplify matching
case ":${PATH}:" in
  *:"$HOME/.cargo/bin":*)
    ;;
  *)
        # Prepending path in case a system-installed rustc needs to be overridden
        export PATH="$HOME/.cargo/bin:$PATH"
        ;;
esac

And that's what ends up adding to $PATH, not Cargo itself. In your linked example for instance, the subcommand has #!/usr/bin/sh, which will end up round-tripping through the configuration above.

@jonhoo
Copy link
Contributor

jonhoo commented Apr 14, 2022

Hmm, no, that doesn't seem right. Even when commenting out all those, and using a subcommand to print the env implemented purely in Rust, I still see PATH being prepended to 🤔

@jonhoo
Copy link
Contributor

jonhoo commented Apr 14, 2022

@jonhoo
Copy link
Contributor

jonhoo commented Apr 14, 2022

See also rust-lang/rustup#2922 and rust-lang/rustup#2848.

jonhoo pushed a commit to jonhoo/rustup.rs that referenced this issue Apr 14, 2022
The current logic forces nested invocations to execute `cargo` and
friends from `$CARGO_HOME/bin`. This makes rustup break in environments
where the appropriate rustup proxies to use happen to be installed
elsewhere (earlier on `$PATH`), and using the binaries in
`$CARGO_HOME/bin` would not work correctly.

It also ensures that Rustup won't change `$PATH` "just for the heck of
it", which _should_ help reduce unnecessary re-compilations when
downstream build logic notices that `$PATH` changes (since it will no
longer).

Helps with rust-lang#2848.

Fixes rust-lang/cargo#7431.
@jonhoo
Copy link
Contributor

jonhoo commented Apr 14, 2022

Filed a fix for this in rust-lang/rustup#2978.

@weihanglo
Copy link
Member

Closed as rustup patch was released in 1.25.0. Thank you all involving in this issue.

Darunada pushed a commit to Darunada/rustup that referenced this issue Feb 25, 2023
The current logic forces nested invocations to execute `cargo` and
friends from `$CARGO_HOME/bin`. This makes rustup break in environments
where the appropriate rustup proxies to use happen to be installed
elsewhere (earlier on `$PATH`), and using the binaries in
`$CARGO_HOME/bin` would not work correctly.

It also ensures that Rustup won't change `$PATH` "just for the heck of
it", which _should_ help reduce unnecessary re-compilations when
downstream build logic notices that `$PATH` changes (since it will no
longer).

Helps with rust-lang#2848.

Fixes rust-lang/cargo#7431.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts A-environment-variables Area: environment variables C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants