rustup icon indicating copy to clipboard operation
rustup copied to clipboard

refactor(cli): rewrite `rustup` and `rustup-init` with `clap_derive`

Open rami3l opened this issue 1 year ago • 8 comments

Closes #3549.

Currently, to add a new subcommand/argument to rustup or rustup-init, one will have to manually maintain consistencies across 3 places (Command, ArgMatches and dispatching). Failing to do so will unavoidably lead to panics.

Hopefully, with clap_derive, it'll be much easier to dive in the codebase and make changes, as:

  • All the subcommands/arguments will be explicit (not hidden in the specific dispatching functions, like run() for rustup run);
  • To add a new subcommand/argument there are no more 3, but only 2 places to change (Parser and dispatching), and this time one will be covered by strongly-typed APIs.

Initially, I decided to experiment with rustup-init since it's relatively isolated, and I was surprised to find out that even with such a simple command, migrating to clap_derive already allows us to address some inconsistencies. Given that successful experience, I think a complete migration will be not only feasible but fruitful, which can hopefully reduce accidental complexities in maintaining this project.

PS: Please don't merge this before closing https://github.com/rust-lang/rustup/issues/3501.


Progress

  • [x] rustup-init
  • [x] rustup
    • [x] rustup install**
    • [x] rustup uninstall**
    • [x] rustup dump-testament**
    • [x] rustup show*
    • [x] rustup update
    • [x] rustup check
    • [x] rustup default
    • [x] rustup toolchain*
    • [x] rustup target*
    • [x] rustup component*
    • [x] rustup override*
    • [x] rustup run
    • [x] rustup which
    • [x] rustup doc
    • [x] rustup man
    • [x] rustup self*
    • [x] rustup set*
    • [x] rustup completions

*: This command has subcommands. **: This command is hidden.

rami3l avatar Dec 21 '23 01:12 rami3l

Thanks for working on this!

Please don't merge this before closing #3501.

Why do you want to keep this out of 1.27.0? It maybe doesn't need to go in, but I don't see any harm from it being merged?

(Is that the only reason this is in Draft state, or are there other reasons?)

djc avatar Dec 21 '23 08:12 djc

Why do you want to keep this out of 1.27.0? It maybe doesn't need to go in, but I don't see any harm from it being merged?

(Is that the only reason this is in Draft state, or are there other reasons?)

@djc Oh, it's the only reason actually. IMO the overall migration is a huge amount of work that consists of multiple patches and I think it's quite weird to ship a new version half-migrated.

I plan to do this on a per-subcommand basis when it comes to rustup, but since rustup-init is quite self-contained I think it's also possible to merge it if the rest of the team thinks it's okay?

rami3l avatar Dec 21 '23 11:12 rami3l

I can see how it might be weird to ship half-migrated so I guess I agree maybe we shouldn't merge this yet until we have the other CLIs migrated.

djc avatar Dec 22 '23 10:12 djc

A reminder for myself: don't rebase this PR until #3644 gets merged.

rami3l avatar Jan 19 '24 12:01 rami3l

@djc How you would like to see my rustup porting history?

I guess porting rustup-init is quite independent and deserves its own commit.

However, porting rustup is a long and gradual process, requiring hacks and scaffolds here and there which are added to glue the code back together only to be removed later etc. which IMHO is not very interesting for reviewing purposes. Looking at your #3444, maybe it's reasonable to just squash all those changes into one big commit in the end?

rami3l avatar Jan 21 '24 09:01 rami3l

As long as there is not a lot of back and forth, I think your current commit history of doing one subcommand at a time looks pretty good actually -- I think this will make it fairly straightforward to review. Looks like you're making good progress!

djc avatar Jan 22 '24 12:01 djc

@djc Just found out that our clap builder broke rustfmt, so before my simplifying that that part of the code, our source file wasn't really being formatted.

I feel a bit guilty right now about using auto-formatting on save to cause weird diffs...

PS: Also, to my surprise, will all the scaffolds removed in the end, the whole rustup refactor is not that hard to read in one piece.

rami3l avatar Jan 23 '24 02:01 rami3l

A reminder for myself: don't rebase this PR until #3650 gets merged.

rami3l avatar Jan 25 '24 03:01 rami3l