Skip to content

Migrate x to using clap#108083

Closed
clubby789 wants to merge 2 commits intorust-lang:masterfrom
clubby789:x-clap
Closed

Migrate x to using clap#108083
clubby789 wants to merge 2 commits intorust-lang:masterfrom
clubby789:x-clap

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

petrochenkov commented Feb 15, 2023

@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

clubby789 commented Feb 15, 2023

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

clubby789 commented Feb 15, 2023

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
@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

clubby789 commented Feb 15, 2023

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

albertlarsan68 commented Feb 16, 2023

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

clubby789 commented Feb 17, 2023

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
Collaborator

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
Collaborator

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
Collaborator

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
filetime = "0.2"
getopts = "0.2.19"
cc = "1.0.69"
clap = { version = "4.1.4", features = ["std", "usage", "help", "derive", "error-context"], default-features = false}
Copy link
Member

@bjorn3 bjorn3 Apr 22, 2023

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

@jyn514 jyn514 Apr 22, 2023

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

clubby789 commented Apr 22, 2023

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

@bjorn3 bjorn3 bjorn3 left review comments

+2 more reviewers

@klensy klensy klensy left review comments

@jyn514 jyn514 jyn514 left review comments

Reviewers whose approvals may not affect merge requirements

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