$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?

[Alexendoo]

I suppose the question is if there are good use cases for having a CARGO set which you want to override using the +toolchain

The original case that ran into it was along the lines of cargo running

fn main() {
    cmd!("cargo clippy");
    cmd!("cargo +nightly clippy");
}

Which seems like a perfectly fine use case, if it were instead written as a shell script it would work as expected. Similarly the equivalent but with cargo check would work as expected

fn main() {
    cmd!("cargo check");
    cmd!("cargo +nightly check");
}

[weihanglo]

I feel like it is a bit too much for rustup to handle this. For example, +toolchain override is not the only way to override toolchain. If we're going to make a special case for it, should we also handle the case that user cd into a directory containing rust-toolchain.toml, or having a directory-override?

Users running nested cargo calls should be cautious to whatever it really calls by themselves, especially when nested switching toolchains. Cargo doesn't have well-supported story for this kind of use case, as it needs to make too many assumptions on user's behalf.

Similarly the equivalent but with cargo check would work as expected

If I understand this correctly, this is the issue mentioned eariler that cargo-clippy is not built-in hence the difference.

[Alexendoo]

My view of it is that cargo inherits $CARGO to avoid unintentionally pointing it at something that is not a real cargo binary, be that ld.so or some binary that uses Cargo as a library internally

Since the rustup proxy knows it's about to call the real cargo it could clear $CARGO unconditionally, because the underlying cargo binary will then set it to a working value

I don't think it's necessarily on rustup to solve but it seems like a pragmatic solution

If I understand this correctly, this is the issue mentioned eariler that cargo-clippy is not built-in hence the difference.

Yeah, it's something specific to external subcommands and perhaps build scripts. For example you can hit the same issue with cargo +nightly nextest run

[smoelius]

I feel like it is a bit too much for rustup to handle this. For example, +toolchain override is not the only way to override toolchain. If we're going to make a special case for it, should we also handle the case that user cd into a directory containing rust-toolchain.toml, or having a directory-override?

In the cases you mention, @weihanglo, I think the proxy would run. In other words, if we're going to address this, I feel like the proxy would be an appropriate place to do it.

I would be happy to submit a PR to "fix" this (and include those as test cases). I think we just need to agree on what the "fix" is.

Ideas that have been floated at this point include:

1. Clear the CARGO environment variable silently.
2. Fail the invocation when the +-specified (or otherwise selected) toolchain and $CARGO don't agree.
3. Issue a warning but allow the command to proceed.

I'm sensing that @Alexendoo prefers to clear CARGO environment variable in some way. We might also consider:

4. Clear the CARGO environment variable and issue a warning.
5. Clear or don't clear the CARGO environment variable based on a configuration, but issue a warning either way.

Opinions?

[weihanglo]

I don't really have a use case in mind, perhaps valgrind?
If Rustup unconditionally fails the invocation or clears $CARGO when they disagree, would it break somebody's workflow which $CARGO is deliberately set to something?

[smoelius]

I've created a PR here: rust-lang/rustup#4175 Everyone is welcome to comment.

The approach I've taken is to clear the CARGO environment variable and issue a warning (4 above). Currently, there is no way to disable the clearing. But if the need arises, I think it should be easy to add.

I don't really have a use case in mind, perhaps valgrind? If Rustup unconditionally fails the invocation or clears $CARGO when they disagree, would it break somebody's workflow which $CARGO is deliberately set to something?

@weihanglo I've tried to do some searching, but I cannot tell whether this is a common practice. Put another way, I would expect people to set RUSTUP_TOOLCHAIN rather than CARGO. But I'd be happy to be corrected.

Because of the warnings, I think my changes should be easy to identify if they break people's workflows.

[rami3l]

@smoelius Thanks for your contribution! Adding a warning is definitely worthwhile. However, I'm still not sure if we should make cargo run that transparent.

I don't expect cargo run to work identically to a Shell script like you have mentioned in #15099 (comment) in the first place:

Which seems like a perfectly fine use case, if it were instead written as a shell script it would work as expected.

... at least you won't be able to cargo run when developing rustup, for example, exactly because extra env vars are being added to the execution process. A proper comparison of your "Shell script" would be directly running the resulting executable after cargo build (that's what we are currently doing with rustup) or something similar 🤔

In other words, you could imagine if somehow cargo run is reprogrammed to run your "Shell script", say you've replaced the resulting executable with that script, then you would probably encounter the same issue.

[smoelius]

@smoelius Thanks for your contribution! Adding a warning is definitely worthwhile. However, I'm still not sure if we should make cargo run that transparent.

@rami3l Do you think we should issue a warning but not clear the environment variable (3 above)?

[rami3l]

@smoelius Yes, I think that'll be the most appropriate way forward.

[smoelius]

Does anyone else have an opinion?

@rami3l I'm going to wait at least 24 hours before modifying the PR.

[Alexendoo]

I think if we can make it work we should, as it did before 1.67

Issuing a warning and clearing the variable both effectively mean specifying CARGO to a different value is now not supported, but clearing it makes #15099 (comment) work as expected

It seems unlikely that somebody would intentionally specify CARGO to a different value than the toolchain though since it only applies to build scripts and subcommands