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).

…_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 7 times, most recently 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 Outdated

/// 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

Copy link
Copy Markdown
Member Author

@rami3l rami3l May 19, 2026

Choose a reason for hiding this comment

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

@djc When I first wrote this I thought this is meant to be read together with other stuff, e.g. Ensured<FooBarToolchain>.

Do you happen to have any suggestions here? Maybe a longer name will help, but EnsuredToolchain<FooBarToolchain> does not necessary make things clearer... 🙏

Copy link
Copy Markdown
Member Author

@rami3l rami3l May 20, 2026

Choose a reason for hiding this comment

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

@djc Let's go with EnsureInstalled? I think that's more specific and it would be more obvious that it's the return type of the .ensure_installed() function.

Comment thread src/cli/rustup_mode.rs Outdated
Comment thread src/config.rs Outdated
@rami3l rami3l force-pushed the feat/warn-auto-install-2 branch 5 times, most recently from deb8e50 to a4e063a Compare May 19, 2026 10:33
@rami3l rami3l self-assigned this May 19, 2026
@rami3l rami3l force-pushed the feat/warn-auto-install-2 branch from a4e063a to d7da769 Compare May 20, 2026 11:41
@rami3l rami3l force-pushed the feat/warn-auto-install-2 branch from d7da769 to 7509cc8 Compare May 20, 2026 11:41
@rami3l rami3l requested a review from djc May 20, 2026 11:43
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