-
Notifications
You must be signed in to change notification settings - Fork 877
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
Support toolchain keyword (e.g. " nightly") to set override at root command #2031
Support toolchain keyword (e.g. " nightly") to set override at root command #2031
Conversation
63b814b
to
1e075d7
Compare
bd13a56
to
0aacf5f
Compare
0aacf5f
to
edfcb0c
Compare
Note: 97e6043 updated these:
Per:
Update: current version does not need warning anymore. |
edfcb0c
to
97e6043
Compare
33fd8db
to
7b7b736
Compare
@kinnison, sounds good. Updated per suggestion. PTAL at convenience? Thanks. |
7b7b736
to
a9d7dad
Compare
a9d7dad
to
4ef8a1c
Compare
@pickfire, there's indeed only one toolchain (either by keyword or option format) in effect. Though the previous "toolchain-*" names were probably misleading. Updated to, hopefully, less confusing names (
The current logic is to follow @kinnison's suggestion ☝️ . I think the assignment of If there's a way to omit |
42a0693
to
7e697b0
Compare
546a9a6
to
b0220fc
Compare
@pickfire / @kinnison, after taking a step back and rethinking the approach, I also agree that overriding toolchain in cfg, and pass would have broader beneficial scope than just I've tested w. the reported command from #1498:
|
796a20d
to
94f639e
Compare
94f639e
to
8b2dac1
Compare
@pickfire / @kinnison, updated 👇 per previous comments, and did some test about not saving settings in default profile. PTAL at convenience? (🤞feel really close to LGTY 😺) Per tests/cli-misc.rs
Indeed the leading underscore was my overlook. Updated. Also indeed the custom-1 to custom-2 conversion is to support the custom toolchain in the Per src/cli/rustup_mode.rs
Tried out From an earlier comment
Tested
|
f8560e4
to
3ff67f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Could you please add a test to assert that rustup stable set profile minimal
doesn't affect the default toolchain?
@BeniCheni Nice, looks good to me. Thanks a lot for taking this on. Just left the test that @kinnison mentioned. |
3ff67f7
to
48b5837
Compare
@kinnison, sure thing! Added tests per the helpful suggestion. Thank you. PTAL? 🙏
@pickfire, thank you! The version wouldn't have matured to the current version now, which could service all subcommed w. " toolchain". Gratitude for your time & effort to review. Please take a final look, if you have time? 🙏 |
@BeniCheni Oh, no worries. I was initially trying to do this but I cannot find how .arg(
Arg::with_name("toolchain")
.help(TOOLCHAIN_ARG_HELP)
.long("toolchain")
.takes_value(true),
Arg::with_name(" toolchain")
.help("release channel (e.g. stable) or custom toolchain to set override"),
) so I learned a lot as well. Either way, it is interesting to see how you use many different functions to piece this up. The only thing I don't get is the need to have https://github.com/rust-lang/rustup.rs/pull/2031/files#diff-a11274be6e834e18a2579dad1b37ce8dR1173, but it does not matter much here. |
@pickfire, I'm guessing the |
@BeniCheni Ah, I saw that there will always be a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it stands, this is not going to work because it stores a persistent override and is very noisy about doing so. We need to ensure that rustup foo ...
doesn't affect any more than the individual execution.
99e07d0
to
6ef832f
Compare
Greeting @kinnison. After the merge of #2075 and the suggestion from Discord ☝️ , I was able to follow along the similar idea and pushed an edit. PTAL at next convenience? Here's some local testing w. the edited version: Tested w. rustup nightly target add wasm32-unknown-unknown
Tested w. rustup beta which cargo
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking very good. Just a couple of minor niggles.
6ef832f
to
3e7c66f
Compare
@kinnison, nice suggestion for those UX tweaks. Added a validator, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to behave as desired. 👍
Thank you very much for your efforts here.
Support the toolchain keyword (e.g " nightly") to set override of the current working directory for toolchain, at the root command.
This PR should resolve #1498, as the local test is using similar test case to the reported command from the issue:
Test w.
rustup nightly target add wasm32-unknown-unknown
Confirm that the target is added in my rustup toolchain nightly folder