Support toolchain keyword (e.g. "+nightly") to set override at root command #2031

BeniCheni · 2019-09-30T20:07:43Z

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`

→ rustup +nightly target add wasm32-unknown-unknown
info: using existing install for 'nightly-x86_64-apple-darwin'
info: override toolchain for '/Users/bchen/github/rustup.rs' set to 'nightly-x86_64-apple-darwin'

  nightly-x86_64-apple-darwin unchanged - rustc 1.40.0-nightly (702b45e40 2019-10-01)

info: downloading component 'rust-std' for 'wasm32-unknown-unknown'
 10.9 MiB /  10.9 MiB (100 %)   7.7 MiB/s in  3s ETA:  0s
info: installing component 'rust-std' for 'wasm32-unknown-unknown'
 10.9 MiB /  10.9 MiB (100 %)  10.0 MiB/s in  1s ETA:  0s

Confirm that the target is added in my rustup toolchain nightly folder

→ ls -la ~/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/wasm32-unknown-unknown/
total 0
drwxr-xr-x   3 bchen  CORP\Domain Users   96 Oct  6 21:54 .
drwxr-xr-x  19 bchen  CORP\Domain Users  608 Oct  6 21:54 ..
drwxr-xr-x  24 bchen  CORP\Domain Users  768 Oct  6 21:54 lib

src/cli/rustup_mode.rs

BeniCheni · 2019-10-03T05:14:30Z

Note: 97e6043 updated these:

added support for release channel support (i.e. "nightly")

→ rustup +stable which cargo
/Users/bchen/.rustup/toolchains/stable-x86_64-apple-darwin/bin/cargo

→ rustup +nightly which cargo                 
/Users/bchen/.rustup/toolchains/nightly-x86_64-apple-darwin/bin/cargo

→ rustup +foo which cargo                 
error: '+foo' isn't a valid value for '<release-channel>'
	[possible values: +beta, +nightly, +stable]


USAGE:
    rustup [FLAGS] [OPTIONS] [release-channel] <SUBCOMMAND>

For more information try --help

simplified some code based on clippy's suggestions
added tests

Per:

I believe there should be a warning

Would it be worth warning the user?

~~added warnings: (happy to edit the warning message to make better UX, if any suggestion)~~

→ rustup +stable --toolchain=nightly which cargo
warning: Both toolchain keyword (e.g. "+nightly") and "--toolchain" option are provided. The toolchain keyword would take precedence.
/Users/bchen/.rustup/toolchains/stable-x86_64-apple-darwin/bin/cargo

→ rustup --toolchain stable which rustc --toolchain=nightly
warning: Toolchain options are provided before and after the "which" subcommand. The first toolchain option would take precedence.
/Users/bchen/.rustup/toolchains/stable-x86_64-apple-darwin/bin/rustc

Update: current version does not need warning anymore.

src/cli/rustup_mode.rs

BeniCheni · 2019-10-05T07:26:46Z

It may be better to transform the +foo into --toolchain foo ...

@kinnison, sounds good. Updated per suggestion. PTAL at convenience? Thanks.

src/cli/rustup_mode.rs

BeniCheni · 2019-10-05T19:51:42Z

Why not just override toolchain instead of adding both toolchain-option and toolchain-keyword? There are supposed to be only one toolchain in effect though.

@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 (toolchain & keyword).

It may be better to transform the +foo into --toolchain foo ..._"

The current logic is to follow @kinnison's suggestion ☝️ . I think the assignment of let toolchain = if let ... else block in "src/cli/rustup_mode.rs" already transforms "+nightly" into toolchain.

If there's a way to omit Arg::with_name("keyword") setup, but still able to take +foo input from CLI, I'm game to edit. Currently, I couldn't figure out any version how to receive +foo input without the "keyword" arg.

src/cli/rustup_mode.rs

BeniCheni · 2019-10-07T02:31:48Z

pickfire
Why not just override toolchain in cfg and pass that in instead? We can keep the current version that way without adding a new parameter.

kinnison
That is a very good suggestion
then all commands will benefit since it's effectively altering the default toolchain entry from the config file.

@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 which. So the last push updated to just take "keyword" arg, and set override for the current dir accordingly.

I've tested w. the reported command from #1498: rustup +nightly target add wasm32-unknown-unknown, and am able to verify the "+nightly" keyword added the target in the correct rustup nightly toolchain path. Therefore, I think the PR is also resolving #1498 and edited the first comment of the PR w. reference. PTAL?

→ rustup +nightly target add wasm32-unknown-unknown
info: using existing install for 'nightly-x86_64-apple-darwin'
info: override toolchain for '/Users/bchen/github/rustup.rs' set to 'nightly-x86_64-apple-darwin'

  nightly-x86_64-apple-darwin unchanged - rustc 1.40.0-nightly (702b45e40 2019-10-01)

info: downloading component 'rust-std' for 'wasm32-unknown-unknown'
 10.9 MiB /  10.9 MiB (100 %)   7.7 MiB/s in  3s ETA:  0s
info: installing component 'rust-std' for 'wasm32-unknown-unknown'
 10.9 MiB /  10.9 MiB (100 %)  10.0 MiB/s in  1s ETA:  0s

→ ls -la ~/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/wasm32-unknown-unknown/
total 0
drwxr-xr-x   3 bchen  CORP\Domain Users   96 Oct  6 21:54 .
drwxr-xr-x  19 bchen  CORP\Domain Users  608 Oct  6 21:54 ..
drwxr-xr-x  24 bchen  CORP\Domain Users  768 Oct  6 21:54 lib

tests/cli-misc.rs

src/cli/rustup_mode.rs

BeniCheni · 2019-10-08T03:26:17Z

@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

pickfire
I wonder why are these changes necessary?

kinnison
Certainly this additional leading underscore isn't needed. However the conversion is needed afaict.

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 which test case.

Per src/cli/rustup_mode.rs

pickfire

I wonder if it would be helpful for the user experience if we use +keyword here instead.

kinnison
Better would be toolchain rather than keyword

Tried out toolchain, but collided with subcommand toolchain and panicked. No worries. Ended using a common-ground name +toolchain. Think it's contextual since the code strips the 1st character (i.e. "+") off to apply toolchain value. Hope it's okay w. you guys.

From an earlier comment

... we don't end up saving the different toolchain back if we do something (not expected, but possible) along the lines of rustup +another set profile default -> alters both profile and default_toolchain in ~/.rustup/settings.toml

Tested rustup +stable set profile default and verified content of settings.toml to ensure the default_toolchain is still nightly in the default profile.

→ rustup +stable set profile default
info: using existing install for 'stable-x86_64-apple-darwin'
info: override toolchain for '/Users/bchen/github/rustup.rs' set to 'stable-x86_64-apple-darwin'

  stable-x86_64-apple-darwin unchanged - rustc 1.38.0 (625451e37 2019-09-23)

info: profile set to 'default'

→ cat ~/.rustup/settings.toml
default_host_triple = "x86_64-apple-darwin"
default_toolchain = "nightly-x86_64-apple-darwin"
profile = "default"
version = "12"

[overrides]
"/Users/bchen/github/rustup.rs" = "stable-x86_64-apple-darwin"

kinnison

Looking good. Could you please add a test to assert that rustup +stable set profile minimal doesn't affect the default toolchain?

pickfire · 2019-10-08T16:25:06Z

@BeniCheni Nice, looks good to me. Thanks a lot for taking this on.

Just left the test that @kinnison mentioned.

BeniCheni · 2019-10-09T03:16:42Z

Looking good. Could you please add a test to assert that rustup +stable set profile minimal doesn't affect the default toolchain?

@kinnison, sure thing! Added tests per the helpful suggestion. Thank you. PTAL? 🙏

@BeniCheni Nice, looks good to me. Thanks a lot for taking this on.

Just left the test that @kinnison mentioned.

@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? 🙏

pickfire · 2019-10-09T16:31:07Z

@pickfire, thank you! The version wouldn't haven 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 cargo do the +nightly thing and I have no idea how to do that, I didn't know it is just as simple as

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

BeniCheni · 2019-10-10T00:10:35Z

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 println!(); is simply to just print a new line. It's used in override_add where the logic & UX override_by_toolchain is aligning to; thus, the same line is in override_by_toolchain:

https://github.com/rust-lang/rustup.rs/blob/b9e968b6656dd999eb09fb6dc8e7952677dd3fc2/src/cli/rustup_mode.rs#L1176

pickfire · 2019-10-10T01:43:38Z

@BeniCheni Ah, I saw that there will always be a println! preceding show_channel_update, maybe I will send a patch to put that in show_channel_update to avoid the duplication.

kinnison

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.

src/cli/rustup_mode.rs

BeniCheni · 2019-10-22T06:24:18Z

... With an ephemeral toolchain entry in Cfg, then OverrideReason can gain a CommandLine variant, and then in Cfg::find_override() Ben can introduce a check for that before testing self.env_override

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

# in ~/github/rustup.rs on git:which-toolchain-by-root-cmd
→ rustup +nightly target add wasm32-unknown-unknown
info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date

# in ~/github/rustup.rs on git:which-toolchain-by-root-cmd
→ ls -la ~/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/wasm32-unknown-unknown/
total 0
drwxr-xr-x   3 bchen  ___   96 Oct 22 01:56 .
drwxr-xr-x  17 bchen  ___  544 Oct 22 01:56 ..
drwxr-xr-x  24 bchen  ___  768 Oct 22 01:56 lib

Tested w. rustup +beta which cargo

# in ~/github/rustup.rs on git:which-toolchain-by-root-cmd
→ rustup +beta which cargo                         
/Users/bchen/.rustup/toolchains/beta-x86_64-apple-darwin/bin/cargo

kinnison

This is looking very good. Just a couple of minor niggles.

src/cli/rustup_mode.rs

src/config.rs

src/cli/rustup_mode.rs

BeniCheni · 2019-10-23T00:03:09Z

... a validator on this to ensure only values starting with a + get through

... if you added OverrideReason::CommandLine ...

@kinnison, nice suggestion for those UX tweaks. Added a validator, the OverrideReason::CommandLine and updated tests accordingly. PTAL? Thanks!

kinnison

This appears to behave as desired. 👍

Thank you very much for your efforts here.

BeniCheni force-pushed the which-toolchain-by-root-cmd branch 2 times, most recently from 63b814b to 1e075d7 Compare September 30, 2019 20:58

BeniCheni mentioned this pull request Oct 2, 2019

Allow +toolchain for rustup #1498

Closed

kinnison reviewed Oct 2, 2019

View reviewed changes

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved

BeniCheni force-pushed the which-toolchain-by-root-cmd branch 2 times, most recently from bd13a56 to 0aacf5f Compare October 3, 2019 04:24

BeniCheni changed the title ~~Support --toolchain option in front of "rustup which"~~ Support release channel (e.g. "+nightly") --toolchain option in front of "rustup which" Oct 3, 2019

BeniCheni force-pushed the which-toolchain-by-root-cmd branch from 0aacf5f to edfcb0c Compare October 3, 2019 05:12

BeniCheni force-pushed the which-toolchain-by-root-cmd branch from edfcb0c to 97e6043 Compare October 3, 2019 12:59

kinnison suggested changes Oct 4, 2019

View reviewed changes

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved

BeniCheni force-pushed the which-toolchain-by-root-cmd branch 2 times, most recently from 33fd8db to 7b7b736 Compare October 5, 2019 07:24

BeniCheni force-pushed the which-toolchain-by-root-cmd branch from 7b7b736 to a9d7dad Compare October 5, 2019 07:35

pickfire reviewed Oct 5, 2019

View reviewed changes

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved

BeniCheni force-pushed the which-toolchain-by-root-cmd branch from a9d7dad to 4ef8a1c Compare October 5, 2019 19:40

BeniCheni force-pushed the which-toolchain-by-root-cmd branch 3 times, most recently from 42a0693 to 7e697b0 Compare October 6, 2019 00:00

pickfire reviewed Oct 6, 2019

View reviewed changes

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved

BeniCheni force-pushed the which-toolchain-by-root-cmd branch 2 times, most recently from 546a9a6 to b0220fc Compare October 7, 2019 02:04

BeniCheni changed the title ~~Support release channel (e.g. "+nightly") --toolchain option in front of "rustup which"~~ Support toolchain keyword (e.g. "+nightly") to set override at root command Oct 7, 2019

BeniCheni force-pushed the which-toolchain-by-root-cmd branch 2 times, most recently from 796a20d to 94f639e Compare October 7, 2019 03:16

pickfire reviewed Oct 7, 2019

View reviewed changes

tests/cli-misc.rs Outdated Show resolved Hide resolved

pickfire reviewed Oct 7, 2019

View reviewed changes

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved

BeniCheni force-pushed the which-toolchain-by-root-cmd branch from 94f639e to 8b2dac1 Compare October 8, 2019 03:11

BeniCheni force-pushed the which-toolchain-by-root-cmd branch 2 times, most recently from f8560e4 to 3ff67f7 Compare October 8, 2019 04:04

kinnison suggested changes Oct 8, 2019

View reviewed changes

BeniCheni force-pushed the which-toolchain-by-root-cmd branch from 3ff67f7 to 48b5837 Compare October 9, 2019 03:10

kinnison suggested changes Oct 12, 2019

View reviewed changes

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved

BeniCheni force-pushed the which-toolchain-by-root-cmd branch 2 times, most recently from 99e07d0 to 6ef832f Compare October 22, 2019 06:20

kinnison suggested changes Oct 22, 2019

View reviewed changes

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved

src/config.rs Outdated Show resolved Hide resolved

src/cli/rustup_mode.rs Show resolved Hide resolved

Add toolchain override support on command line

3e7c66f

BeniCheni force-pushed the which-toolchain-by-root-cmd branch from 6ef832f to 3e7c66f Compare October 22, 2019 23:56

BeniCheni requested a review from kinnison October 23, 2019 00:29

kinnison approved these changes Oct 23, 2019

View reviewed changes

kinnison merged commit 7223b4f into rust-lang:master Oct 23, 2019

BeniCheni deleted the which-toolchain-by-root-cmd branch October 23, 2019 20:24

Aloxaf mentioned this pull request Mar 18, 2020

Duplicative auto completions #2268

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

BeniCheni commented Sep 30, 2019 •

edited

Loading

BeniCheni commented Oct 3, 2019 •

edited

Loading

BeniCheni commented Oct 5, 2019 •

edited

Loading

BeniCheni commented Oct 5, 2019 •

edited

Loading

BeniCheni commented Oct 7, 2019 •

edited

Loading

BeniCheni commented Oct 8, 2019 •

edited

Loading

kinnison left a comment

pickfire commented Oct 8, 2019

BeniCheni commented Oct 9, 2019 •

edited

Loading

pickfire commented Oct 9, 2019

BeniCheni commented Oct 10, 2019

pickfire commented Oct 10, 2019

kinnison left a comment

BeniCheni commented Oct 22, 2019 •

edited

Loading

kinnison left a comment

BeniCheni commented Oct 23, 2019

kinnison left a comment

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

BeniCheni commented Sep 30, 2019 • edited Loading

Test w. rustup +nightly target add wasm32-unknown-unknown

Confirm that the target is added in my rustup toolchain nightly folder

BeniCheni commented Oct 3, 2019 • edited Loading

BeniCheni commented Oct 5, 2019 • edited Loading

BeniCheni commented Oct 5, 2019 • edited Loading

BeniCheni commented Oct 7, 2019 • edited Loading

BeniCheni commented Oct 8, 2019 • edited Loading

kinnison left a comment

Choose a reason for hiding this comment

pickfire commented Oct 8, 2019

BeniCheni commented Oct 9, 2019 • edited Loading

pickfire commented Oct 9, 2019

BeniCheni commented Oct 10, 2019

pickfire commented Oct 10, 2019

kinnison left a comment

Choose a reason for hiding this comment

BeniCheni commented Oct 22, 2019 • edited Loading

kinnison left a comment

Choose a reason for hiding this comment

BeniCheni commented Oct 23, 2019

kinnison left a comment

Choose a reason for hiding this comment

BeniCheni commented Sep 30, 2019 •

edited

Loading

Test w. `rustup +nightly target add wasm32-unknown-unknown`

BeniCheni commented Oct 3, 2019 •

edited

Loading

BeniCheni commented Oct 5, 2019 •

edited

Loading

BeniCheni commented Oct 5, 2019 •

edited

Loading

BeniCheni commented Oct 7, 2019 •

edited

Loading

BeniCheni commented Oct 8, 2019 •

edited

Loading

BeniCheni commented Oct 9, 2019 •

edited

Loading

BeniCheni commented Oct 22, 2019 •

edited

Loading