Skip to content

Conversation

lucienfer
Copy link
Contributor

What does this PR do?

This PR extends the existing format job in .github/workflows/build.yml to
check Rust formatting with nightly rustfmt:

- name: Check Rust formatting (rustfmt --check)
  run: cargo fmt --all -- --check

Why?

• The repository already defines a rustfmt.toml with nightly-only settings (imports_granularity, use_small_heuristics = "Max", etc.).
• The CI installs nightly rustfmt but wasn’t actually using it.
• Currently, dprint/check verifies formatting for Markdown/PO/JS/etc., but not Rust code.
This PR makes sure PRs fail if Rust code is not formatted according to rustfmt.toml.

How was it tested?

• Ran locally with:

rustup default nightly
rustup component add rustfmt
cargo fmt --all -- --check
dprint fmt --check

Both checks pass on a clean branch.

• CI workflow updated accordingly.
If Rust code is unformatted, the format job will now fail.

Closes #2794

Copy link

google-cla bot commented Sep 7, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@mgeisler
Copy link
Collaborator

mgeisler commented Sep 7, 2025

Hey @lucienfer, thanks for the PR! I'm surprised this is necessary: dprint is supposed to run rustfmt as well, but I guess it's somehow not doing this (since you include formatting changes in this PR)?

@lucienfer
Copy link
Contributor Author

You’re right, after digging a bit I realized that dprint wasn’t actually checking the Rust formatting properly because it was configured to run rustfmt on the stable toolchain. Since the repo’s rustfmt.toml uses unstable options like imports_granularity = "module", those settings were simply being ignored.

To fix this I made two changes:
1. I added the unstable_features = true flag in rustfmt.toml to make sure nightly-specific settings are explicitly enabled.
2. Instead of relying on cargo fmt in CI, I updated the dprint.json config so that it directly runs rustup run nightly rustfmt. This way dprint is aligned with the nightly toolchain already used in CI, and it will fail if the code isn’t correctly formatted.

This should ensure that formatting is consistently checked both locally (via dprint) and in CI.

@mgeisler
Copy link
Collaborator

mgeisler commented Sep 9, 2025

Awesome that you investigated, super helpful! I remember we had #2168 a while ago, which made us go from nightly to stable for this. I think we should revisit my comment from then and simply pin the nightly version for this. Something like nightly-2025-09-01 or simila.

When I did this recently in my new company, I ran into dprint being slow or basically erroring out: I had not primed the cache on CI, so I think dprint basically tried to auto-install the nightly version on the fly for every file at the same time.

I filled dprint/dprint#1023 about this in dprint just now, perhaps there is an elegant solution. In the meantime, I suggest we pin the version and add a rustup install --profile minimal nightly-2025-09-01 step before calling dprint in CI.

For people at home calling dprint fmt, we should also be helpful: we could install the right nightly toolchain with cargo xtask install-tools or similar? Cc @egithinji and @michael-kerscher who have both looked at our build infrastructure.

@lucienfer
Copy link
Contributor Author

I updated the PR to pin rustfmt to nightly-2025-09-01, pre-install it in CI, make dprint call that exact toolchain, and wire the same install into cargo xtask install-tools. This should keep dprint fast on CI and ensure identical formatting locally and in CI.

Comment on lines 27 to 28
rustup toolchain install --profile minimal nightly-2025-09-01
rustup component add rustfmt --toolchain nightly-2025-09-01
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! One suggestion: could you use a variable for this so it's easier to update in a few months?

Cc @egithinji, could we use cargo xtask install-tools here? Not now, but in a later PR since this just refactors the existing install commands.

@mgeisler mgeisler enabled auto-merge (squash) September 9, 2025 21:19
Copy link
Collaborator

@mgeisler mgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good now, we just need to fix the formatting!

auto-merge was automatically disabled September 9, 2025 21:32

Head branch was pushed to by a user without write access

@mgeisler mgeisler changed the title ci: add rustfmt check to formatting workflow ci: use pinned nightly rustfmt to make unstable features take effect Sep 10, 2025
@mgeisler mgeisler merged commit 4731acf into google:main Sep 10, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply rustfmt to exercises
2 participants