feat(config)!: use unqualified names for default_toolchain - #4947
Conversation
Cfg::get_default_resolvable()default_toolchain
1d2c5c6 to
108ca96
Compare
This comment has been minimized.
This comment has been minimized.
37bfef2 to
8975942
Compare
This comment has been minimized.
This comment has been minimized.
8975942 to
bb5bf44
Compare
bb5bf44 to
9f1d12c
Compare
9f1d12c to
5616ba4
Compare
This comment has been minimized.
This comment has been minimized.
5616ba4 to
34ad35f
Compare
This comment has been minimized.
This comment has been minimized.
aa51f83 to
04c0cc9
Compare
04c0cc9 to
52d7cb1
Compare
This comment has been minimized.
This comment has been minimized.
52d7cb1 to
e737b5f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e737b5f to
afe6341
Compare
This comment has been minimized.
This comment has been minimized.
|
@djc Would you mind giving this PR another look please? 🙇 |
This comment has been minimized.
This comment has been minimized.
afe6341 to
b2b31f2
Compare
This comment has been minimized.
This comment has been minimized.
I think the PR description should explain (a) what you're trying to achieve here and (b) how you're trying to achieve it, and probably (c) the effects on backwards compatibility, instead of just relying on issue/PR references. |
@djc Thank you so much for this suggestion! I have completely rewritten the PR description to include the 3 points mentioned above. Please don't hesitate to tell me if you happen to have any more concerns, as I will try my best to address them. |
In the code, it was not clear to me how/where old configurations with fully qualified names where still kept supported. I think it would be good to add some comments that make it obvious where/how that happens and that it must not regress. |
b2b31f2 to
e12fcfc
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
e12fcfc to
f8de503
Compare
@djc Sure! I have just added one regression test specifically for the case you've mentioned, asserting that In addition, I've added two more assertions against the exact shape of |
f8de503 to
5207d09
Compare
…s::select_toolchain()`
5207d09 to
b24bd2b
Compare
Stacked on #4950 and #4991, closes #4945.
Problem Description
The original problem as seen in both #3651 and #4945 is the following:
... this is notably problematic because the value of
default_toolchainis inappropriately coupled with the value of the default host at the wrong moment: not the moment of running anyrustupcommand, but the last execution ofrustup default(whether manually by running that very command, or implicitly uponrustup-init):Proposed Solution
This PR makes rustup use unqualified names such as
stableinstead of fully qualified names such asstable-aarch64-apple-darwinfor thedefault_toolchainfield in thesettings.tomlfile.Impact
Compatibility
This change is breaking exactly because the determination of the default toolchain will change to reflect the change of the default host tuple.
However, the user can always get the old behavior by specifying the fully qualified name when running
rustup default(such asrustup default stable-aarch64-apple-darwin), in the rare case that they would like to decouple the default toolchain from the default host tuple.For users with an existing
settings.toml, manually runningrustup default stableor similar commands should be necessary for them to get the new behavior.Performance
As discussed in #4945 (comment), since the default host tuple is resolved at the beginning of
rustup's execution, this change has introduced no extra I/O operations, and thus the performance impact is negligible.Validation
Validated through both local testing, a specific test case that reflects @jyn514's original use case as described in #3651, as well as another specific test case that explicitly asserts backwards compatibility with existing
settings.tomlformat with a fully qualifieddefault_toolchainname.