Upgrade to clap 4 #3444

djc · 2023-08-15T08:25:42Z

Review the changelog here: https://github.com/clap-rs/clap/blob/master/CHANGELOG.md#400---2022-09-28.

Previously: #3094, which seems stalled.

Fixes #3443 and, apparently, fixes #3128.

src/cli/rustup_mode.rs

djc · 2023-08-15T11:39:58Z

The rustup_init_ui_doc_text_tests failure is confusing to me, I tried to get it lined up but the way the test fails doesn't quite make sense to me.

rbtcollins · 2023-08-15T12:13:13Z

Thank you for taking this on.

The first line is an expected line - rustup-init ...
The second line The installer for rustup is line 1 of the clap4 output and line 2 of the expected output.

Line 3 -Usage ... is new in the clap4 output
Line 4 + USAGE: is expected but not in the clap4 output

etc.

We're ok with changes to the help layout as long as:

its internally consistent between subcommands and the like
it is clean and readable

There was discussion about this during the clap3 upgrade - I forget whether on discord or in the clap3 PR (perhaps in both in fact).

---- expected: stdout
++++ actual:   stdout
        1 + rustup-init 1.26.0 (577bf51ae 2023-04-05)
   1    2 | The installer for rustup
   2    3 | 
   3      - Usage: rustup-init [OPTIONS]
        4 + USAGE:
        5 +     rustup-init [OPTIONS]
        6 + 
        7 + OPTIONS:
        8 +     -v, --verbose
        9 +             Enable verbose output
       10 + 
       11 +     -q, --quiet
       12 +             Disable progress output
       13 + 
       14 +     -y
       15 +             Disable confirmation prompt.
       16 + 
       17 +         --default-host <default-host>
       18 +             Choose a default host triple
       19 + 
       20 +         --default-toolchain <default-toolchain>
       21 +             Choose a default toolchain to install. Use 'none' to not install any toolchains at all
       22 + 
       23 +         --profile <profile>
       24 +             [default: default] [possible values: minimal, default, complete]
       25 + 
       26 +     -c, --component <components>...
       27 +             Component name to also install
       28 + 
       29 +     -t, --target <targets>...
       30 +             Target name to also install
       31 + 
       32 +         --no-update-default-toolchain
       33 +             Don't update any existing default toolchain after install
       34 + 
       35 +         --no-modify-path
       36 +             Don't configure the PATH environment variable
       37 + 
       38 +     -h, --help
       39 +             Print help information
   4   40 | 
   5      - Options:
   6      -   -v, --verbose
   7      -           Enable verbose output
   8      -   -q, --quiet
   9      -           Disable progress output
  10      -   -y
  11      -           Disable confirmation prompt.
  12      -       --default-host <default-host>
  13      -           Choose a default host triple
  14      -       --default-toolchain <default-toolchain>
  15      -           Choose a default toolchain to install. Use 'none' to not install any toolchains at all
  16      -       --profile <profile>
  17      -           [default: default] [possible values: minimal, default, complete]
  18      -   -c, --component <components>...
  19      -           Component name to also install
  20      -   -t, --target <targets>...
  21      -           Target name to also install
  22      -       --no-update-default-toolchain
  23      -           Don't update any existing default toolchain after install
  24      -       --no-modify-path
  25      -           Don't configure the PATH environment variable
  26      -   -h, --help
  27      -           Print help
  28      -   -V, --version
  29      -           Print version
       41 +     -V, --version
       42 +             Print version information

Cargo.lock

rbtcollins · 2023-08-15T12:25:46Z

One other thing, once the tests are passing and we've figured out everything so the output won't churn on you, please also update the bootstrap shell scripts' help to match - we keep them in sync for visual consistency.

src/cli/rustup_mode.rs

rbtcollins · 2023-08-15T12:52:37Z

The changes look sane, though I'm no clap expert, so I defer to you on all that!

The only caveat that isn't looking ok is the removal of the rustup version details from the top of the help. Its not necessarily a show stopper, but it does make me nervous, sometimes we have behavioural expectations in the oddest places.

