Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions src/cli/rustup_mode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,36 @@ fn update_toolchain_value_parser(s: &str) -> Result<PartialToolchainDesc> {
})
}

impl RustupSubcmd {
fn allow_auto_install(&self) -> bool {
match self {
// 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,
}
}
}

#[derive(Debug, Subcommand)]
enum ShowSubcmd {
/// Show the active toolchain
Expand Down Expand Up @@ -631,15 +661,20 @@ pub async fn main(

update_console_filter(process, &console_filter, matches.quiet, matches.verbose);

let cfg = &mut Cfg::from_env(current_dir, matches.quiet, true, process)?;
cfg.toolchain_override = matches.plus_toolchain;

let Some(subcmd) = matches.subcmd else {
let help = Rustup::command().render_long_help();
writeln!(process.stderr().lock(), "{}", help.ansi())?;
return Ok(ExitCode::FAILURE);
};

let cfg = &mut Cfg::from_env(
current_dir,
matches.quiet,
subcmd.allow_auto_install(),
process,
)?;
cfg.toolchain_override = matches.plus_toolchain;

match subcmd {
RustupSubcmd::DumpTestament => common::dump_testament(process),
RustupSubcmd::Install { opts } => update(cfg, opts, true).await,
Expand Down
21 changes: 13 additions & 8 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,10 @@ impl<T> EnsureInstalled<T> {
}

impl<T: Display> EnsureInstalled<T> {
fn warn_auto_install(&self, process: &Process) {
fn warn_auto_install(&self, cfg: &Cfg<'_>) {
// If we're already in a recursion, or we haven't just installed the active toolchain, then
// don't print the warning.
let recursions = process.var("RUST_RECURSION_COUNT");
let recursions = cfg.process.var("RUST_RECURSION_COUNT");
if recursions.is_ok_and(|it| it != "0") || !matches!(self.status, UpdateStatus::Installed) {
return;
}
Expand All @@ -156,6 +156,15 @@ impl<T: Display> EnsureInstalled<T> {
);
warn!("this might cause rustup commands to take longer time to finish than expected");
info!("you may opt out with `RUSTUP_AUTO_INSTALL=0` or `rustup set auto-install disable`");

if cfg.allow_auto_install {
return;
}

// NOTE: Special behavior for rustup v1.29
warn!("auto-installation of the active toolchain is enabled when running `rustup`");
Copy link
Copy Markdown
Contributor

@djc djc Jun 6, 2026

Choose a reason for hiding this comment

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

Seems weird to have both the warning above and this warning shown at the same time?

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@djc Good catch!

How about showing the existing warning only when the deprecation notice is not shown?

warn!("this behavior may change in an upcoming release for most `rustup` subcommands");
warn!("see <https://github.com/rust-lang/rustup/issues/4836> for more info")
}
}

Expand Down Expand Up @@ -423,10 +432,6 @@ impl<'a> Cfg<'a> {
}

pub(crate) fn should_auto_install(&self) -> Result<bool> {
if !self.allow_auto_install {
Copy link
Copy Markdown
Contributor

@djc djc Jun 6, 2026

Choose a reason for hiding this comment

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

What is this trying to achieve?

View changes since the review

Copy link
Copy Markdown
Member Author

@rami3l rami3l Jun 7, 2026

Choose a reason for hiding this comment

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

@djc Thanks for asking!

In c037408 (#4840) which is what this PR is based on, self.allow_auto_install is used to reject auto install from happening which is achieved by passing false instead of true for some rustup subcommands which will cause this function to reject auto installation right away.

However, since it is decided that in 1.29 we should only warn instead of rejecting directly, this commit replaces it with a warning.

It should be noted that the warning should have existed in this function but since this will cause too many false positives (we should only show it when an auto-installation is actually triggered, not whenever we are in an auto-install deprecated subcommand), it is moved to another module.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Please don't hesitate to suggest any changes if you think that will make your review easier.

return Ok(false);
}

if let Ok(mode) = self.process.var("RUSTUP_AUTO_INSTALL") {
Ok(mode != "0")
} else {
Expand Down Expand Up @@ -562,7 +567,7 @@ impl<'a> Cfg<'a> {
match self.ensure_active_toolchain(true, false).await {
Ok(r) => {
let (tc, source) = r;
tc.warn_auto_install(self.process);
tc.warn_auto_install(self);
Ok(Some((tc.inner, source)))
}
Err(e) => match e.downcast_ref::<RustupError>() {
Expand Down Expand Up @@ -768,7 +773,7 @@ impl<'a> Cfg<'a> {
let install_if_missing = self.should_auto_install()?;
let EnsureInstalled { inner: tc, status } =
Toolchain::from_local(tc, install_if_missing, self).await?;
EnsureInstalled::new(tc.name(), status).warn_auto_install(self.process);
EnsureInstalled::new(tc.name(), status).warn_auto_install(self);
Ok((tc, source))
}
None => {
Expand Down
27 changes: 27 additions & 0 deletions tests/suite/cli_misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1795,3 +1795,30 @@ info: you may opt out with `RUSTUP_AUTO_INSTALL=0` or `rustup set auto-install d
"#]])
.is_ok();
}

#[tokio::test]
async fn warn_auto_install_on_rustup_deprecated() {
let cx = CliTestContext::new(Scenario::SimpleV2).await;
cx.config
.expect_with_env(
["rustup", "component", "list"],
[("RUSTUP_TOOLCHAIN", "stable"), ("RUSTUP_AUTO_INSTALL", "1")],
)
.await
.with_stdout(snapbox::str![[r#"
...
rustc-[HOST_TUPLE] (installed)
...
"#]])
.with_stderr(snapbox::str![[r#"
...
warn: the missing active toolchain `stable-[HOST_TUPLE]` has been auto-installed
warn: this might cause rustup commands to take longer time to finish than expected
info: you may opt out with `RUSTUP_AUTO_INSTALL=0` or `rustup set auto-install disable`
warn: auto-installation of the active toolchain is enabled when running `rustup`
warn: this behavior may change in an upcoming release for most `rustup` subcommands
warn: see <https://github.com/rust-lang/rustup/issues/4836> for more info

"#]])
.is_ok();
}
Loading