rustup warns about its own environment? ("warning: default toolchain implicitly overridden")

[nabijaczleweli]

Downstream issue: nabijaczleweli/cargo-update#337

cargo-update (.../cargo-install-update) runs cargo install to install stuff, so the call-stack is (1) cargo install-update ... -> (2) cargo-install-update -> (3) cargo install. It appears that the rustup cargo wrapper puts the toolchain selected in 1 in the environment so lower layers will obey this configuration (good), understands and obeys it in 3 (good), but now decides to warn about this everything-going-as-designed scenario (bad).

[ChrisDenton]

While it's possible rustup could make some improvements, the warning is coming from cargo: https://github.com/rust-lang/cargo/blob/f40cc7a13a8ebe3dd0a35b1e0e37a1e26c8bb0e5/src/cargo/ops/cargo_install.rs#L337-L363

[nabijaczleweli]

The usual suspect >_<

Forwarded to rust-lang/cargo#17172

[smoelius]

Admittedly, I was surprised when I saw the inner cargo proxy setting RUSTUP_TOOLCHAIN_SOURCE to environment. (Note that the environment variable was already set to default.)

Should a proxy set RUSTUP_TOOLCHAIN_SOURCE only when it is not already set?

[rami3l]

@smoelius Looking at this issue, it does seem like the default toolchain has been implicitly overridden because now a subprocess (2) is calling back into rustup, which usually doesn't happen. Of course that subprocess can choose whatever active toolchain to call (say it calls cargo +nightly install, how can you tell?).

IMHO you are right that RUSTUP_TOOLCHAIN_SOURCE should be cleaned/reset, but I think it should be that subprocess' responsibility, not cargo's or rustup's?

@nabijaczleweli I would suggest that you tweak RUSTUP_TOOLCHAIN_SOURCE and/or RUSTUP_TOOLCHAIN to your liking before calling back so the problem should be resolved.

[smoelius]

@smoelius Looking at this issue, it does seem like the default toolchain has been implicitly overridden because now a subprocess (2) is calling back into rustup, which usually doesn't happen.

I feel like that misrepresents the issue. cargo-install-update is simply calling cargo, which happens to be a Rustup proxy. I suspect that cargo-invoked tools call Rustup proxies all the time.

Moreover, RUSTUP_TOOLCHAIN_SOURCE is kind of an implementation detail. I would argue that common Rust tools should not need to worry about this variable, or even know that it exists.

Regarding your suggestion:

@nabijaczleweli I would suggest that you tweak RUSTUP_TOOLCHAIN_SOURCE and/or RUSTUP_TOOLCHAIN to your liking before calling back so the problem should be resolved.

Which tool should act on the tweaked values, rustup or cargo?

rustup does not seem to check RUSTUP_TOOLCHAIN_SOURCE; rustup only sets it (please correct me if I am wrong).

