rust-lang / rustup

The Rust toolchain installer
https://rust-lang.github.io/rustup/
Apache License 2.0
6.13k stars 882 forks source link

Tracking: Rustup should not perform implicit installations #3635

Closed jyn514 closed 1 month ago

jyn514 commented 9 months ago

Problem you are trying to solve

here is a transcript of a session with rustup

PS C:\Users\jyn\src\example> rustup toolchain list                              
nightly-x86_64-pc-windows-msvc (default)
1.74-x86_64-pc-windows-msvc
stage1
PS C:\Users\jyn\src\example> rustup uninstall nightly
info: uninstalling toolchain 'nightly-x86_64-pc-windows-msvc'
info: toolchain 'nightly-x86_64-pc-windows-msvc' uninstalled
PS C:\Users\jyn\src\example> rustup toolchain list   
1.74-x86_64-pc-windows-msvc
stage1
PS C:\Users\jyn\src\example> rustc
info: syncing channel updates for 'nightly-x86_64-pc-windows-msvc'
info: latest update on 2024-01-07, rust version 1.77.0-nightly (b6a8c762e 2024-01-06)
info: downloading component 'cargo'
^C
PS C:\Users\jyn\src\example> rustc +nightly
error: toolchain 'nightly-x86_64-pc-windows-msvc' is not installed
PS C:\Users\jyn\src\example> rustup --version
rustup 1.26.0 (5af9b9484 2023-04-05)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: syncing channel updates for 'nightly-x86_64-pc-windows-msvc'

Proposed Solution & Progress

[The following behaviors of the current rustup should be changed:]

Notes

rustup 1.26.0 (5af9b9484 2023-04-05)

### Tasks
- [ ] #3319
- [ ] https://github.com/rust-lang/rustup/issues/2686
- [ ] https://github.com/rust-lang/rustup/issues/3943
- [ ] https://github.com/rust-lang/rustup/pull/3948
- [ ] #1397
- [ ] https://github.com/rust-lang/rustup/issues/3982
rami3l commented 9 months ago

@jyn514 Thanks a lot for filing this issue! I see basically two points being made here:

This is a nice suggestion, and I'm looking forward to implementing it :)

  • running rustc afterwards implicitly reinstalls the toolchain. i do not expect rustup to implicitly install in any case other than when rust-toolchain.toml is in the current directory. in fact, you can see that rustc +nightly does not implicitly install, only rustc by itself.
  • running rustup --version also silently installs.

As for removing implicit installations, it's already on our list (https://github.com/rust-lang/rustup/issues/3546#issuecomment-1831292725), but it seems to me that a separate issue hasn't been created yet.

Would you mind if I split this issue into two so that they can be tracked separately? I personally suggest use #3546 to track the removal of implicit installations, and move the first point over to #3319. Does that sound okay to you?

jyn514 commented 9 months ago

sounds good to me :)

rami3l commented 9 months ago

Hmmm I tried to change the description of #3546 but maybe it's not the best place to track implicit installation removal. I'll reuse this thread for that instead.

rbtcollins commented 8 months ago

See https://github.com/rust-lang/rustup/pull/2797#issuecomment-864506040 and follow the links out fro there. Multiple toolchains in #3546 seems entirely orthogonal. Closing as duplicate with https://github.com/rust-lang/rustup/issues/1397

rami3l commented 8 months ago

@rbtcollins Sorry, I've changed this to be a meta tracking issue which has #1397 on its list. Maybe that's not clear enough?

Doineann commented 2 months ago

I expect rustc --version to just output version information, like how --version would behave in any other application.

In my humble opinion, rustc --version should not be responsible for upgrading toolchains. The user should do that either explicitly or maybe when running rustc with any other flags for which it makes more sense for silent updates to be performed.

The --version argument should really just be a 'request' for version information and not trigger any other actions.

To be clear... I think it's ok to trigger silent updates for different calls to rustc with different arguments, but I believe --version should really only output the version (and maybe a hint that the current version is a mismatch by what is specified in rust-toolchain.toml).

Performing silent updates, and maybe even more actions, may even pose security risks to users that do not expect these actions to happen silently when running a command like rustc --version.

