Skip to content

feat(config): warn user if auto-install is enabled, take 2#4859

Open
rami3l wants to merge 4 commits into
rust-lang:mainfrom
rami3l:feat/warn-auto-install-2
Open

feat(config): warn user if auto-install is enabled, take 2#4859
rami3l wants to merge 4 commits into
rust-lang:mainfrom
rami3l:feat/warn-auto-install-2

Conversation

@rami3l
Copy link
Copy Markdown
Member

@rami3l rami3l commented May 16, 2026

Alternative design for #4454.

Closes #4445.

Opening a draft early to collect CI errors, please don't review yet 🙏

This PR has minimized its dependencies on #4840 so hopefully it'll be easier to backport this to 1.29 as well.
Since #4840 is a breaking change, I'd still like to add some 1.29-exclusive deprecation warnings based on it (part of #4211).

rami3l added 2 commits May 16, 2026 10:14
…_RECURSION_COUNT`

This commit clears the value of `RUSTUP_AUTO_INSTALL` and the
`RUST_RECURSION_COUNT` environment variables before running tests, to
make sure that every test case relying on their effects is explicitly
setting them.
@rami3l rami3l force-pushed the feat/warn-auto-install-2 branch 6 times, most recently from 5d4438c to b2e4ba4 Compare May 16, 2026 13:04
@rami3l rami3l force-pushed the feat/warn-auto-install-2 branch from b2e4ba4 to a9611a3 Compare May 16, 2026 13:07
@rami3l rami3l marked this pull request as ready for review May 16, 2026 15:13
Comment thread src/config.rs

/// Represents the result of an operation that may ensure the installation of a certain toolchain.
#[derive(Clone, Debug)]
pub(crate) struct Ensured<T> {
Copy link
Copy Markdown
Contributor

@djc djc May 18, 2026

Choose a reason for hiding this comment

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

Ensured is a pretty generic name. Something more specific might be nicer?

View changes since the review

Comment thread src/cli/rustup_mode.rs
} else if ensure_active_toolchain {
let (toolchain, source) = cfg.ensure_active_toolchain(force_non_host, true).await?;
info!("the active toolchain `{toolchain}` has been installed");
info!(
Copy link
Copy Markdown
Contributor

@djc djc May 18, 2026

Choose a reason for hiding this comment

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

Could these be replaced by a Deref impl on Ensured?

View changes since the review

Comment thread src/config.rs
RustupError::ToolchainNotSelected(process.name().unwrap_or_else(|| "Rust".into())).into()
}

fn auto_install_warning(process: &Process, toolchain: impl Display, status: &UpdateStatus) {
Copy link
Copy Markdown
Contributor

@djc djc May 18, 2026

Choose a reason for hiding this comment

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

Could/should this take an Ensured? Could it be a method on Ensured? I'd suggest moving the Process argument last, since it's more boring/long-lived/less relevant.

View changes since the review

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.

Show that a toolchain download is due to auto installation

2 participants