$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

[smoelius]

What if we only emitted a warning, but the message said something like:

warn: In a future version of the cargo proxy, 'CARGO' will be cleared.

Would that be acceptable to everyone?

[smoelius]

I updated the PR, though I softened the language from "will be cleared" to "may be cleared".

[epage]

So I'm working to catch up on this.

It sounds like there are the following care abouts

1. Subprocesses of Cargo should inherit the same toolchain as the their parent Cargo process via CARGO
2. Bins using cargo-as-a-library should not be put in CARGO (CARGO is not Cargo when build script is invoked through cargo used as a library #10119)
3. Ld can mess with CURRENT_EXE (among other things) (CARGO env var set incorrectly when Cargo invoked through ld #10113)

#11285 broke the (1) to fix (2) and (3). It did so by setting the precedence in GlobalContext to

1. from_env
2. from_current_exe
3. from_argv

I'm uncomfortable with a rustup solution because this isn't inherently a rustup problem This can happen in other cases like cargo script invoked via rustup spawning the deb build process which spawns Debian-built cargo which spawns CARGO, wanting Debian's cargo and not rustups.

In the Cargo team meeting, someone brought up users manually unsetting CARGO but I worry about how error prone that is and how difficult to debug.

Building off of the idea in #15099 (comment), my proposed solution is that cargo-the-bin opts-in to different behavior than the default.

default-behavior (when using cargo-the-library):

1. from_env
2. from_current_exe
3. from_argv

opt-in behavior (set by cargo-the-bin):

1. from_current_exe if its cargo[EXE_SUFFIX]
2. from_argv if its cargo[EXE_SUFFIX]
3. from_env
4. from_current_exe
5. from_argv

Characteristics

• No change in behavior for cargo-the-library
• We don't try set CARGO=ld
• We don't fix this issue for ld but at least we still work
• We don't fix this issue for when users have an alternative name for the Cargo binary (or on some platforms, an alternative symlink name)
• We fix this issue for regular use of cargo

[smoelius]

@epage Thank you very much for your reply.

Just to be sure I understand:

opt-in behavior (set by cargo-the-bin):

1. from_current_exe if its cargo[EXE_SUFFIX]
2. from_argv if its cargo[EXE_SUFFIX]
3. from_env
4. from_current_exe
5. from_argv

You're saying cargo-the-bin should set the CARGO environment variable based on these rules?

So, for example, if nightly cargo-the-bin is run and finds that CARGO is already set (say, to stable cargo-the-bin), it will nonetheless set CARGO because the current exe satisfies condition 1?

Should a warning be issued in that case, or should the override happen silently?

[epage]

imo overriding CARGO would be intended behavior and wouldn't be reasonable to warn about.

Cargo also tends to avoid warnings that can't be reasonably silenced. We hope to one day have #12235 resolved but that will be project focused and not invocation focused. We do not have plans atm for invocation-focused lint control.

[weihanglo]

Fixed via #15208