Set RUSTUP_TOOLCHAIN_SOURCE

[smoelius]

This PR is in pursuit of rust-lang/cargo#11036. Specifically, it sets an environment variable RUST_TOOLCHAIN_SOURCE. The variable's content will be used to issue a warning when a cargo install is performed with a non-default toolchain.

The variable's content is one of cli, env, override, config, or default.

Nits are welcome.

cc: @epage

[smoelius]

The variable's content has the form source toolchain

This is redundant, because the latter component is RUSTUP_TOOLCHAIN. I'll fix this PR in the next few hours.

EDIT: Done.

[djc]

Thanks for working on this!

(Please squash your commits -- except for a potential ActiveReason rename, which would be nice to keep in a separate commit or PR.)

[smoelius]

(Please squash your commits -- except for a potential ActiveReason rename, which would be nice to keep in a separate commit or PR.)

I will once the dust has settled. 🙂

[smoelius]

I made one small change ("downstream" -> "proxied") and squashed all of my commits down to one.

I think there are still three outstanding points, which could be separate PRs:

• Document that we should only inherit RUSTUP_TOOLCHAIN_SOURCE if we are inheriting the parent's toolchain (Set RUSTUP_TOOLCHAIN_SOURCE #4518 (comment)). My attempts at this felt unnatural, so I left this alone for now.
• Mention RUSTUP_TOOLCHAIN_SOURCE in src/toolchain.rs (Set RUSTUP_TOOLCHAIN_SOURCE #4518 (comment)). I propose adding "Note that RUSTUP_TOOLCHAIN_SOURCE is set in src/cli/proxy_mode.rs." or similar to to set_env's rustdoc comment.
• Possibly rename ActiveReason (Set RUSTUP_TOOLCHAIN_SOURCE #4518 (comment)). I'd like to let @rami3l comment, as I have no strong opinion.

[smoelius]

I think all unchanged uses of "reason" are in license files, markdown files, or comments. But I apologize if I missed anything.

[rami3l]

I don't have other questions myself but I'd like to wait for other reviewers to make sure that their concerns have been properly addressed.

Nice work!

[djc]

LGTM, thanks!

[smoelius]

LGTM, thanks!

And the tests haven't even passed yet! 😆 Thank you!

[smoelius]

Is anything still needed before this PR can be merged?

[djc]

Don't think so, thanks again!

[weihanglo]

AFAIK, rust-analyzer (and some other tools) overrides RUSTUP_TOOLCHAIN environment variable directly based on the result of its probing ¹. They also documents that user might set that directly ².

My questions are:

• Should those cases above all be considered as environment overrides source? What is the source of true of RUSTUP_TOOLCHAIN_SOURCE?
• Should tool override and provide the original RUSTUP_TOOLCHAIN_SOURCE if they have resolved to the actual toolchain first on their own through?

Note that this is probably not going to affect general user cases, and RUSTUP_TOOLCHAIN_SOURCE can just be a best-effort env. Just FYI at $WORK we have a quite complex layer of rustup overrides and I wonder whether we should fully trust this or reset it whenever we override RUSTUP_TOOLCHAIN.

Footnotes

1. https://github.com/rust-lang/rust-analyzer/blob/1e20331e42449dfc0b44bce84147a06772d045d7/crates/rust-analyzer/src/flycheck.rs#L639 ↩

2. https://github.com/rust-lang/rust-analyzer/blob/1e20331e42449dfc0b44bce84147a06772d045d7/docs/book/src/installation.md?plain=1#L24-L26 ↩

[smoelius]

AFAIK, rust-analyzer (and some other tools) overrides RUSTUP_TOOLCHAIN environment variable directly based on the result of its probing 1. They also documents that user might set that directly 2.

My questions are:

• Should those cases above all be considered as environment overrides source? What is the source of true of RUSTUP_TOOLCHAIN_SOURCE?

Could your question be restated as, "What are the definitions of the five RUSTUP_TOOLCHAIN_SOURCE categories?"

I would argue they are the same as in the "Overrides" section of the Rustup book.

For the sake of your examples, the toolchain would not be specified on the command line,¹ so the toolchain should be determined by RUSTUP_TOOLCHAIN. In that sense, it seems correct that RUSTUP_TOOLCHAIN_SOURCE should be set to env.

• Should tool override and provide the original RUSTUP_TOOLCHAIN_SOURCE if they have resolved to the actual toolchain first on their own through?

...Just FYI at $WORK we have a quite complex layer of rustup overrides and I wonder whether we should fully trust this or reset it whenever we override RUSTUP_TOOLCHAIN.

My non-authoritative opinion is "no", tools other that rustup should not set RUSTUP_TOOLCHAIN_SOURCE.

I am reminded of a comment @epage made in rust-lang/cargo#15099 (comment):

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.

If non-rustup tools are allowed to set RUSTUP_TOOLCHAIN_SOURCE, I could imagine it leading to a million bug reports down the road.

Should we expand the documentation to say that non-rustup tools should not set RUSTUP_TOOLCHAIN_SOURCE?

Footnotes

1. It's also not clear to me what that would mean. ↩

[weihanglo]

the toolchain would not be specified on the command line,1 so the toolchain should be determined by RUSTUP_TOOLCHAIN. In that sense, it seems correct that RUSTUP_TOOLCHAIN_SOURCE should be set to env.

Given the current definition of RUSTUP_TOOLCHAIN_SOURCE, I agree. Although it’s somewhat of an optimization on rust-analyzer’s side, the value r-a sets is still retrieved through the rustup proxy.

I guess my question could be rephrased as: “If a tool caches the rustup-resolved toolchain path for optimization, should it also propagate the original RUSTUP_TOOLCHAIN_SOURCE?”

I don’t think we need to overthink this, since rust-analyzer users likely won’t be affected. That said, we should keep in mind that some “rustup” wrappers might produce unexpected or less precise diagnostics if Cargo starts treating RUSTUP_TOOLCHAIN_SOURCE as the source of truth. (Those wrappers probably want to preserve the original source instead of being treated as env.)

Anyway, I agree that we may want to make this clearer in the documentation, especially which environments are meant for reading and which are for writing.

[djc]

I guess my question could be rephrased as: “If a tool caches the rustup-resolved toolchain path for optimization, should it also propagate the original RUSTUP_TOOLCHAIN_SOURCE?”

It seems reasonable to answer this with yes.

I wonder if we can test this whole thing before we release rustup with this code? I guess it would need some of the relevant Cargo changes (not sure if anyone is working on those already) and then pulling in both as snapshot builds.

[rami3l]

IMHO I agreed to include this patch based on the premise that this is a) a private contract between us and cargo; b) a best-effort optimization to communicate with the latter why a toolchain is activated.

As such, I am wondering what will the problems caused by overriding that env var be if you are already bypassing rustup's toolchain resolution. So "yes" to the question above, since the results are purely cargo-oriented.

[smoelius]

I guess it would need some of the relevant Cargo changes (not sure if anyone is working on those already)

@djc Just to answer your question: rust-lang/cargo#16131