Skip to content

Introducing clap on tidy#151262

Merged
rust-bors[bot] merged 2 commits intorust-lang:mainfrom
Shunpoco:tidy-clap
Jan 31, 2026
Merged

Introducing clap on tidy#151262
rust-bors[bot] merged 2 commits intorust-lang:mainfrom
Shunpoco:tidy-clap

Conversation

@Shunpoco
Copy link
Contributor

@Shunpoco Shunpoco commented Jan 17, 2026

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:
rust_tidy_build_time

  • 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

@rustbot rustbot added A-tidy Area: The tidy tool S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jan 17, 2026
@Shunpoco Shunpoco changed the title Using clap on tidy [WIP] Using clap on tidy Jan 17, 2026
@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

how does this affect the compile times of tidy?

@Shunpoco

This comment was marked as outdated.

@Shunpoco

This comment was marked as duplicate.

@rust-log-analyzer

This comment has been minimized.

@Shunpoco Shunpoco force-pushed the tidy-clap branch 7 times, most recently from 091ac48 to d0e3e42 Compare January 24, 2026 10:15
@Shunpoco Shunpoco changed the title [WIP] Using clap on tidy Introducing clap on tidy Jan 24, 2026
@Shunpoco Shunpoco force-pushed the tidy-clap branch 3 times, most recently from 0b2f709 to 95a20e6 Compare January 24, 2026 10:52
@Shunpoco Shunpoco marked this pull request as ready for review January 24, 2026 12:20
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 24, 2026

tidy extra checks were modified.

cc @lolbinarycat

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 24, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 24, 2026

r? @Zalathar

rustbot has assigned @Zalathar.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@Zalathar
Copy link
Member

r? bootstrap

@Zalathar
Copy link
Member

r? Zalathar

@rustbot rustbot assigned Zalathar and unassigned clubby789 Jan 25, 2026
mod tests;

#[derive(Debug, Clone)]
pub struct TidyParser {
Copy link
Member

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.

pub verbose: bool,
pub bless: bool,
pub extra_checks: Option<Vec<String>>,
pub pos: Vec<String>,
Copy link
Member

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.

@Zalathar
Copy link
Member

Hopefully that's all; sorry for the nitpicks. 😅

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 28, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 28, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@Shunpoco
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot 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 Jan 28, 2026
@Zalathar
Copy link
Member

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.
@Shunpoco
Copy link
Contributor Author

Sure thing, I squashed the commit 😄

@Zalathar
Copy link
Member

Two things that I want to note, but that we can leave for a follow-up PR:

  • The --npm flag really should be called --yarn, but that is related to some internal identifiers in tidy that should be renamed too.
  • extra_checks doesn't need to be Option<Vec<String>>; it can just be Vec<String>, but again that would lead to more follow-on changes in tidy.

@Zalathar
Copy link
Member

@bors r+

@rust-bors
Copy link
Contributor

rust-bors bot commented Jan 31, 2026

📌 Commit 1485873 has been approved by Zalathar

It is now in the queue for this repository.

@rust-bors rust-bors bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2026
rust-bors bot pushed a commit that referenced this pull request Jan 31, 2026
Rollup of 4 pull requests

Successful merges:

 - #151262 (Introducing clap on tidy)
 - #151896 (Revert enabling `outline-atomics` on various platforms)
 - #151849 (refactor: remove `Ty::pinned_ref` in favor of `Ty::maybe_pinned_ref`)
 - #151892 (Document enum types used as values for E0423)
@rust-bors rust-bors bot merged commit df7d12a into rust-lang:main Jan 31, 2026
11 checks passed
@rustbot rustbot added this to the 1.95.0 milestone Jan 31, 2026
rust-timer added a commit that referenced this pull request Jan 31, 2026
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
@Shunpoco Shunpoco deleted the tidy-clap branch January 31, 2026 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-testsuite Area: The testsuite used to check the correctness of rustc A-tidy Area: The tidy tool S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants