Skip to content
New issue

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

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

Already on GitHub? # to your account

Can't provide custom subcommand in $PATH if it also exists in $CARGO_HOME/bin #6507

Closed
TimNN opened this issue Jan 1, 2019 · 9 comments
Closed
Labels
A-custom-subcommands Area: custom 3rd party subcommand plugins C-bug Category: bug S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@TimNN
Copy link
Contributor

TimNN commented Jan 1, 2019

Problem

Cargo will always search $CARGO_HOME/bin first for custom subcommands. This means that testing subcommands (e.g. using PATH="$PWD/target/debug:$PATH" cargo my-command) won't work if cargo-my-command is also installed in $CARGO_HOME/bin.

Steps

# Step 1: Prepare a dummy command on `$PATH`.
mkdir -p "cargo-foo-bin"
echo -e '#!/bin/bash\necho "From Path."' > "cargo-foo-bin/cargo-foo"
chmod +x "cargo-foo-bin/cargo-foo"

# Step 2: Run the dummy command. It is correctly found in `$PATH`.
PATH="$PWD/cargo-foo-bin:$PATH" cargo foo

# Step 3: Prepare the same command in `$CARGO_HOME/bin`.
echo -e '#!/bin/bash\necho "Installed."'> "${CARGO_HOME:-$HOME/.cargo}/bin/cargo-foo"
chmod +x "${CARGO_HOME:-$HOME/.cargo}/bin/cargo-foo"

# Step 4: Run the dummy command. The version from `$CARGO_HOME/bin` is used.
PATH="$PWD/cargo-foo-bin:$PATH" cargo foo

Possible Solution(s)

  • Use $CARGO_HOME/bin as a fallback location rather than the primary location [breaking-change].
  • Allow overriding subcommand binaries with environment variables, eg. $CARGO_MY_COMMAND=... [breaking-change].

Notes

Output of cargo version: cargo 1.31.0 (339d9f9c8 2018-11-16)

Behavior introduced in #2192.

@TimNN TimNN added the C-bug Category: bug label Jan 1, 2019
@dwijnand
Copy link
Member

dwijnand commented Jan 1, 2019

  • Allow overriding subcommand binaries with environment variables, eg. $CARGO_MY_COMMAND=... [breaking-change].

Why would this be a breaking change?

@TimNN
Copy link
Contributor Author

TimNN commented Jan 1, 2019

Why would this be a breaking change?

Because, technically, someone could currently have an environment variable like $CARGO_MY_COMMAND (maybe for a totally unrelated purpose) and Cargo starting to use it could break things for them.

Although, to be honest, I don't think that any of the two proposed solutions are likely to cause real breakage. Although I would consider the chance of breakage a bit higher for the first solution.

@dwijnand
Copy link
Member

dwijnand commented Jan 1, 2019

That sounds like https://xkcd.com/1172 (although xkcd, being satirical, takes the example to the max).

As a large aside, conversations around what's a "breaking change" or not or "feature vs bug", which is relevant in things like semver, continue to fascinate me because it's never crystal clear. It's particularly ironic in the issue/PR tracker of semver, where contributors can't tell if a change to the semver spec is breaking or not 😁...

Back on topic, the first one seems to be idea, but I agree poses the bigger risk. From the original issue #2189 and the fixing PR #2192 it doesn't sound like this behaviour was intentional, so it would be lovely to fix it in the more straightforward manner. If it's too risky we can go with the slightly more laborious other options like $CARGO_MY_COMMAND or some other "I'm developing cargo-foo here" override.

@CobaltCause
Copy link

I like the "just search $CARGO_HOME/bin last" option the best since it doesn't require any extra work from users to get it to behave in the expected manner. Though, it does have the potential downside of changing the behavior in general, which may be relied upon. I think that searching there last instead of first is much less likely to break users than not searching there at all (unless in $PATH). If we still don't want to change the default behavior without warning for fear of breaking configurations, then I have some other ideas, in order of personal preference:

