run tidy's style check from the root path #110316
Conversation
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
A-testsuite
|
rustbot can read my mind! |
| check!(style, &src_path); | ||
| check!(style, &tests_path); | ||
| check!(style, &compiler_path); | ||
| check!(style, &library_path); |
There was a problem hiding this comment.
Note that by applying the check separately we're getting parallelism. I suspect that this will make tidy slower since style is one of the longer-running tasks.
Can you benchmark this?
There was a problem hiding this comment.
It doesn't seem to be any worse (no numbers). On the roadmap for tidy is file-level parallelism in walk which would alleviate any possible perf regressions.
There was a problem hiding this comment.
Can you measure the performance? Something like hyperfine may help there.
There was a problem hiding this comment.
Sorry, I'm on holiday right now and only have access to my laptop (which can barely run x test tidy in the first place lol). I will note that it's not noticeably different but I have no benchmarks to confirm that at the micro-scale.
There was a problem hiding this comment.
Without this patch: 2.2 seconds, with this patch: 2.6 seconds (results consistent across two systems, one with macOS the other with Linux). I personally don't really pay super close attention to tidy test times, but I believe folks are running it in e.g. commit hooks so this might be significant. @the8472 do you have an opinion on this?
There was a problem hiding this comment.
So the intent here is to just get README.md under tidy control? If so then spending 0.4 seconds more on just that seems a bit much. And yes I run tidy while rebasing so the extra wait can be a bit annoying.
Would it work to add README.md as additional root point for the traversal?
On the roadmap for tidy is file-level parallelism in walk which would alleviate any possible perf regressions.
If it's on the radar I won't object strongly to have this merged but if we can achieve the goal without the loss it would still be preferable.
There was a problem hiding this comment.
So the intent here is to just get README.md under tidy control?
Well, more than just README.md, all the other files from the root directory which aren't in the well-defined directories (for instance the RELEASES.md formatting issue was caught here; as well as something which I put in another PR I think).
There was a problem hiding this comment.
Ah sorry, yes, I meant RELEASES.md
There was a problem hiding this comment.
Anyway, if we can somehow avoid putting everything into a single task instead add a way to only capture the additional files at the root level (e.g. globbing /* but not /**/*?) then that'd help with the performance issue.
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
@Ezrashaw FYI: when a PR is ready for review, send a message containing |
|
Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks |
Dylan-DPC
added
S-inactive
See https://github.com/rust-lang/rust/pull/110307/files#r1166327524.
🤞 the change in
RELEASES.mdis ok? maybe cc @Mark-Simulacrum