-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Run rustfmt on batches of multiple files #7966
Conversation
r? @camsteffen (rust-highfive has picked a reviewer for you, use r? to override) |
c812bf6
to
507030f
Compare
This was sorely needed, thanks! @bors r+ |
📌 Commit 507030f has been approved by |
Wait it took so long for you? It was only at 10s on every machine for me, so I didn't bother looking into it. 😄 Thanks so much for this! |
I was surprised that it was that long. Has it gotten slower?? |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Now that you mention it the before numbers do seem a bit odd, I measured it after a rebase but it never felt like 2 minutes, more like 20-30s |
Surprisingly it got slower in 24d561f. Looks like rustup takes ~100ms extra if you don't manually specify the toolchain, which adds up over lots of tests
|
Is this maybe a perf regression of rustfmt since 2021-11-04? Those numbers seem really high and would explain why it took up to 2 minutes. |
I believe it's not on rustfmt's end, it happens with rustc as well, here on stable (with no override/rust-toolchain file around)
I plan to take a better look tomorrow |
I think it's down to rust-lang/rustup#2626 |
changelog: none
This gives
cargo dev fmt
a nice speed boost, down from 90s (because old) on my laptop and 120s (because windows) on my desktop to ~5s on both250 at a time was to give windows a good amount of headroom (failed at ~800, rust-lang/rust#40384)
Also adds rustfmt to the toolchain file and has the clippy_dev workflow test using the pinned version as a follow up to #7963