Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking: Rustup should not perform implicit installations #3635

Closed
5 tasks done
jyn514 opened this issue Jan 7, 2024 · 64 comments
Closed
5 tasks done

Tracking: Rustup should not perform implicit installations #3635

jyn514 opened this issue Jan 7, 2024 · 64 comments
Assignees
Labels
enhancement tracking This is a tracking issue
Milestone

Comments

@jyn514
Copy link
Member

jyn514 commented Jan 7, 2024

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:]

  • uninstalling the default toolchain happens silently. i would expect either a warning (by the same logic as Give a warning before uninstalling the active/default toolchain, or the last/host target for a toolchain #3319), or for rustup to switch to a different default toolchain.
  • 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. [@rami3l: We shouldn't do even that, instead we should let the user explicitly install the required toolchain with a command.]
  • running rustup --version also silently installs.
  • running rustup show also silently installs.

Notes

rustup 1.26.0 (5af9b94 2023-04-05)

@rami3l

This comment was marked as outdated.

@rami3l rami3l self-assigned this Jan 8, 2024
@jyn514

This comment was marked as outdated.

@rami3l rami3l changed the title behavior when uninstalling default toolchain is not ideal Rustup should not perform implicitly installations Jan 12, 2024
@rami3l

This comment was marked as outdated.

@rami3l rami3l added this to the On Deck milestone Jan 17, 2024
@rami3l rami3l changed the title Rustup should not perform implicitly installations Rustup should not perform implicit installations Jan 17, 2024
@rbtcollins

This comment was marked as outdated.

@rami3l

This comment was marked as outdated.

@rami3l rami3l reopened this Mar 13, 2024
@rami3l rami3l added tracking This is a tracking issue and removed duplicate labels May 8, 2024
@rami3l rami3l modified the milestones: On Deck, 1.29.0 Jul 10, 2024
@rami3l rami3l changed the title Rustup should not perform implicit installations Tracking: Rustup should not perform implicit installations Jul 10, 2024
@Doineann

This comment was marked as duplicate.

@rami3l

This comment was marked as duplicate.

@rami3l
Copy link
Member

rami3l commented Jul 15, 2024

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 #1397 (comment)

@abonander
Copy link

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.

@rami3l
Copy link
Member

rami3l commented Jul 30, 2024

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
Copy link

abonander commented Jul 30, 2024

@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?

@djc
Copy link
Contributor

djc commented Aug 23, 2024

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
Copy link
Member

rami3l commented Aug 23, 2024

@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. (#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
Copy link
Member

rami3l commented Aug 30, 2024

@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
Copy link
Contributor

djc commented Sep 4, 2024

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:

  • Will do the expected thing in the common case (especially for inexperienced users)
  • Smoother experience than running a separate command or confirming a prompt
  • The heuristics probably won't be too complex and won't need a ton of maintenance over time
  • Closer to the current behavior so less likely to (negatively) surprise users

Downsides:

  • Heuristics introduce some additional complexity (maintenance over time)
  • Behavior is harder to fully understand
  • Implicit installation behavior might still be surprising to some users

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
Copy link
Contributor

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:

  • IDE integrations's can add code to manage the installed toolchain in a naive way very easily. IDE integrations -really- don't behave well when versions change underneath them, so I think it will be better for everyone if this is a little more explicit. Not necessarily user-driven-selection, but rust-analyzer getting an error about an uninstalled toolchain - which it can then use to trigger an install and reload its state, is better than an implicit install that then leads to super confusing outcomes, after a user pulls a new version of something with a pinned nightly version. For instance.
  • CI - this is mixed, and has changed over the years. Now github includes rustup and a toolchain in their builder images. Some people will definitely need to make changes to their builds; many can probably make them a lot simpler, some will pay a small complexity tax
  • interactive use - this is where the most frustration I recall encountering was.
    • -most- users get stable installed when installing rustup, and this entire discussion is moot/irrelevant
    • some users will be working on, or download, crate sources that require a different toolchain version, and then will encounter this different behaviour

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
Copy link

ickk commented Sep 4, 2024

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
Copy link
Member

Veykril commented Sep 5, 2024

(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
Copy link
Contributor

@Veykril #3434 is probably of interest too then :)

@djc
Copy link
Contributor

djc commented Sep 5, 2024

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

Well, it's a large change and I could see how people get frustrated with the experience hit (similar to @samestep). I think effectively no one is using main. But I guess we could just release it and see how it goes.

@rami3l
Copy link
Member

rami3l commented Sep 5, 2024

I think effectively no one is using main.

@djc That's not entirely true: I use it as my daily driver :]

And from my experience, not many repos I work with use toolchain overrides, so I'm feeling okay anyway in this regard.

But I guess we could just release it and see how it goes.

We have a test driving phase to do before going live, so I hope this will help in terms of collecting user feedback.

@djc
Copy link
Contributor

djc commented Sep 5, 2024

I think effectively no one is using main.

@djc That's not entirely true: I use it as my daily driver :]

Note the use of "effectively". 😉

@djc
Copy link
Contributor

djc commented Sep 5, 2024

Okay, let's close this and see what happens during the beta period.

@djc djc closed this as not planned Won't fix, can't repro, duplicate, stale Sep 5, 2024
@LukeMathWalker
Copy link

LukeMathWalker commented Sep 10, 2024

I'm just catching up with this whole conversation as a person who routinely relies on rustup show in Docker files to trigger a toolchain installation according to what's inside the rust-toolchain.toml file.
What would be the recommended upgrade path for me? What command should I invoke after this update?

I explicitly don't want to perform any compilation, so invoking cargo commands like check or build is not the way forward.

@ChrisDenton
Copy link
Member

rustup install

@rami3l
Copy link
Member

rami3l commented Sep 10, 2024

I'm just catching up with this whole conversation as a person who routinely relies on rustup show in Docker files to trigger a toolchain installation according to what's inside the rust-toolchain.toml file.

What would be the recommended upgrade path for me? What command should I invoke after this update?

I explicitly don't want to perform any compilation, so invoking cargo commands like check or build is not the way forward.

@LukeMathWalker You'd do rustup toolchain install/rustup install (without any arguments) to manually install the active toolchain starting from v1.28.0. I promise we'll deliver the new release notes with a huge emphasis on this change.

@LukeMathWalker
Copy link

I'm just catching up with this whole conversation as a person who routinely relies on rustup show in Docker files to trigger a toolchain installation according to what's inside the rust-toolchain.toml file.
What would be the recommended upgrade path for me? What command should I invoke after this update?
I explicitly don't want to perform any compilation, so invoking cargo commands like check or build is not the way forward.

You'd do rustup toolchain install (without any arguments) to manually install the active toolchain starting from v1.28.0. I promise we'll deliver the new release note with a huge emphasis on this change.

Has that been changed recently? Or was it always an option to invoke rustup toolchain install without any arguments?
Because it is highly non-obvious if going off what's returned by rustup toolchain install --help.

@rami3l
Copy link
Member

rami3l commented Sep 10, 2024

Has that been changed recently? Or was it always an option to invoke rustup toolchain install without any arguments? Because it is highly non-obvious if going off what's returned by rustup toolchain install --help.

@LukeMathWalker It's a recent change in #3983. The actual syntax is suggested by @ChrisDenton IIRC.

We have also updated the help message:

> rustup --version
rustup 1.27.1+445 (15a7ec40f 2024-09-09)
[..]

> rustup toolchain install --help
Install or update the given toolchains, or by default the active toolchain

Usage: rustup[EXE] toolchain install [OPTIONS] [TOOLCHAIN]...
[..]

@rami3l
Copy link
Member

rami3l commented Sep 11, 2024

I am looking forward to this change, so thanks for working on it

re: breakage

Might I suggest adding the new behaviour (#3983), and then waiting a release or two before completely removing the implicit installs? My concern is having a path with some continuity; rustup toolchain install on <= 1.27.1 is an error, but after >= 1.28 it will be an error not to do so, leaving no grace period.

If there is need - perhaps some tool that is blocked waiting on the new behaviour - during the transitional period an extra flag could be added to opt-out of implicit installs, at the risk of some complexity.
#3985 (comment)

@ickk It's a perfectly reasonable suggestion. Let's think about the available options together and find out how to do this properly.

@rami3l
Copy link
Member

rami3l commented Sep 11, 2024

I am looking forward to this change, so thanks for working on it
re: breakage
Might I suggest adding the new behaviour (#3983), and then waiting a release or two before completely removing the implicit installs? My concern is having a path with some continuity; rustup toolchain install on <= 1.27.1 is an error, but after >= 1.28 it will be an error not to do so, leaving no grace period.
If there is need - perhaps some tool that is blocked waiting on the new behaviour - during the transitional period an extra flag could be added to opt-out of implicit installs, at the risk of some complexity.
#3985 (comment)

@ickk It's a perfectly reasonable suggestion. Let's think about the available options together and find out how to do this properly.

@ickk Do you think rustup show active-toolchain || rustup toolchain install will suffice?*

  • If you're on the old version, the first command should install the active toolchain and return true, short-circuiting the second;
  • If you're on the new version, the first command should return false if the active toolchain is not installed, causing the second to ensure its installation.

*: I'm not sure about the corresponding PowerShell syntax, maybe the following? (cc @ChrisDenton)

$(rustup show active-toolchain | Out-Host; $?) -or $(rustup toolchain install | Out-Host; $?)

@ChrisDenton
Copy link
Member

What's wrong with using rustup show active-toolchain || rustup toolchain install in powershell?

@rami3l
Copy link
Member

rami3l commented Sep 11, 2024

What's wrong with using rustup show active-toolchain || rustup toolchain install in powershell?

@ChrisDenton I did some more research; looks like that's a new syntax in PS7, and I must have been looking at some old literature. I'm not sure how we should properly suggest this for Windows users in general, is || widespread enough as for now?

@ChrisDenton
Copy link
Member

ChrisDenton commented Sep 11, 2024

Oh for old powershell I'd do it like:

rustup show active-toolchain; if ($LASTEXITCODE -ne 0) { rustup toolchain install }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement tracking This is a tracking issue
Projects
None yet
Development

No branches or pull requests