(I understood that my issue with rustc should really be discussed here... sorry, still new here)

rami3l commented 2 months ago

@Doineann Thanks for your feedback! Yes, what you said is part of what we want to change with this issue, see the 2nd point.

rami3l commented 2 months ago

Regarding implicit installs, as a user it seems obvious to me that commands that I expect be purely informational wouldn't modify my system under any circumstances: rustup show rustup check rustup which rustup --version rustup --help

It wouldn't really bother me to have commands that already modify the behaviour of my system in some way automatically install things, and would often be a pleasant convenience: rustup run rustup ensure? rustup update rustup target rustup default rustup toolchain rustup component rustup override

I think rustup doc could go either way. by @ickk in https://github.com/rust-lang/rustup/issues/1397#issuecomment-817282560

abonander commented 2 months ago

I've been looking through related issues and PRs for the last 20 minutes and I still don't have a straight answer:

I was sent to #1397 by https://rust-lang.github.io/rustup/overrides.html#overrides but that issue is now closed. I'm here because this appears to be the latest one with activity and I didn't want to necro.

rami3l commented 2 months ago

I've been looking through related issues and PRs for the last 20 minutes and I still don't have a straight answer:

  • Is rustup show meant to automatically install the toolchain in rust-toolchain.toml?
  • If so, is this behavior going to change without warning in a future update?
  • If not, what command should I use in CI to ensure the correct version of Rust is installed?

I was sent to #1397 by https://rust-lang.github.io/rustup/overrides.html#overrides but that issue is now closed. I'm here because this appears to be the latest one with activity and I didn't want to necro.

@abonander Hi! #1397 has been closed and the fix will be shipped in Rustup v1.28.0 (see #3225's milestone).

The plan for this release also includes introducing a rustup toolchain ensure command that installs the toolchain.

We don't currently have a specific timeline for a new release though as the team has limited bandwidth right now, I'm really sorry 🙇

abonander commented 2 months ago

@rami3l I still don't understand. I saw #3225 and the discussion was very difficult to follow as it mixed conversations about the behavior of the command with those concerning the output of the command.

Could I just get a plain answer to my question, please?

rami3l commented 2 months ago

Could I just get a plain answer to my question, please?

I've edited my response above. TLDR:

  • Is rustup show meant to automatically install the toolchain in rust-toolchain.toml?
  • No, it isn't.
  • If so, is this behavior going to change without warning in a future update?
  • Yes, it will.
  • If not, what command should I use in CI to ensure the correct version of Rust is installed?
  • Through the (still unimplemented) rustup toolchain ensure command.
abonander commented 2 months ago

Thank you.

rami3l commented 2 months ago

Thank you.

@abonander Thank you for letting us know we are not being clear enough as well! As this is a major change we definitely don't want to ship this halfway done... Thanks again for your comprehension.

rbtcollins commented 2 months ago

@rami3l I'm not clear exactly where the plan is at on this, but with respect to this comment I think that convenience is how we got to the current place.

I believe it is much easier to write tools on top of very predictable systems, and that there is a convenience argument for implicit installs throughout. My suspicion (not validated via research) is that having no implicit installs, one command ensure that a toolchain is installed, is enough.

I have previously written about having one command to install, where an existing install is an error, and one to ensure an install, but I'm struggling to think of a place where the error 'cannot install toolchain X, it is already installed' would be useful. There is a clear nearby-case of linked toolchains where X can mean anything, but having a single command that errors when the linked path is different in that case, and has a --force override, seems like it would be sufficient.

rami3l commented 2 months ago

I believe it is much easier to write tools on top of very predictable systems, and that there is a convenience argument for implicit installs throughout. My suspicion (not validated via research) is that having no implicit installs, one command ensure that a toolchain is installed, is enough.

I have previously written about having one command to install, where an existing install is an error, and one to ensure an install, but I'm struggling to think of a place where the error 'cannot install toolchain X, it is already installed' would be useful. There is a clear nearby-case of linked toolchains where X can mean anything, but having a single command that errors when the linked path is different in that case, and has a --force override, seems like it would be sufficient.

@rbtcollins Thanks for the extra context! My current plan is indeed to remove implicit installations entirely. This goes even further than @jyn514 has originally envisioned, as I plan to add rustup toolchain ensure to the mix.