Idea 1

Introduce a configuration option to control this behavior when desired, like an environment variable named CARGO_HOME_BIN_SEARCH with a few options:

  1. first: Search $CARGO_HOME/bin before $PATH (current behavior)
  2. off: Don't add $CARGO_HOME/bin to cargo's internal search path at all
  3. last: Search $CARGO_HOME/bin after $PATH

Then we'd pick a default: probably first, in the interest of not breaking things. We could optionally transition the default later on by giving warnings and suggesting to use the default-to-be, I've seen some other tools do this.

Idea 2

Do both, trying idea 3 first and falling back to idea 1.

Idea 3

Introduce a CARGO_SUBCMD_* group of environment variables where * would be the name of the subcommand as it's invoked by cargo (e.g. CARGO_SUBCMD_foo for cargo foo). This would allow finer control than relying on $PATH. For example, if things are added to $PATH in an order you don't control (e.g. Nix buildInputs) and there is a cargo-foo in multiple entries, this would let you reliably choose the right cargo-foo to run. It also (kind of) has precedence in the RUSTFMT, RUSTC, and RUSTDOC environment variables that Cargo already reads.


Personally, I ran into this issue while working on a project whose environment is controlled by nix and direnv. If other developers or CI platforms happen to have a $CARGO_HOME/bin/cargo-fmt (for example), then running cargo fmt as usual will, currently, pick the wrong binary, because the rustfmt I actually want to use ends up in the /nix/store in $PATH somewhere. I discovered that there is an environment variable that Cargo reads for rustfmt specifically and a couple other things, but this does not generalize to custom tools or even clippy. I could change CARGO_HOME but that would sacrifice the cache, so I'm not a fan of that solution.

I haven't contributed to Cargo before but this seems simple enough; I'm willing to write a PR for whichever solution to this is desired.

@MarinPostma
Copy link

MarinPostma commented Jul 4, 2022

I am hitting the same issue. It seems that the solution proposed in #2189 would have worked as well by just appending the cargo root to the path rather than pretending it, since it seems that the issue was with some users not having the $CARGO/bin set in their path. A sensible solution now could be to prepend $CARGO/bin iif it is not already present in the PATH.

If that is an acceptable solution, I have a patch ready.

@mati865
Copy link
Contributor

mati865 commented Sep 12, 2023

Is there a reason why Cargo should not look for binaries alongside it first?

So in case we have cargo-clippy binaries in three places:

  • $CARGO_HOME
  • $PATH
  • alongside cargo

It would default to std::env::current_exe() and replace the result with requested binary, then probe $CARGO_HOME and finally $PATH.

@epage epage added the S-needs-team-input Status: Needs input from team on whether/how to proceed. label Oct 30, 2023
@epage
Copy link
Contributor

epage commented Oct 30, 2023

#11020 is quite nearly a duplicate of this. Closing in favor of that.

The solution used for closing that was by not checking ~/.cargo/bin if its already in PATH, giving users the ability to control the precedence.

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Oct 30, 2023
@CobaltCause
Copy link

CobaltCause commented Oct 30, 2023

Oh, I had no idea that was a thing. Is this behavior documented anywhere? I don't see anything about it in these sections for example: https://doc.rust-lang.org/cargo/commands/cargo.html?highlight=$CARGO_HOME/bin/#files, https://doc.rust-lang.org/cargo/guide/cargo-home.html?highlight=bin#directories.

@epage
Copy link
Contributor

epage commented Nov 2, 2023

I honestly can't think of a good place to put it. I feel like most people who are wondering about precedence for this wouldn't look there but there isn't any other central place for using third-party subcommands, only a place for developing them.

Follpvosten pushed a commit to Follpvosten/nix-rust-quickstart that referenced this issue Nov 21, 2024
This makes it easier to discover when [this bug][0] is hit.

[0]: rust-lang/cargo#6507
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-custom-subcommands Area: custom 3rd party subcommand plugins C-bug Category: bug S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests

7 participants