If its easy to re-instate that, that would be great. Otherwise if we can check that the version shows up somewhere in the help, that would be sufficient.

djc · 2023-08-16T12:14:12Z

Addressed your suggestions, including adding back the version to the rustup-init help.

Xenfo · 2023-08-16T14:24:37Z

Hey @djc, I've tried this PR out as you suggested on #3156 and completions are completely broken now on ZSH.

rustup on  clap-4 is 📦 v1.26.0 via 🦀 v1.71.1 
❯ rustup
_rustup:850: parse error near `\n'
_rustup:850: parse error near `\n'
_rustup:850: parse error near `\n'

djc · 2023-08-16T14:46:21Z

@Xenfo did you reinstall the completions? I personally don't have any experience with shell completion setup stuff, but all I did here was change a version number for the clap_complete crate.

Xenfo · 2023-08-16T14:47:20Z

Yes I did, home/bin/rustup completions zsh >> ~/.oh-my-zsh/completions/_rustup.

Xenfo · 2023-08-16T14:51:15Z

Ignore what I said, looks like it was appending to the completion file. That issue is fixed but the issue from #3156 is still present.

Xenfo · 2023-08-16T14:59:15Z

It looks like it works after providing 2 main commands.

Xenfo · 2023-08-16T16:38:16Z

I've tracked the issue down to https://github.com/rust-lang/rustup/blob/master/src/cli/rustup_mode.rs#L268-L278. Removing that and https://github.com/rust-lang/rustup/blob/master/src/cli/rustup_mode.rs#L147-L149 causes completions to work as expected.

Edit: it's because of the argument before the command, it causes completions not to know if the first command was specified for the toolchain or as an actual command.

rbtcollins · 2023-08-16T16:48:12Z

tests/suite/cli_ui.rs

+        assert_eq!(
+            rustup_init_help_std_out.stdout.unwrap(),
+            rustup_init_sh_help_std_out.stdout.unwrap()
+        )


I have no objection here, but I'm curious why the change

Just trying to reduce the noise from this. We should probably use something like the similar-asserts crate to make the errors from this less inscrutable.

Thats a great suggestion. Feel free to add it in incrementally where you need it.

djc · 2023-08-16T15:30:43Z

tests/suite/cli-ui/rustup/rustup_only_options_stdout.toml

-The Rust toolchain installer
+stdout = ""
+stderr = """
+error: error: 'rustup' requires a subcommand but one was not provided


@rbtcollins any clue why the [..] is not doing the right thing here?

This behaviour is surprising. rustup -v should print the version and basic help, not an error about subcommand selection.

Good catch, clap-rs/clap#5076.

I agree with @epage's point in clap-rs/clap#5076 (reply in thread) and I don't even know what rustup -v is supposed to do personally... 👀

If this test had instead used -y or --no-modify-path, would the answer be any different of what this should do?

The metadata output is the result of -v having effect; I suggest changing to -q for the test, which is effectively guaranteed to never be adding more output.

@epage I can appreciate the sentiment about engineering effort / reward. But it seems to me that you take issue with the value judgement not the analysis - you haven't disputed the historical behaviour, argued that the error message is easily understood or proposed alternative ways forward.

So if the question is about the relative merits of option 2 vs option 3, what I can say right now is that rustup is under-resourced in terms of human capacity. And triaging and improving non-functional bugs takes quite a bit of time. We could of course say 'if it isn't a functional problem, we will just ignore/close such bugs'.... but I don't feel that that would be a good way of upholding the excellent work that preceeded my joining the project a few years back.

Perceptions of quality in software often hinge on non-happy-path, non-functional-requirements. We know that at least some people went and tried this path, or we wouldn't have the additional test added to cover it. With it covered, we won't get another bug report about it, which is good.

And @djc seems to have quite rapidly figured out and solved the problem, so thats nice.

Well, CI is still failing, which is not so nice.

I take issue with the analysis itself but since it started at the root of the analysis, I didn't go further into it. I didn't just remove SubcommandRequiredElseHelp for code size and binary size.

And for a project that is under resourced, flowing against the stream seems like it'd be the opposite of what you'd want to be doing. I'm not saying this in terms of accepting bugs but of practices

I think it depends on where we think our time is going no? We're getting quite meta at this point though.

tests/suite/cli-ui/rustup/rustup_only_options_stdout.toml

epage · 2023-08-22T14:02:12Z

rustup-init.sh

@@ -26,47 +26,36 @@ RUSTUP_UPDATE_ROOT="${RUSTUP_UPDATE_ROOT:-https://static.rust-lang.org/rustup}"
 usage() {
    cat <<EOF
 rustup-init 1.26.0 (577bf51ae 2023-04-05)


The only caveat that isn't looking ok is the removal of the rustup version details from the top of the help. Its not necessarily a show stopper, but it does make me nervous, sometimes we have behavioural expectations in the oddest places.

If its easy to re-instate that, that would be great. Otherwise if we can check that the version shows up somewhere in the help, that would be sufficient.

If people are relying on parsing --help output, we will break them; there are no guarentees.

We intentionally dropped this to recover vertical space. If you want precedence / consistency with other rust commands, cargo does not include the version.

epage · 2023-08-22T14:06:35Z

src/cli/rustup_mode.rs

                .value_parser(|s: &str| {
+                    use clap::{Error, error::ErrorKind};
                    if let Some(stripped) = s.strip_prefix('+') {
-                        ResolvableToolchainName::try_from(stripped).map_err(|e| clap::Error::raw(clap::ErrorKind::InvalidValue, e))
+                        ResolvableToolchainName::try_from(stripped).map_err(|e| Error::raw(ErrorKind::InvalidValue, e))
                    } else {
-                        Err(clap::Error::raw(clap::ErrorKind::InvalidSubcommand, format!("\"{s}\" is not a valid subcommand, so it was interpreted as a toolchain name, but it is also invalid. {TOOLCHAIN_OVERRIDE_ERROR}")))
+                        Err(Error::raw(ErrorKind::InvalidSubcommand, format!("\"{s}\" is not a valid subcommand, so it was interpreted as a toolchain name, but it is also invalid. {TOOLCHAIN_OVERRIDE_ERROR}")))
                    }


Honestly, I'm surprised this works. The intention was to support any user errors. I'm surprised that clap errors render well when nested. I wouldn't be surprised if we accidentally break this

What do you suggest we do instead?

Use any normal error type that implements Error

Ok, and - just checking for complete clarity - so I don't need to use clap error types in those helpers?

Correct. If curious, this is the relevant trait implementation you need to match

epage · 2023-08-22T14:10:16Z

I've tracked the issue down to https://github.com/rust-lang/rustup/blob/master/src/cli/rustup_mode.rs#L268-L278. Removing that and https://github.com/rust-lang/rustup/blob/master/src/cli/rustup_mode.rs#L147-L149 causes completions to work as expected.

Edit: it's because of the argument before the command, it causes completions not to know if the first command was specified for the toolchain or as an actual command.

FYI completest now exists for testing of completions. clap has adopted it to test the completion generators. I'd like to adopt it in cargo to test our hand written completions but haven't yet.

Also, clap is (slowly) working towards completions being implemented mostly in Rust which should offer more correctness and flexibility

rbtcollins · 2023-08-22T14:59:00Z

failure windows dev:

---- expected: stderr
++++ actual:   stderr
   1    1 | rustup [..]
   2    2 | 
   3    3 | The Rust toolchain installer
   4    4 | 
  5      - Usage: rustup[EXE] [OPTIONS] [+toolchain] [COMMAND]
   5 + Usage: rustup [OPTIONS] [+toolchain] [COMMAND]
   6    6 | 
   7    7 | Commands:
   8    8 |   show         Show the active and installed toolchains or profiles
   9    9 |   update       Update Rust toolchains and rustup
  10   10 |   check        Check for updates to Rust toolchains and rustup
          ⋮
  47   47 |     for common platforms.
  48   48 | 
  49   49 |     If you are new to Rust consider running `rustup doc --book` to
  50   50 |     learn Rust.
  51   51 |

mac is a flake.

Linux is

---- expected: stderr
++++ actual:   stderr
   1      - rustup [..]
   2      - 
   ...
  51      - 
        1 + error: could not create home directory: '/.rustup': Permission denied (os error 13)

rbtcollins · 2023-08-22T15:06:09Z

tests/suite/cli-ui/rustup/rustup_only_options_stdout.toml

-    run            Run a command with an environment configured for a given toolchain
-    which          Display which binary will be run for a given command
-    doc            Open the documentation for the current toolchain
+Usage: rustup[EXE] [OPTIONS] [+toolchain] [COMMAND]


I'm not sure why, but the help for this particular case seems to be skipping .exe on Windows. Try this suggestion?

Suggested change

Usage: rustup[EXE] [OPTIONS] [+toolchain] [COMMAND]

Usage: rustup [OPTIONS] [+toolchain] [COMMAND]

rbtcollins · 2023-08-22T15:23:41Z

At a guess I think the linux error is coming from

rustup/src/config.rs

Lines 198 to 199 in 2fa788a

	utils::ensure_dir_exists("home", &rustup_dir, notify_handler.as_ref())?;

- that is from_env() is being called for some reason.

The test suite that runs the trycmd stuff isn't setting up isolated environments (HOME temp dir, environment variables) so this looks like an interaction with the global environment, and could well be super racy.

That said the reason this is triggering that code path isn't too obvious to me. I'll see if get time this weekend to checkout the branch and poke around.

edit: noticed it is '/.rustup' so this is running with HOME=/ I think. Perhaps trycmd does that automatically.

djc · 2023-08-22T15:56:58Z

All in all I feel like these trycmd tests are superflaky and hard to work with.

rbtcollins · 2023-08-25T17:33:08Z

All in all I feel like these trycmd tests are superflaky and hard to work with.

They are certainly sensitive to changes, possibly because we've only just added them this year and not had much time to figure out where we want our sensitivity and where not.

I'm not sure that they are flaky: I don't recall a single CI run that failed nondeterministically because of them.

djc · 2023-08-28T07:49:21Z

Sure, I guess "overly sensitive" is more accurate than flaky.

rbtcollins · 2023-08-29T19:50:48Z

I've fixed the failure on windows.

On linux I've validated that it is a hermiticity issue leading to the test error. Prepping a separate patch for that.

rbtcollins · 2023-08-29T20:28:02Z

#3470 should fix the isolation failure. I haven't figured out yet what this patch is doing that introduces the assertion of settings.toml, though probably we should look into that - writing to the settings file when nothing is being explicitly changed is a bit odd.

The overridden binary name means that the [EXE] override is not required in the usage output.

Upgrade to clap 4

djc commented Aug 15, 2023

View reviewed changes

src/cli/rustup_mode.rs Show resolved Hide resolved

djc force-pushed the clap-4 branch from edd63ca to 70ab7c0 Compare August 15, 2023 09:54

rbtcollins reviewed Aug 15, 2023

View reviewed changes

Cargo.lock Show resolved Hide resolved

rbtcollins reviewed Aug 15, 2023

View reviewed changes

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

rbtcollins reviewed Aug 15, 2023

View reviewed changes

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

djc force-pushed the clap-4 branch from 70ab7c0 to 41d288c Compare August 16, 2023 12:12

djc force-pushed the clap-4 branch from 41d288c to 56bdd58 Compare August 16, 2023 12:14

djc mentioned this pull request Aug 16, 2023

Avoid printing trailing whitespace in help output clap-rs/clap#5074

Open

2 tasks

djc force-pushed the clap-4 branch from 56bdd58 to 4ea07e6 Compare August 16, 2023 13:53

djc mentioned this pull request Aug 16, 2023

Duplicative auto completions #3156

Open

djc force-pushed the clap-4 branch from 1bb5ddc to 0988a51 Compare August 16, 2023 15:02

rbtcollins reviewed Aug 16, 2023

View reviewed changes

djc commented Aug 16, 2023

View reviewed changes

djc force-pushed the clap-4 branch 4 times, most recently from 92cb005 to 1063602 Compare August 21, 2023 14:50

rbtcollins reviewed Aug 22, 2023

View reviewed changes

tests/suite/cli-ui/rustup/rustup_only_options_stdout.toml Outdated Show resolved Hide resolved

djc force-pushed the clap-4 branch from 1063602 to 2eb3c2f Compare August 22, 2023 09:46

epage reviewed Aug 22, 2023

View reviewed changes

rbtcollins reviewed Aug 22, 2023

View reviewed changes

djc and others added 4 commits August 29, 2023 22:42

Avoid deprecated clap API

61f9133

Upgrade to clap 4

618530a

Bring additional help section style in line with clap 4

ab8835b

Fix rustup_only_options_stdout

6954674

The overridden binary name means that the [EXE] override is not required in the usage output.

rbtcollins force-pushed the clap-4 branch from 46089e2 to 6954674 Compare August 29, 2023 20:42

rbtcollins approved these changes Aug 29, 2023

View reviewed changes

rbtcollins merged commit 3333cdd into rust-lang:master Aug 29, 2023
15 checks passed

rami3l mentioned this pull request Sep 26, 2023

Cargo and rustc are using different tones of red for errors on the terminal rust-lang/cargo#12740

Open

This was referenced Oct 5, 2023

Bump clap to 4.0 #3094

Closed

1.27.0 release planning #3501

Closed

AJ-Ianozi pushed a commit to AJ-Ianozi/getada-download that referenced this pull request Oct 12, 2023

Merge pull request rust-lang#3444 from djc/clap-4

d1197c3

Upgrade to clap 4

rami3l mentioned this pull request Jan 21, 2024

refactor(cli): rewrite rustup with clap_derive #3596

Merged

20 tasks

rami3l mentioned this pull request Jan 29, 2024

dist: bump rustup version to 1.27.0 #3653

Merged

1 task

rami3l mentioned this pull request May 25, 2024

Improve PowerShell completion setup documentation #3131

Open

	Usage: rustup[EXE] [OPTIONS] [+toolchain] [COMMAND]
	Usage: rustup [OPTIONS] [+toolchain] [COMMAND]

Upgrade to clap 4 #3444

Upgrade to clap 4 #3444

Conversation

djc commented Aug 15, 2023 • edited Loading

djc commented Aug 15, 2023

rbtcollins commented Aug 15, 2023

rbtcollins commented Aug 15, 2023

rbtcollins commented Aug 15, 2023

djc commented Aug 16, 2023

Xenfo commented Aug 16, 2023

djc commented Aug 16, 2023

Xenfo commented Aug 16, 2023

Xenfo commented Aug 16, 2023

Xenfo commented Aug 16, 2023

Xenfo commented Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtcollins Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

djc Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djc Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtcollins Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtcollins Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage commented Aug 22, 2023

rbtcollins commented Aug 22, 2023

rbtcollins Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

rbtcollins commented Aug 22, 2023 • edited Loading

djc commented Aug 22, 2023

rbtcollins commented Aug 25, 2023

djc commented Aug 28, 2023

rbtcollins commented Aug 29, 2023

rbtcollins commented Aug 29, 2023

djc commented Aug 15, 2023 •

edited

Loading

Xenfo commented Aug 16, 2023 •

edited

Loading

rbtcollins Aug 17, 2023 •

edited

Loading

djc Aug 17, 2023 •

edited

Loading

djc Aug 22, 2023 •

edited

Loading

rbtcollins Aug 22, 2023 •

edited

Loading

rbtcollins Aug 25, 2023 •

edited

Loading

rbtcollins Aug 22, 2023 •

edited

Loading

rbtcollins commented Aug 22, 2023 •

edited

Loading