In addition, this will probably provide a better foundation to addressing the toolchain pruning problem as described in https://github.com/rust-lang/rustup/issues/3919#issuecomment-2208299114:

I installed Lean 4 with rustup-derived elan. When I was maintaining a project that relied on mathlib4, the latter included a toolchain file. As I bumped it from time to time, the toolchain file also got updated, so I ended up with more and more versioned installations of the toolchain without a way to "prune" them... I imagine the same thing will happen with Rust projects as well.

Apart from that new command, the only thing left to do seems to be rustc --version (i.e. #3943). However since our own test cases are also relying on implicit installations on rustc --version (and potentially cargo --version as well), I must implement rustup toolchain ensure first and then dogfood it immediately in our tests in order to close #3943.

djc commented 2 months ago

@rami3l I think @rbtcollins is arguing that adding an ensure command is not worth it, because people who want to use it can just use rustup toolchain install instead? This line of reasoning makes sense to me: what are the additional UX affordances we can provide by having a separate command? How would its behavior be different?

abonander commented 2 months ago

rust toolchain install requires a toolchain specification which makes it useless if you just want to ensure that the toolchain specified in rust-toolchain.toml is installed.

Unless the plan is to make it do exactly that if no toolchain is specified on the command line, then yeah, ensure is redundant.

rami3l commented 2 months ago

@rami3l I think @rbtcollins is arguing that adding an ensure command is not worth it, because people who want to use it can just use rustup toolchain install instead? This line of reasoning makes sense to me: what are the additional UX affordances we can provide by having a separate command? How would its behavior be different?

@djc rustup toolchain ensure is a dummy command, of course the actual syntax can be rustup toolchain install with no additional arguments. I agree with @abonander.

Currently:

> rustup toolchain install
error: the following required arguments were not provided:
  <TOOLCHAIN>...

Usage: rustup[EXE] toolchain install <TOOLCHAIN>...

For more information, try '--help'.
ChrisDenton commented 2 months ago

And presumably the rustup install alias.

What would be the semantics here? Would it only install if there is a rust-toolchain.toml file or would it install any missing default toolchain?

rami3l commented 2 months ago

What would be the semantics here? Would it only install if there is a rust-toolchain.toml file or would it install any missing default toolchain?

@ChrisDenton My understanding is that it is expected to do anything rustc --version currently does before actually running that command on the corresponding rustc, it's just that everything will become explicit now. So yes, this includes (and isn't limited to) installing the toolchain specified in the rust-toolchain.toml file.

And presumably the rustup install alias.

Yes! As the two subcommands has been unified with #3596, that looks like a less painful move indeed. A single change for these two places!

samestep commented 1 month ago

I don't have much context about this, so this is probably a stupid question, but in what way are the drawbacks of automatic toolchain installation big enough to outweigh the benefits? When collaborating on a Rust project, I find it incredibly convenient that my collaborators can just run cargo test after I merge a PR adding a dependency to Cargo.toml, and it will automatically download, build, and run code from the newly added dependency; this is in contrast to the very common case on JavaScript projects where people will forget to run npm install and then message me asking why npm run test fails. I see rust-toolchain.toml as just an extension of that idea: if I merge a PR updating our Rust version, I want everyone to be able to continue going about their day by running cargo test, instead of having to Google to find an unfamiliar rustup toolchain ensure command. The analogous story in JavaScript is, again, worse: I need to insist everyone already be using nvm, and run an nvm command that I don't even remember in order to install the version specified in the repo's .nvmrc file.

I'm guessing there's some security argument here, but I have a hard time understanding why installing and running a newer version of Rust itself is more dangerous than installing and running a random crate which is needed by a test suite. Could someone point me to the discussion where this implicit installation was decided against? Or am I just misunderstanding, and this thread is only about removing implicit installation from rustup commands, not cargo commands?

Alternatively, if the decision has already been solidly made against implicit Rust toolchain installation, would it be possible for cargo commands to just fail if the toolchain specified in rust-toolchain.toml is not already installed, with an error message like this?

Error: the Rust toolchain specified in rust-toolchain.toml is not installed. Please run the following command, then run cargo again:

rustup toolchain ensure
ChrisDenton commented 1 month ago

Or am I just misunderstanding, and this thread is only about removing implicit installation from rustup commands, not cargo commands?

It is only about removing implicit installation from rustup commands, not cargo commands. I.e. whether or not it will automatically download a new rustc and cargo version even if you didn't ask for it. Rustup has no control over what Cargo does or doesn't do

samestep commented 1 month ago

@ChrisDenton OK perfect, thank you for clarifying!

jyn514 commented 1 month ago

Rustup has no control over what Cargo does or doesn't do

this is not true. in a rustup managed environment, cargo is a hardlink to a rustup proxy that decides which version of cargo to run.

(outside of a rustup-managed environment, rust-toolchain simply doesn't work)

samestep commented 1 month ago

@jyn514 so... what is the answer to my question above?

ChrisDenton commented 1 month ago

this is not true. in a rustup managed environment, cargo is a hardlink to a rustup proxy that decides which version of cargo to run.

I mean, I guess that's pedantically true. But deciding on updating dependencies is totally Cargo's domain.

jyn514 commented 1 month ago

I mean, I guess that's pedantically true. But deciding on updating dependencies is totally Cargo's domain.

that's not what @samestep's question is about, though. they're asking about what version of cargo implicitly gets run when there's a rust-toolchain file in that folder.

@samestep i don't know; i haven't been following this issue closely. you'd need to ask the rustup maintainers.

ChrisDenton commented 1 month ago

Then I'm confused because they say:

I find it incredibly convenient that my collaborators can just run cargo test after I merge a PR adding a dependency to Cargo.toml, and it will automatically download, build, and run code from the newly added dependency

Which will still work fine with or without this change.

samestep commented 1 month ago

@ChrisDenton it seems like you didn't read the rest of my comment?

I see rust-toolchain.toml as just an extension of that idea: if I merge a PR updating our Rust version, I want everyone to be able to continue going about their day by running cargo test, instead of having to Google to find an unfamiliar rustup toolchain ensure command.

ChrisDenton commented 1 month ago

Ah, yes that will change. They would need to do rustup install if they don't have the required toolcahin. But it could provide a useful error saying that.

djc commented 1 month ago

@samestep makes some good points. So what was the rationale for not implicitly installing a toolchain?

So maybe the guideline should be that rustup commands should never implicitly install, but invocations of the proxy should?

jyn514 commented 1 month ago

So maybe the guideline should be that rustup commands should never implicitly install, but invocations of the proxy should?

i would be happy with that. i originally wrote in this issue

I do not expect rustup to implicitly install in any case other than when rust-toolchain.toml is in the current directory. in fact, you can see that rustc +nightly does not implicitly install, only rustc by itself.

but i no longer feel as strongly about that; i only feel that—in the case where there's no rust-toolchain.toml in the current directory—rustc and rustc +toolchain should be consistent with each other about whether they implicitly install.

ickk commented 1 month ago

I don't have much context about this, so this is probably a stupid question, but in what way are the drawbacks of automatic toolchain installation big enough to outweigh the benefits?

It is really weird and alarming that certain commands will start downloading an entire toolchain instead of just providing information. In particular, if you're just trying to list the help, rustup --help, you don't need to connect to the internet, download a toolchain (& possibly fail), before simply printing the help message. The other commands for which automatic downloads are really weird are listed in https://github.com/rust-lang/rustup/issues/3635#issuecomment-2227602648. The previous issues have a bunch of comments from users surprised by the current situation.

This can also be problematic when writing 3rd party tools that rely on rustup/cargo, as it is implicit and surprising, and there's not really any way to avoid it.

I'm not sure what the plan is with regards to the possible addition of ensure (or whatever the name will be). It could either replace all automatic installs with an explicit call to rustup ensure; alternatively it could just be an addition (i.e. many existing commands might continue to implicitly install), for when you don't want to do anything except ensure you have the required toolchain (this might be desirable for e.g. an explicit toolchain install step for CI workflows).

samestep commented 1 month ago

@ickk it seems like everyone in this thread agrees that having rustup --help or other rustup commands implicitly download a toolchain is undesirable; the only controversial part seems to be about similar behavior for cargo.

ickk commented 1 month ago

If the maintainers' current plans are to remove implicit install behaviour altogether (that's how this issue reads - I don't necessarily agree with this, but w/e), a short message describing how to install the missing toolchain would pretty much be just as easy for the user

Diggsey commented 1 month ago

I think rustup should follow this logic:

Does the requested command require a toolchain to be installed that is not currently installed?

Yes: Ask the user if they want to install the toolchain, and if they say yes, do it and then continue the command (or do it automatically when in silent mode)

No: Do not install a toolchain.

Note: running a proxy (such as cargo) would count as requiring the current toolchain to be installed.

rami3l commented 1 month ago

If the maintainers' current plans are to remove implicit install behaviour altogether (that's how this issue reads - I don't necessarily agree with this, but w/e), a short message describing how to install the missing toolchain would pretty much be just as easy for the user

@ickk The message is already there, as per the description in https://github.com/rust-lang/rustup/pull/3985:

https://github.com/rust-lang/rustup/blob/e857897d74215c6f33db7b7360fbbb12a89b46b3/tests/suite/cli_v2.rs#L320-L336

rami3l commented 1 month ago

I don't have much context about this, so this is probably a stupid question, but in what way are the drawbacks of automatic toolchain installation big enough to outweigh the benefits?

@samestep The main issue here is that other tools (like starship) would want to call rustc --version from time to time. While this should return immediately in general, auto installation is making this command potentially blocking for minutes. As a result, I guess we have to make rustup more predictable (and less smart) this time.

Does the requested command require a toolchain to be installed that is not currently installed? Yes: Ask the user if they want to install the toolchain, and if they say yes, do it and then continue the command (or do it automatically when in silent mode) No: Do not install a toolchain.

@Diggsey Thanks for your suggestion. I'm open to adding the prompt in the future.

rbtcollins commented 1 month ago

I think as interactive prompt when a tty is attached might be good, but we should definitely:

djc commented 1 month ago

I don't have much context about this, so this is probably a stupid question, but in what way are the drawbacks of automatic toolchain installation big enough to outweigh the benefits?

@samestep The main issue here is that other tools (like starship) would want to call rustc --version from time to time. While this should return immediately in general, auto installation is making this command potentially blocking for minutes. As a result, I guess we have to make rustup more predictable (and less smart) this time.

Might make sense to have special handling for running the proxy with -V or --version?

rami3l commented 1 month ago
  • 3943 argued that rustup --version should not implicitly install a toolchain

So maybe the guideline should be that rustup commands should never implicitly install, but invocations of the proxy should?

@djc Hmmm, I'm afraid not, since that issue actually talked about the proxy.

Might make sense to have special handling for running the proxy with -V or --version?

Would you mind elaborating on this point? As I see it, there are other ways to make rustc (or other proxied binaries) print informational stuff other than the version info (e.g. rustc --print target-list) and whitelisting all that sounds too magical.

The current design is for maximal consistency and fewest surprises, as explained in https://github.com/rust-lang/rustup/issues/3635#issuecomment-2299104619 (thanks, @jyn514!). I don't think we should do different things for the proxy and for other rustup commands.

djc commented 1 month ago

So maybe the guideline should be that rustup commands should never implicitly install, but invocations of the proxy should?

@djc Hmmm, I'm afraid not, since that issue actually talked about the proxy.

That's fair.

The current design is for maximal consistency and fewest surprises, as explained in #3635 (comment) (thanks, @jyn514!). I don't think we should do different things for the proxy and for other rustup commands.

While optimizing for consistency and fewest surprises makes sense, I'm inclined to agree with @samestep that this might come at a non-trivial cost to developer experience, where cargo check requires a manual rustup invocation after someone updates the toolchain version in a rustup-toolchain.toml in your repo. I suppose this might happen with some regularity in projects that use nightly versions. So I don't think it's crazy to wonder about refined heuristics here.

@samestep in your experience, how often is the toolchain updated in projects you work on? Do you think it would still be prohibitive if the change was instead to put up an explicit prompt that you could just Enter on to move forward with installing the new toolchain?

Might make sense to have special handling for running the proxy with -V or --version?

Would you mind elaborating on this point? As I see it, there are other ways to make rustc (or other proxied binaries) print informational stuff other than the version info (e.g. rustc --print target-list) and whitelisting all that sounds too magical.

Not sure, allowlisting (I think we prefer to use "whitelisting" these days) both --version/-V and --print might still make sense? Or we could just decide that --version/-V is special because it is more often used as a sort of probe.

rami3l commented 1 month ago

@djc Now cargo check on Rustup v1.27 should be equivalent to rustup install && cargo check on v1.28, for example, so hopefully there shouldn't be that much overhead being introduced even in its current state. (https://github.com/rust-lang/rustup/pull/3987 will further improve the output noise problem, if that's relevant.)

