Do not add home bin path to PATH if it's already there

[dpc]

This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different cargo-foo scripts in $HOME/.cargo/bin and $HOME/bin:

> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/

and trailing slash:

> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/

Fix #11020

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) soon.

Please see the contribution instructions for more information.

[epage]

@rfcbot merge

I feel like this solves the problem in #11020 in a reasonable way though wanted to check-in with everyone due to the slight behavior change (we'll respect CARGO_HOME/.bin's precedence if its in the path) and the potential for brittleness (no normalization is done).

[rfcbot]

Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

Concerns:

• code-review resolved by Do not add home bin path to PATH if it's already there #11023 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[ehuss]

I'm a bit confused about the status and history here, can someone clarify?

It looks like @jonhoo tried to fix something almost identical in rust-lang/rustup#2978 (for a slightly different use case). @weihanglo closed #7431, which I think is a duplicate of #11020.

Is it correct that those two issues are duplicates? Was Jon's use case just different enough that this wasn't an issue on the cargo side?

I can't immediately think of when this will cause a particular issue, though it seems somewhat risky.

[dpc]

It looks like @jonhoo tried to fix something almost identical in rust-lang/rustup#2978 (for a slightly different use case). @weihanglo closed #7431, which I think is a duplicate of #11020.

I might be missing something but they look somewhat similar in nature. rust-lang/rustup#2978 is about PATH being modified needlessly by adding .cargo/bin to PATH even when it's already there, while this one is about how cargo needlessly prefers binaries from .cargo/bin even when .cargo/bin is already in the PATH.

[weihanglo]

Would it worth to have a doc about subcommand searching algorithm in cargo user guide here? This can be a separate pull request though.

I can't immediately think of when this will cause a particular issue, though it seems somewhat risky.

One risk (?) is that some Linux distros provide cargo subcommand via their own repositories, but those packages usually get outdated. User with env such as PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" might execute the outdated one provided by distros.

[dpc]

User with env such as PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" might execute the outdated one provided by distros.

If they've included $HOME/.cargo/bin in their PATH after system directories they've made their choice explicitly, even if by mistake.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[epage]

Like with #10807, I feel like there isn't any harm in ending the FCP early as this was to get a straw poll of the team.

All blockers have been resolved and the only remaining conversation is more on the nit level, has no user impact for us to change later, and two experienced developers can reasonably come to different opinions on, so it doesn't seem worth blocking forward progress on it.

@bors r+

Thanks @dpc for working with us through this process!

[bors]

📌 Commit c712f08 has been approved by epage

It is now in the queue for this repository.

[bors]

⌛ Testing commit c712f08 with merge c187114...

[bors]

☀️ Test successful - checks-actions
Approved by: epage
Pushing c187114 to master...

[tiagolobocastro]

How will this work with pure shells? In this case it's actually the opposite, where the user PATH is removed, so the user cargo bin will not be in the PATH and yet cargo will attempt to use it, unless I misunderstand?

       • --pure
         If this flag is specified, the environment is almost entirely cleared before the interactive shell is started, so you get an environment that more closely corresponds to the “real” Nix build. 
         A few variables, in  particular HOME, USER and DISPLAY, are retained.

[dpc]

How will this work with pure shells? In this case it's actually the opposite, where the user PATH is removed, so the user cargo bin will not be in the PATH and yet cargo will attempt to use it, unless I misunderstand?

Yes. The outside PATH will be wiped, so cargo will use the normal behavior of using binaries from $HOME/.cargo/bin.

Usually when using pure shells, something inside the shell (like shellHook in flake.nix is responsible for reproducibly setting up PATH. My plan is to just add $HOME/.cargo/bin there (at the end) from there.

[tiagolobocastro]

My plan is to just add $HOME/.cargo/bin there (at the end) from there.

That also kind "breaks" the purity of the shell because if a binary does not exist in the nix-shell environment, now you'd be pulling out of $HOME/.cargo/bin which is what the pure is meant to avoid?

[weihanglo]

now you'd be pulling out of $HOME/.cargo/bin which is what the pure is meant to avoid?

As I understand it, this is sadly the old Cargo behaviour existing for a while. It's not a new feature introduced by this PR. Conversely, this PR tries to relax the behaviour a bit. Removing it (search in default cargo home bins dir) results in a breaking change.

(I am not a nix user and may be wrong about nix)

[tiagolobocastro]

No I don't think this PR is breaking things - I'm simply suggesting we haven't solved the "issue" for nix-shell in a general manner, but rather for a uses case where the cargo home bin will be in the path which is why nix-shell --pure tries to avoid in the first place.

Don't know if this was discussed before, but what about a new env variable that basically tells cargo not to search for binaries in the cargo home, unless it's in the path?
Granted it's not a nice solution but we only need it because we can't break existing "bad" behaviour IMHO.
What do you think @epage ?

[dpc]

That also kind "breaks" the purity of the shell because if a binary does not exist in the nix-shell environment, now you'd be pulling out of $HOME/.cargo/bin which is what the pure is meant to avoid?

That's why for full purity Nix sets HOME=/homeless-shelter. Pretty much all Unix software will refer to their config files under $HOME which can change their behavior breaking purity.

Full purity is often undesirable for my use case - when the developers are working on a project in nix develop shell, I do want them to have access to their local cargo-installed tools, just like I want them to be able to launch their favorite shell, text editor and IDE and other tools.

The scripts and tools inside dev shell should not call binaries the shell does not already provide. So the only requirement that I see is that script in cargo home bin do not take precedence over the tools provided by the shell (in the $PATH).

Everything will get checked again in CI anyway, so purity does not have to be perfect locally.

[tiagolobocastro]

Full purity is often undesirable for my use case - when the developers are working on a project in nix develop shell, I do want them to have access to their local cargo-installed tools, just like I want them to be able to launch their favorite shell, text editor and IDE and other tools.

That's reasonable, we also work like that most of the time, that's why there's the --pure flag which is not mandatory.
However, when we want a "pure" environment for whatever reason, we don't want to add unnecessary things to the path which might end up being picked up inadvertently.