Skip to content
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

refactor(cli): rewrite rustup with clap_derive #3596

Merged
merged 13 commits into from
May 12, 2024
Merged

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented Dec 21, 2023

Closes #3549.

Rationale

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

Progress

  • rustup-init (became refactor(cli): rewrite rustup-init with clap_derive #3814)
  • rustup
    • rustup install 12
    • rustup uninstall 12
    • rustup dump-testament 1
    • rustup show 3
    • rustup update
    • rustup check
    • rustup default
    • rustup toolchain 3
    • rustup target 3
    • rustup component 3
    • rustup override 3
    • rustup run
    • rustup which
    • rustup doc
    • rustup man
    • rustup self 3
    • rustup set 3
    • rustup completions

Footnotes

  1. This command is hidden. 2 3

  2. This command is implemented as an alias of the corresponding subcommand under rustup toolchain. 2

  3. This command has subcommands. 2 3 4 5 6 7

@rami3l rami3l force-pushed the refactor/clap-derive branch from c146305 to baf8b69 Compare December 21, 2023 02:26
@rami3l rami3l changed the title refactor(cli): rewrite argument parsing with clap_derive refactor(cli): rewrite rustup-init with clap_derive Dec 21, 2023
@rami3l rami3l force-pushed the refactor/clap-derive branch 2 times, most recently from 26ca09e to 39e49e0 Compare December 21, 2023 04:44
@djc
Copy link
Contributor

djc commented Dec 21, 2023

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?)

@rami3l
Copy link
Member Author

rami3l commented Dec 21, 2023

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?

src/cli/setup_mode.rs Outdated Show resolved Hide resolved
src/cli/setup_mode.rs Outdated Show resolved Hide resolved
src/toolchain/names.rs Outdated Show resolved Hide resolved
@djc
Copy link
Contributor

djc commented Dec 22, 2023

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.

@rami3l rami3l force-pushed the refactor/clap-derive branch from 39e49e0 to 55a1861 Compare December 22, 2023 14:29
@rami3l rami3l added this to the 1.28.0 milestone Jan 17, 2024
@rami3l rami3l force-pushed the refactor/clap-derive branch from 55a1861 to 0d85d0f Compare January 19, 2024 10:31
src/cli/setup_mode.rs Outdated Show resolved Hide resolved
@rami3l

This comment was marked as outdated.

@rami3l rami3l force-pushed the refactor/clap-derive branch 2 times, most recently from 1d30214 to 5dcbd32 Compare January 19, 2024 13:19
@rami3l rami3l changed the title refactor(cli): rewrite rustup-init with clap_derive refactor(cli): rewrite rustup and rustup-init with clap_derive Jan 20, 2024
@rami3l rami3l force-pushed the refactor/clap-derive branch 5 times, most recently from 63b7b41 to 30e959c Compare January 21, 2024 09:24
@rami3l
Copy link
Member Author

rami3l commented Jan 21, 2024

@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 rami3l force-pushed the refactor/clap-derive branch 5 times, most recently from 5d389e7 to 8a22d57 Compare January 22, 2024 12:20
@djc
Copy link
Contributor

djc commented Jan 22, 2024

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!

@rami3l
Copy link
Member Author

rami3l commented Jan 23, 2024

@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 rami3l added this pull request to the merge queue May 12, 2024
@rami3l rami3l removed this pull request from the merge queue due to a manual request May 12, 2024
@rami3l rami3l force-pushed the refactor/clap-derive branch from f8e9756 to 8fe3c00 Compare May 12, 2024 12:52
@rami3l rami3l force-pushed the refactor/clap-derive branch from f175f18 to 986b302 Compare May 12, 2024 12:56
@rami3l rami3l enabled auto-merge May 12, 2024 12:56
@rami3l rami3l added this pull request to the merge queue May 12, 2024
Merged via the queue into master with commit fcff18c May 12, 2024
22 checks passed
@rami3l rami3l deleted the refactor/clap-derive branch May 12, 2024 13:30
@rami3l rami3l restored the refactor/clap-derive branch May 12, 2024 14:13
@rami3l rami3l deleted the refactor/clap-derive branch July 31, 2024 11:51
@rami3l rami3l mentioned this pull request Oct 6, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using clap_derive for argument parsing
2 participants