rustup does check RUSTUP_TOOLCHAIN (that's kind of the problem), but it's not clear what cargo-install-update should set it to, because the user might have set it intentionally.

cargo does check RUSTUP_TOOLCHAIN_SOURCE (again, that's part of the problem). But I would argue that users should not set this variable.

cargo does check RUSTUP_TOOLCHAIN, but again, the user might have set it intentionally.

I am simply having a hard time seeing how cargo-install-update could address this problem.

[rami3l]

@smoelius Thanks a lot for the clarification. In that case I agree with you that there is currently no way for either cargo or any cargo plugin to distinguish what is actually going on without leaking abstractions. I propose instead the following change in rustup if it makes sense to you:

If RUST_RECURSION_COUNT exists and has a numerical value that is larger than 0, rustup should not set RUSTUP_TOOLCHAIN_SOURCE. This way, only the top-level proxy call can set that value.

Still I see a downside with that: any binary that calls rustup this way will behave differently in cargo run compared to when it is called directly. In other words we have to assume that the indicated source is always inherited and the nature cannot change in the middle.

[rami3l]

@smoelius Sorry, let me take a step back: the root of this issue seems to be the dual meaning of a RUSTUP_TOOLCHAIN override and whether we can see it is overridden by the user or inherited for the sake of coherence.

Since the latter is controlled by rustup itself, maybe we can invent a new reason for that specifically? For example: "set to FOO for inheritance", and once set to that kind of value, the reason itself will not be reset by rustup to "environment".

This way, if RUSTUP_TOOLCHAIN is reset to a different value by the user, cargo will be able to know because FOO is already registered in the env vars.

[smoelius]

If RUST_RECURSION_COUNT exists and has a numerical value that is larger than 0, rustup should not set RUSTUP_TOOLCHAIN_SOURCE. This way, only the top-level proxy call can set that value.

I think this idea makes sense. (I considered proposing something like this.)

Since the latter is controlled by rustup itself, maybe we can invent a new reason for that specifically? For example: "set to FOO for inheritance", and once set to that kind of value, the reason itself will not be reset by rustup to "environment".

I'm not sure I follow this idea. "set to FOO for inheritance" would be a new value that RUSTUP_TOOLCHAIN_SOURCE is set to?

[rami3l]

I'm not sure I follow this idea. "set to FOO for inheritance" would be a new value that RUSTUP_TOOLCHAIN_SOURCE is set to?

@smoelius Yes, you have understood that correctly.

so the call-stack is (1) cargo install-update ... -> (2) cargo-install-update -> (3) cargo install

My observation here is that (3) relies on the toolchain passed down from (1) via RUSTUP_TOOLCHAIN so the default toolchain is really implicitly overridden as warned by cargo, just to the same value by chance.

Just to make it clearer, I wanted to say that it is currently impossible to distinguish between:

1. RUSTUP_TOOLCHAIN has been set to FOO because rustup wants its subprocesses to inherit that value.
2. RUSTUP_TOOLCHAIN has been set to FOO because it's an override from the user.

Currently, both cases would cause a proxy to set RUSTUP_TOOLCHAIN_SOURCE to environment which might not be what you want to see from downstream rustup or cargo, and the intention is partially lost down the chain. So the best thing you can currently do from cargo's perspective is to assume that environment may be an implicit override and warn the user (case 2), even if what actually happened is the case 1.

This semantic difference is best demonstrated in the following scenarios:

A. Imagine a cargo plugin with RUSTUP_TOOLCHAIN=FOO set by rustup upstream calls back into rustup with the implicit active toolchain:

• With the current behavior, RUSTUP_TOOLCHAIN_SOURCE is set to environment and a false alarm is triggered.
• If we dispatch based on RUST_RECURSION_COUNT, now the count should be larger than zero, so RUSTUP_TOOLCHAIN_SOURCE=default is retained, and we have correctly eliminated a warning.
• With the new proposed scheme, rustup knows from RUSTUP_TOOLCHAIN_SOURCE that the upstream was using FOO so it keeps that unchanged, and the downstream knows there hasn't been a conflict between RUSTUP_TOOLCHAIN_SOURCE's FOO and RUSTUP_TOOLCHAIN=FOO.

B. Imagine a cargo plugin with RUSTUP_TOOLCHAIN=FOO set by rustup upstream calls back into rustup with a new active toolchain BAR:

• With the current behavior, RUSTUP_TOOLCHAIN_SOURCE is set to environment and the warning is correctly triggered.
• If we dispatch based on RUST_RECURSION_COUNT, now the count should be larger than zero, so RUSTUP_TOOLCHAIN_SOURCE=default is retained, and we have swallowed a warning by mistake.
• With the new proposed scheme, rustup now knows the distinction between cases 1 and 2. From RUSTUP_TOOLCHAIN_SOURCE, it knows that the upstream was using FOO so it keeps that unchanged. In the meantime, RUSTUP_TOOLCHAIN is updated to BAR, and now the downstream knows there has been an implicit change from FOO to BAR.

Please feel free to correct me if you have spotted anything, I might not be thinking super straight at this point...

But looking back, your current implementation/behavior seems like the simplest at the cost of false alarms from time to time.

[smoelius]

B. Imagine a cargo plugin with RUSTUP_TOOLCHAIN=FOO set by rustup upstream calls back into rustup with a new active toolchain BAR:
...

• If we dispatch based on RUST_RECURSION_COUNT, now the count should be larger than zero, so RUSTUP_TOOLCHAIN_SOURCE=default is retained, and we have swallowed a warning by mistake.

I see your point.

I'm still not following why RUSTUP_TOOLCHAIN_SOURCE should be "set to FOO for inheritance", though. Why not just "inheritance" or "inherited"? All of the existing values are simple strings that can be explicitly matched. This new one would require something more complicated.

[rami3l]

Why not just "inheritance" or "inherited"?

@smoelius Because the downstream would need to know if THE CURRENT value of RUSTUP_TOOLCHAIN is inherited from an upstream rustup, not just whether ANY value has been inherited.

As you said, anyone outside of rustup is not supposed to know what RUSTUP_TOOLCHAIN_SOURCE means, once set to "inherited", it will remain like that while RUSTUP_TOOLCHAIN can be freely swapped in the middle. Once that happens, cargo will look at "inherited" and think this newly-swapped RUSTUP_TOOLCHAIN is inherited from the upstream, so it will not issue the warning: we have a false negative here.

I am not suggesting any specific format we should change the value of those two environment variables to; the above was just an analysis of the minimal information required for this warning to have neither false positives nor false negatives.

This new one would require something more complicated.

I agree that it would imply a complex design by the way, and that's why I'm currently inclined to just not fixing it. Having a few false positives is better than having false negatives, and probably also better than having a much more complex mechanism that is 100% accurate... unless we can come up with something simpler.

[nabijaczleweli]

Just to make it clearer, I wanted to say that it is currently impossible to distinguish between:

1. RUSTUP_TOOLCHAIN has been set to FOO because rustup wants its subprocesses to inherit that value.
2. RUSTUP_TOOLCHAIN has been set to FOO because it's an override from the user.

It seems pretty simple to distinguish since it's rustup talking to itself (or an internal contract between rustup and cargo? either way.):

1. set RUSTUP_TOOLCHAIN=rustup-internal!FOO in situation (1) above
2. parse like

env::var("RUSTUP_TOOLCHAIN").map(|raw| match raw.strip_prefix("rustup-internal!") {
    Some(inherited) => (inherited, true)
    None => {
        eprintln!("don't set RUSTUP_TOOLCHAIN yourself pls :)");
        (raw, false)
    }
})

or

env::var("RUSTUP_TOOLCHAIN").and_then(|rt| rt.strip_prefix("rustup-internal!")) 

or whatever?