`$CARGO` passed to external subcommands can point to the wrong `cargo` if already set

[Alexendoo]

Problem

Observed in rust-lang/rust-clippy#14045, if cargo +nightly clippy is ran by a process with CARGO pointing to the stable cargo then nightly clippy-driver will be ran by stable cargo

In this case that resulted in a confusing warning

Steps

1. Put the following into your PATH as cargo-foo

   #!/usr/bin/env bash
   
   echo "$CARGO"

2. With stable cargo use cargo run on

   use std::process::Command;
   
   fn main() {
       Command::new("cargo")
           .args(["+nightly", "foo"])
           .status()
           .unwrap();
   }

3. The output will be .../.rustup/toolchains/stable-.../bin/cargo

Possible Solution(s)

Looks to be due to #11285, perhaps there could be an exception for when the current exe really is cargo

Notes

No response

Version

cargo 1.84.0 (66221abde 2024-11-19)

[weihanglo]

perhaps there could be an exception for when the current exe really is cargo

How to achieve that? I mean isn't it be like a revert of #11285?

[weihanglo]

Feel like one remote cause is the cargo-clippy is, while "official", still an external command, so need to rely on $CARGO.

[Alexendoo]

How to achieve that? I mean isn't it be like a revert of #11285?

Looking at just #10119 Cargo the binary could set a flag that disables the forwarding, that would keep #11285 working for commands that use Cargo as a library

For #10113 though I'm not sure

[smoelius]

If one invokes cargo +nightly <anything>, it's pretty clear one wants to run the cargo proxy because of the +nightly.

Are there cases where one would want the cargo proxy to preserve the CARGO environment variable? I'm wondering if the proxy should clear it.

If I'm reading #10119 right, that issue is about using Cargo as a library. So I don't think clearing the CARGO environment variable in the proxy would impact that issue's fix.

[smoelius]

I'm going to boldly assume #15099 (comment) is not crazy and cc some rustup maintainers. @ChrisDenton @djc @rami3l

My question comes down to: should the cargo proxy clear the CARGO environment variable? Or are there legitimate reasons for the proxy to preserve the environment variable?

For completeness, @Alexendoo poses a different solution in #15099 (comment).

[rami3l]

@smoelius Hmmm there doesn't seem to be any mention of $CARGO in our codebase so I might assume it's currently not handled at all. Where is that variable usually read then? Maybe it's better to handle this logic over there if possible? (Otherwise I imagine it'd be difficult for rustup-less setups.)

[djc]

I think we should fail the invocation if the +-specified toolchain and $CARGO don't agree -- seems like PEBKAC to me.

[smoelius]

@rami3l

@smoelius Hmmm there doesn't seem to be any mention of $CARGO in our codebase so I might assume it's currently not handled at all.

That the variable is not handled at all (by the proxy) is my takeaway as well.

Where is that variable usually read then?

It looks like it is read by many tools: https://github.com/search?q=var%28%22CARGO%22%29&type=code

This specific issue arose because Clippy reads the variable: https://github.com/rust-lang/rust-clippy/blob/e02c8857e83e9113c29e8bd2b429f62dfaba04a7/src/main.rs#L110

Maybe it's better to handle this logic over there if possible?

Please see my response to djc below.

(Otherwise I imagine it'd be difficult for rustup-less setups.)

I may be misunderstanding what you mean. But I think the issue is specifically about cargo +<toolchain> invocations, which implies rustup setups.

---

@djc

I think we should fail the invocation if the +-specified toolchain and $CARGO don't agree

I think failing the invocation would be better than the current situation (i.e., allowing the command to proceed with the wrong cargo).

seems like PEBKAC to me.

In fairness, the problem is subtle. As @Alexendoo explains in rust-lang/rust-clippy#14045 (comment):

• The cargo proxy runs, selects the default cargo, and sets the CARGO environment variable to default toolchain's cargo.
• That cargo invocations runs some other code, which invokes cargo +<toolchain> <subcommand>.
• <subcommand> reads the CARGO environment variable and runs the default toolchain's cargo rather than <toolchain>'s cargo.

@rami3l Your question ("Maybe it's better to handle this logic over there if possible?") is fair, and may be the right answer. But please notice that <subcommand> above could be just about anything.

---

Thank you both for your responses. It looks like there are currently two options:

• Clear the CARGO environment variable in the proxy.
• Fail the invocation when the +-specified toolchain and $CARGO don't agree.

Are there other options? Do folks have strong opinions regarding these two?

[djc]

I suppose the question is if there are good use cases for having a CARGO set which you want to override using the +toolchain. Having rustup decide to prioritize the +toolchain over the environment-specified one feels a little opinionated which I tend to dislike, but on the other hand explicit overrides taking priority over environment-specified ones is usually sane.

[rami3l]

@smoelius Thanks a lot for your clarification! This looks like a cargo-calling-rustup issue so indeed it's specific to rustup.

And, from my perspective, it looks like rustup needs to start following some external convention. If it's already established somewhere and our not responding (such as sending a warning instead of actually doing anything) is not an option, I guess we would have to add more magic (such as wiping $CARGO) following the current direction, because, as a user, I have no idea why calling cargo in cargo run would make things difficult for rustup, so I expect everything to "just work" as if cargo run were not setting $CARGO at all.

@weihanglo This is a bit off-topic, but recursive calls are also exactly why it's so difficult for me to add a proper lock to rustup in rust-lang/rustup#988, where a common locking scheme can also be broken by considering rustup calls in cargo run . Given all this, I'm afraid to open a can of worms trying to fix all the edge cases related to recursive calls while keeping rustup mostly transparent to our end users. Maybe we should be less transparent and take @djc's approach, concluding that it's up to the user to prevent such complexities?