fix(cli/rustup_mode)!: skip auto-installation in some rustup commands#4840
fix(cli/rustup_mode)!: skip auto-installation in some rustup commands#4840rami3l wants to merge 5 commits intorust-lang:mainfrom
rustup commands#4840Conversation
4892599 to
8ab1b84
Compare
|
@djc Commands that should respect auto-installation:
rustup-init No, because the notion of toolchain doesn't exist yet in that stage
install No, because `rustup install` installs the active toolchain by default
uninstall No, similarly to `rustup install`
toolchain No
default No, because `rustup default foo` installs `foo`
show No, because it's query-only
update No, similarly to `rustup toolchain`
check No, because it's query-only
target No, because it operates on an installed toolchain
component No, similarly to `rustup target`
override No
run Yes, because the said toolchain is invoked
which No, because it's query-only
doc Yes, because the said toolchain is relied upon
man Yes, similarly to `rustup doc`
self No, similarly to `rustup-init`
set No
completions NoWhat do you think? |
|
I like it! Should we review |
@djc We have four subcommands here: |
|
Sounds good. Are there other commands with subcommands? |
@djc I wrote all the above analysis with all subcommands in mind already. Please feel free to tell me if you think there are any cases you don't agree with. PS: The best way to enumerate all dispatch points is to go to the code itself: Lines 643 to 770 in 6e68164 |
Okay, just checking! |
9e5fee3 to
9f734ca
Compare
d3c1bfb to
320c081
Compare
|
Almost done. The only concern would be that I'll try to fix it here soon... |
|
PS: It's very fun in that I'll just need to search for |
320c081 to
29e0119
Compare
29e0119 to
370bf93
Compare
370bf93 to
42b4bea
Compare
|
Oh, the CI passes now, so I'm marking this first PR as ready. Please feel free to let me know if there are any improvements/clarifications to be made :) |
There was a problem hiding this comment.
Pull request overview
This PR addresses rustup auto-install being triggered too eagerly (notably under certain rustup subcommands) by adding a configuration-level switch to disable auto-install earlier during clap dispatch in rustup_mode.
Changes:
- Add
Cfg::allow_auto_installand gateCfg::should_auto_install()on it, so auto-install can be disabled without consulting env/settings. - Decide
allow_auto_installperRustupSubcmdinrustup_mode(enabling it only for subcommands that execute/rely on the active toolchain). - Adjust
rustup show/--versionbehavior and update CLI test snapshots accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/suite/cli_v2.rs | Updates auto-install disable tests to exercise proxy (cargo +nightly) instead of rustup target. |
| tests/suite/cli_rustup.rs | Updates rustup show expectations when the active toolchain is missing. |
| tests/suite/cli_misc.rs | Updates snapshots for version message and which behavior when toolchains are missing. |
| src/config.rs | Introduces allow_auto_install and makes should_auto_install() return false when disallowed. |
| src/cli/setup_mode.rs | Updates Cfg::from_env call to pass allow_auto_install for setup/init flow. |
| src/cli/self_update.rs | Updates test to match new Cfg::from_env signature. |
| src/cli/rustup_mode.rs | Computes allow_auto_install from the selected subcommand; adjusts show and version output behavior. |
| src/cli/proxy_mode.rs | Updates Cfg::from_env call to pass allow_auto_install for proxy execution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let allow_auto_install = match subcmd { | ||
| // These subcommands execute or rely on the active toolchain, so auto-installing it when | ||
| // missing may be reasonable depending on the user's decision. | ||
| #[cfg(not(windows))] | ||
| RustupSubcmd::Man { .. } => true, | ||
| RustupSubcmd::Doc { .. } | RustupSubcmd::Run { .. } => true, | ||
|
|
||
| // These subcommands don't require the active toolchain, so auto-installing it should be | ||
| // disabled to avoid surprises. | ||
| RustupSubcmd::Check { .. } | ||
| | RustupSubcmd::Completions { .. } | ||
| | RustupSubcmd::Component { .. } | ||
| | RustupSubcmd::Default { .. } | ||
| | RustupSubcmd::DumpTestament | ||
| | RustupSubcmd::Install { .. } | ||
| | RustupSubcmd::Override { .. } | ||
| | RustupSubcmd::Self_ { .. } | ||
| | RustupSubcmd::Set { .. } | ||
| | RustupSubcmd::Show { .. } | ||
| | RustupSubcmd::Target { .. } | ||
| | RustupSubcmd::Toolchain { .. } | ||
| | RustupSubcmd::Uninstall { .. } | ||
| | RustupSubcmd::Update { .. } | ||
| | RustupSubcmd::Which { .. } => false, | ||
| }; | ||
|
|
| let allow_auto_install = match subcmd { | ||
| // These subcommands execute or rely on the active toolchain, so auto-installing it when | ||
| // missing may be reasonable depending on the user's decision. | ||
| #[cfg(not(windows))] | ||
| RustupSubcmd::Man { .. } => true, | ||
| RustupSubcmd::Doc { .. } | RustupSubcmd::Run { .. } => true, | ||
|
|
||
| // These subcommands don't require the active toolchain, so auto-installing it should be | ||
| // disabled to avoid surprises. | ||
| RustupSubcmd::Check { .. } | ||
| | RustupSubcmd::Completions { .. } | ||
| | RustupSubcmd::Component { .. } | ||
| | RustupSubcmd::Default { .. } | ||
| | RustupSubcmd::DumpTestament | ||
| | RustupSubcmd::Install { .. } | ||
| | RustupSubcmd::Override { .. } | ||
| | RustupSubcmd::Self_ { .. } | ||
| | RustupSubcmd::Set { .. } | ||
| | RustupSubcmd::Show { .. } | ||
| | RustupSubcmd::Target { .. } | ||
| | RustupSubcmd::Toolchain { .. } | ||
| | RustupSubcmd::Uninstall { .. } | ||
| | RustupSubcmd::Update { .. } | ||
| | RustupSubcmd::Which { .. } => false, | ||
| }; |
There was a problem hiding this comment.
Nit: suggest moving this into a method attached to RustupSubCmd?
| } | ||
|
|
||
| // show active toolchain | ||
| { |
There was a problem hiding this comment.
Pre-existing nits to avoid going very deep into rightward drift:
- It's not clear why we're using a nested scope here, but I think we could get rid of it since this is the last expression in the function anyway?
- Yield the
Somecase fromactive_toolchain_and_sourceout of the match, since theNonecase is pretty trivial
| writeln!(t.lock(), "name: {active_toolchain_name}")?; | ||
| writeln!(t.lock(), "active because: {}", active_source.to_reason())?; | ||
|
|
There was a problem hiding this comment.
This change looks more or less unrelated, but feels related, maybe split it into a separate commit?
| }) | ||
| .collect() | ||
| match Toolchain::new(cfg, active_toolchain_name.clone().into()) { | ||
| Ok(active_toolchain) => { |
There was a problem hiding this comment.
Similarly, get this case out of the match if possible? By this point line 1218 is 13 levels deep which IMO is not a good state.
Closes #4836 by making it possible to skip
should_auto_install's dynamic detection entirely.Whether the skip should be applied can be predetermined during
clapdispatch (at least forrustup_mode.rs), so the only remaining question seems to be where we would want to apply the skip.