OTOH, I definitely think a prompt will achieve similar results while remaining explicit. I've originally planned to do that as part of the console/indicatif series of patches so that we can unify the handling for all interactive prompts, but there's nothing to stop that from happening sooner.

I'm also curious about @samestep's feedback.

rami3l commented 1 month ago

@djc Also, I'd like to mention that a common case that would trigger #988 is, for example, running rustup update in the IDE, when the latter is also running a blocking and/or long-running cargo or rust-analyzer subprocess.

djc commented 1 month ago

So zooming out: IMO if we introduce the need for rustup install where previously only cargo check was sufficient, or even if we introduce a prompt that asks for installation of the toolchain and users opt to run the suggested command or confirm the prompt like 99% of the time, then IMO we've made things worse for them. So I wonder, do we think this is a likely scenario?

I actually think the case of running cargo via Rust-Analyzer is another good reason that implicit installations for common commands that it uses is a good thing, as a suggested command will not be as easily accessible when you're running RA in your IDE and now it's even more of a context switch.

(I think we should avoid mixing #988 into this issue -- yes, that can happen, but it happens today and there would still be other ways it can happen if we require explicit toolchain installations.)

So I think there are like 5 paths potential ways to go:

  1. Released versions, toolchains are implicitly installed for rustup commands and the proxy
  2. Current main, toolchains are never implicitly installed, suggested command must be run by the user
  3. Some UX improvement: in terminals, prompt the user instead of requiring running the suggested command separately
  4. Partial rollback, clean boundary: rustup commands don't implicitly install, proxy commands do
  5. Partial rollback, heuristics: rustup commands don't implicitly install, "informational" proxy commands don't implicitly install but other proxy commands do

After two more weeks of consideration, I still like (5) best:

Downsides:

We could also introduce a new environment variable (RUSTUP_IMPLICIT_INSTALL?) that could be used to disable implicit installations entirely which could help (separately from anything else).

@Veykril do you have opinions from the RA side of things?

rbtcollins commented 1 month ago

IMO if we introduce the need for rustup install where previously only cargo check was sufficient, or even if we introduce a prompt that asks for installation of the toolchain and users opt to run the suggested command or confirm the prompt like 99% of the time, then IMO we've made things worse for them.

The experience Daniel and I had over 5 or so years of supporting users here has been that rustup going out to the internet unpredictable is a significant problem.

Consider this breakdown instead:

Separately, goenv, pyeng, nodenv - and others - all have an explicit model, and I haven't seen any frustration at that on the 'net.

tl;dr, I think this is being over-thought! do we have any actual data of people using main and being unhappy with it?

ickk commented 1 month ago

I'm mostly curious what the plan is to roll this out; Is the impact expected to be minimal?, is there going to be some kind of deprecation strategy for the old behaviour to help tools and scripts which depend on rustup?

Veykril commented 1 month ago

(not reading up on the thread but as I was pinged) r-a would likely benefit from an explicit install step. I'm annoyed with checking out a repo that has a toolchain override in it just for r-a to hide the installation behind a fetching metadata step as its being installed while r-a invoked cargo metadata. Even better would be if we could query whether there is an override for the given dir so we could prompt the user (if desired, otherwise fallback to installing ourselves). Likewise was was said, having rustup management integration in the IDE would be nice as well (which would require querying more state about rustup things as well)

Oh also being able to query whether the toolchain for the dir needs to be installed of course

rbtcollins commented 1 month ago

@Veykril https://github.com/rust-lang/rustup/issues/3434 is probably of interest too then :)