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.

Sign up for GitHub

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

Jump to bottom

Migrate x to using clap #108083

Closed
wants to merge 2 commits into from
Closed

Migrate x to using clap #108083

wants to merge 2 commits into from

Conversation

clubby789
Copy link
Contributor

@clubby789 clubby789 commented Feb 15, 2023 •
edited
Loading

cc #107375, #107827, @jyn514

Beginning experimenting with moving src/bootstrap/flags.rs to clap instead of getopts. A fair chunk of logic is missing but this is basically usable right now, so I'm making a draft PR to have it available.

@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2023

r? @albertlarsan68

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Feb 15, 2023
@jyn514
Copy link
Member

jyn514 commented Feb 15, 2023

cc @Mark-Simulacrum - this adds a few more dependencies, but makes it easier to maintain the arguments and add features like shell completion.

@clubby789 can you time x --help before and after this change to see how much of a slow down there is?

@petrochenkov
Copy link
Contributor

@jyn514

can you time x --help before and after this change to see how much of a slow down there is?

If you mean the time including compiling bootstrap including clap, then I'd very much want to see that too.

@jyn514
Copy link
Member

jyn514 commented Feb 15, 2023

yes, that's what I meant, with an x clean before so it's rebuilt from scratch.

@clubby789
Copy link
Contributor Author

rm -rf build/bootstrap && time x --help
master:
x --help 42.59s user 3.94s system 360% cpu 12.914 total
x-clap
x --help 52.26s user 4.70s system 323% cpu 17.625 total

@clubby789
Copy link
Contributor Author

I'm not sure how to handle the old behaviour of x <command> -h -v for listing paths, since -h causes the parser to fail. Possibly we could handle the failure, then iterate through the commands looking for a subccomand, --help/-h and --verbose/-{v,vv,vvv} but that doesn't feel too neat

@jyn514
Copy link
Member

jyn514 commented Feb 15, 2023

@clubby789 we could look for -h -v before passing the output to clap; it might not be 100% accurate because of --test-args but in practice I think it will work

klensy
klensy reviewed Feb 15, 2023
View reviewed changes
src/bootstrap/Cargo.toml Outdated Show resolved Hide resolved
@clubby789 clubby789 force-pushed the x-clap branch 2 times, most recently from aca8c02 to 58f4930 Compare February 15, 2023 22:54
@clubby789
Copy link
Contributor Author

Given that we have clap now, I decided to just have a local Parser struct we can use to make sure the parsing logic is the same.
This works enough now (CI runs sucessfully) that I'm going to mark ready for review, but unsure if I'm missing any edge cases/behaviour.

@clubby789 clubby789 marked this pull request as ready for review February 15, 2023 22:55
@albertlarsan68
Copy link
Member

Hello,
I have not yet fully reviewed the PR. Here is the main thing I noticed: Clap has a way to test the correctness of the code (https://docs.rs/clap/latest/clap/_derive/index.html#tips), but I did not see it being used. Can you add it please (as a test)?

@albertlarsan68 albertlarsan68 mentioned this pull request Feb 16, 2023
Merged
@clubby789 clubby789 force-pushed the x-clap branch 2 times, most recently from d449e4c to f1e1008 Compare February 16, 2023 18:54
@clubby789 clubby789 mentioned this pull request Feb 16, 2023
Closed
@clubby789
Copy link
Contributor Author

I'm working on integrating clap_complete for automatic shell completions, but unfortunately it seems a little buggy with subcommands. clap-rs/clap#4652 is one issue I've encountered, as well as zsh erroring due to positional arguments being repeated in all subcommands

@bors
Copy link
Contributor

bors commented Feb 17, 2023

☔ The latest upstream changes (presumably #108145) made this pull request unmergeable. Please resolve the merge conflicts.

@clubby789 clubby789 force-pushed the x-clap branch 2 times, most recently from 53bed61 to 67a2533 Compare February 17, 2023 12:36
@clubby789 clubby789 mentioned this pull request Feb 25, 2023
Closed
@bors
Copy link
Contributor

bors commented Feb 26, 2023

☔ The latest upstream changes (presumably #108464) made this pull request unmergeable. Please resolve the merge conflicts.

@albertlarsan68 albertlarsan68 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2023
@albertlarsan68 albertlarsan68 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 26, 2023
@albertlarsan68 albertlarsan68 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 7, 2023
@bors
Copy link
Contributor

bors commented Mar 14, 2023

☔ The latest upstream changes (presumably #109097) made this pull request unmergeable. Please resolve the merge conflicts.

@est31 est31 mentioned this pull request Mar 21, 2023
Open
bjorn3
bjorn3 reviewed Apr 22, 2023
View reviewed changes
src/bootstrap/Cargo.toml
cc = "1.0.69"
clap = { version = "4.1.4", features = ["std", "usage", "help", "derive", "error-context"], default-features = false}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How bad would it be to manually implement rather than use the derive macro to reduce the compile time impact?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should focus so heavily on the compile times. If they're an issue, we should finally fix #99989 , which is very close to being done.

@clubby789 clubby789 mentioned this pull request Apr 22, 2023
Merged
@clubby789
Copy link
Contributor Author

Closing in favour of #110693

@clubby789 clubby789 closed this Apr 22, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2023
@bors
Auto merge of rust-lang#110693 - clubby789:x-clap-take-2, r=Mark-Simu…
70a779c 
…lacrum

Migrate bootstrap to Clap-based argument parsing

Supercedes rust-lang#108083

I chose to re-do the work rather than rebase the onto the large changes since the original PR. If it's preferred I can instead force-push the original PR to this version.

cc `@jyn514` `@albertlarsan68`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

@bjorn3 bjorn3 bjorn3 left review comments

@klensy klensy klensy left review comments

Assignees

@albertlarsan68 albertlarsan68

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@clubby789 @rustbot @jyn514 @petrochenkov @albertlarsan68 @bors @klensy @bjorn3