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

fix!(config): re-enable active toolchain installation in Cfg::find_active_toolchain() with optional opt-out #4214

Merged
merged 2 commits into from
Mar 4, 2025

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented Mar 4, 2025

Note

Following @djc's previous statement at #4211 (comment), I sincerely apologize for all the previous issues related to the abrupt breaking change without enough clarity and transparency, and, if a well-organized public discussion allows this change to happen, a potential experimental stage with a properly-extended migration period.

This PR tries to mitigate #4211 by introducing a new environment variable RUSTUP_AUTO_INSTALL that defaults to 1 and is subsumed under CI. This environment variable, when evaluated to true, will try to install the active toolchain in Cfg::find_active_toolchain().

@rust-lang/rustup please feel free to change this PR as you see fit so that we can land the fix as promptly as possible.

Concerns

Remaining work around this PR might include:

  • Introducing a proper warning message about the originally-planned change in behavior;
  • Making sure that the tests pass properly;
  • Ensuring that both the old way and the new way are working as intended.

@rami3l rami3l requested review from djc and ChrisDenton March 4, 2025 05:55
@rami3l rami3l force-pushed the fix/bandaid branch 2 times, most recently from 81de518 to fc6d18e Compare March 4, 2025 13:02
@rami3l rami3l marked this pull request as ready for review March 4, 2025 13:07
@rami3l rami3l requested a review from ChrisDenton March 4, 2025 13:21
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

This looks great to me. There are things I could quibble with but given the circumstances I don't think they need to be addressed in this PR.

@djc
Copy link
Contributor

djc commented Mar 4, 2025

I've split this into two commits to make it a little easier to review, and have made some stylistic tweaks. LGTM!

@djc djc enabled auto-merge March 4, 2025 14:57
@djc djc added this pull request to the merge queue Mar 4, 2025
Merged via the queue into master with commit 95da617 Mar 4, 2025
29 checks passed
@djc djc deleted the fix/bandaid branch March 4, 2025 15:29
@ChrisDenton ChrisDenton mentioned this pull request Mar 4, 2025
4 tasks
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.

3 participants