rustup
rustup copied to clipboard
refactor(cli): rewrite `rustup` and `rustup-init` with `clap_derive`
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()
forrustup 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.
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?)
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?
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.
A reminder for myself: don't rebase this PR until #3644 gets merged.
@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?
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 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.
A reminder for myself: don't rebase this PR until #3650 gets merged.