-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
Introducing clap on tidy #151262
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
Introducing clap on tidy #151262
Conversation
This comment has been minimized.
This comment has been minimized.
|
how does this affect the compile times of tidy? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment has been minimized.
This comment has been minimized.
091ac48 to
d0e3e42
Compare
0b2f709 to
95a20e6
Compare
|
|
|
r? bootstrap |
|
r? Zalathar |
src/tools/tidy/src/parser.rs
Outdated
| mod tests; | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct TidyParser { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename the module to arg_parser, and rename this struct to TidyArgParser.
The reason is that tidy parses a few different things, so it's good to be clear that this module and type are for parsing command-line arguments.
src/tools/tidy/src/parser.rs
Outdated
| pub verbose: bool, | ||
| pub bless: bool, | ||
| pub extra_checks: Option<Vec<String>>, | ||
| pub pos: Vec<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please name this pos_args, which I think is a bit clearer.
|
Hopefully that's all; sorry for the nitpicks. 😅 @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
|
@rustbot ready |
|
Ah, could you please squash the renames into the main two commits? It's a bit of extra work, but it really helps when looking back through history. |
Current tidy parses paths and options manually, and parsing is spreading multple files. This commit introduces a parser using clap to clean and centralize it.
Currently some required arguments (like path of the root dir) are ordered, but it causes an user (mainly bootstrap) needs to remember the order. This commit introduces long arguments (e.g., --root-path) for required args.
|
Sure thing, I squashed the commit 😄 |
|
Two things that I want to note, but that we can leave for a follow-up PR:
|
|
@bors r+ |
Rollup merge of #151262 - Shunpoco:tidy-clap, r=Zalathar Introducing clap on tidy ### Context Currently tidy parses paths/flags from args_os manually, and the extraction is spreading multiple files. It may be a breeding ground for bugs. ref: https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/How.20--ci.3Dtrue.20interact.20with.20CiEnv.3F/near/543171560 (N.B. We've talked about introducing a ci flag in tidy in that thread, but I don't do it in this PR as I don't want to put multiple changes into a PR. I will introduce the flag in a coming PR.) ### Changes This PR replaces current parsing logic with clap. To confirm the new parser works fine, I introduce an unit test for it. ### Build time We've concerned about how clap increases the build time. In order to confirm the increment is acceptable, I did an experiment on CI: - Run cargo build without cache for tidy 50 times in each environment on CI - Calculate an average and a standard deviation from the result, and plot them Here is the graph: <img width="943" height="530" alt="rust_tidy_build_time" src="https://github.com/user-attachments/assets/c7deee69-9f38-4044-87dc-76d6e7384f76" /> - Clap tends to increase build time ~2s. We think this is not a big problem - Build time differs in each environment - In some cases standard deviation are high, I suppose that busyness of CI instances affect build time
Context
Currently tidy parses paths/flags from args_os manually, and the extraction is spreading multiple files. It may be a breeding ground for bugs.
ref: https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/How.20--ci.3Dtrue.20interact.20with.20CiEnv.3F/near/543171560
(N.B. We've talked about introducing a ci flag in tidy in that thread, but I don't do it in this PR as I don't want to put multiple changes into a PR. I will introduce the flag in a coming PR.)
Changes
This PR replaces current parsing logic with clap. To confirm the new parser works fine, I introduce an unit test for it.
Build time
We've concerned about how clap increases the build time. In order to confirm the increment is acceptable, I did an experiment on CI:
Here is the graph:
