Skip to content

Conversation

smoelius
Copy link
Contributor

As suggested by @weihanglo in #4518 (comment).

@djc
Copy link
Contributor

djc commented Oct 19, 2025

Should we add a caveat like "except insofar as to mirror an earlier invocation from rustup"?

Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo #4549 (comment).

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@smoelius
Copy link
Contributor Author

I've made the change since that seems to be the consensus. But, personally, I think it's a bad idea to include the caveat.

First, there is no demonstrated need. The situation described in #4518 (comment) is hypothetical. I think it is safer to not include caveats until there is a demonstrated need for them.

And second, it's not clear to me how it would work. Currently, the proxy sets RUSTUP_TOOLCHAIN_SOURCE. So, for a tool to set RUSTUP_TOOLCHAIN_SOURCE, it would have to set it and call the proxied tool directly, not through the rustup proxy. Is that what we expect?

@smoelius smoelius force-pushed the expand-rts-documentation branch from 3232562 to 03102e2 Compare October 19, 2025 13:20
@rami3l
Copy link
Member

rami3l commented Oct 19, 2025

@smoelius Indeed, if recursive calls are in place then this caveat currently serves no purpose; however IIRC what you described (bypassing rustup's toolchain resolution) was exactly the scenario @weihanglo has pointed out.

This gets me thinking if recursive calls have a role to play in this story and, if they do, which one. I personally think it has to do with what cargo wants from this feature (cc @weihanglo).

@smoelius
Copy link
Contributor Author

Please merge if you are happy with the changes.

@weihanglo
Copy link
Member

First, there is no demonstrated need. The situation described in #4518 (comment) is hypothetical.

That kind of use cases I believe are mostly and more likely in enterprise environments, rather than in a public space. And I assure it is not hypothetical.

The only use case right now is rust-lang/cargo#16131, which would incur an imprecise warning in cargo install if wrapper doesn't propogate RUSTUP_TOOLCHAIN_SOURCE (which is not too bad to be honest).

This gets me thinking if recursive calls have a role to play in this story and, if they do, which one.

It depends on what kind of recursive call, I guess? If they call something like cargo +nightly build in a build.rs, then there is nothing Cargo can/should do, as the code path will just be handled by normal rustup toolchain resolution. If they are setting RUSTUP_TOOLCHAIN directly, then we know they are doing weird thing, so either they should set RUSTUP_TOOLCHAIN_SOURCE along with that, or do whatever they want.

@rami3l rami3l added this pull request to the merge queue Oct 19, 2025
@weihanglo
Copy link
Member

weihanglo commented Oct 19, 2025

Currently, the proxy sets RUSTUP_TOOLCHAIN_SOURCE. So, for a tool to set RUSTUP_TOOLCHAIN_SOURCE, it would have to set it and call the proxied tool directly, not through the rustup proxy.

Not exactly, The rust-analyzer case is like

  1. Run rustc --print sysroot. The rustc here is actually a rustup proxy
  2. Retrieve the sysroot path and set it in RUSUTP_TOOLCHAIN to ensure the same toolchain is called fix: Set RUSTUP_TOOLCHAIN and invoke the proxies instead of directly invoking sysroot binaries rust-analyzer#16563.

rust-analyzer itself doesn't really call proxies directly IIUC. Granted, the way rust-analyzer dealing with toolchain proxies has changed a couple of times and I might already have the outdated understanding.

Merged via the queue into rust-lang:main with commit 68c13cf Oct 19, 2025
29 checks passed
@smoelius smoelius deleted the expand-rts-documentation branch October 19, 2025 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants