Add a clang-format check to GH actions #7302

mjschmidt271 · 2025-04-29T19:22:53Z

Adds a github workflow, to run on PRs, that determines adherence to clang-format style requirement.

This new GitHub Action runs as a check on PRs. It is intended to do a formatting check only on the files changed by the PR using the clang-format tool. Note, however, that clang-format will be applied to the entire file, and not only the lines that were changed.

For this workflow, I compared a couple of the most used Actions from the GitHub Marketplace, and thought that the diff provided by DoozyX/clang-format-lint-action added useful information (See this output for an example.), as opposed to only the clang-format error output, as provided by jidicula/clang-format-action (example output).

~~I do like that the latter provides a summary on the page that denotes which files fail the clang-format, but I may just hack that in myself in a subsequent PR.~~ (see EDIT 2, below)

Details

The formatting style is defined by the config file components/eamxx/.clang-format.
- I've chosen the LLVM style for formatting, since that is what we've been using in MAM4xx and seems reasonably non-controversial.
- I have not added any customizations to the .clang-format config, but I have included, as comments, all of the options that the LLVM style imposes.
- I expect there will be opinions on formatting choices, so please reach out with comments.
Different clang versions have slightly different behaviors, so I've chosen clang-format v14, again, because that's what we use in MAM4xx.
This is intended to be low-impact on developers, so please reach out with any questions, comments, angry rants, etc. 🙂

EDIT

I've started a discussion page where others can provide feedback.

EDIT 2

As mentioned above, I modified the DoozyX/clang-format-lint-action Action so that it prints a step summary indicating which, if any, files fail the clang-format test. The fork of that Action is here

DRAFT - PR Must be Open to Trigger Testing

This new GitHub Action is intended to do a formatting check only on the files changed by the PR using the clang-format tool.

To begin with, I am testing 2 different actions provided by the GitHub Marketplace. One of them only returns a binary pass/fail, and the other can change the files in-place (not the desired behavior) but it's not clear to me whether disabling that option returns a diff and whether it writes new files with the changes or merely outputs the diff to the log.

.github/workflows/e3sm-gh-clang-format.yml

components/eamxx/src/atmosphere_driver.hpp

mahf708

let's get @rljacob to weigh in as well pls

.github/workflows/eamxx-gh-clang-format.yml

components/eamxx/.clang-format

rljacob · 2025-05-02T09:05:33Z

This will check for format but not automatically change the file?

bartgol · 2025-05-02T15:04:15Z

This will check for format but not automatically change the file?

That's my understanding. I think the idea would be that once the PR is good to merge, the author (or maybe the integrator) runs the clang formatter, formats the files, pushes a new commit and then merges. I don't want the action to add a commit when it runs, since if the PR is open for a while, it may run several times, and commit multiple times a "clang format" commit.

The alternative could be to run the action also when the main branch is updated, so that we get a clang format commit after PRs are merged (assuming they are not already well formatted).

mahf708 · 2025-05-02T15:08:02Z

This will check for format but not automatically change the file?

That's my understanding. I think the idea would be that once the PR is good to merge, the author (or maybe the integrator) runs the clang formatter, formats the files, pushes a new commit and then merges. I don't want the action to add a commit when it runs, since if the PR is open for a while, it may run several times, and commit multiple times a "clang format" commit.

The alternative could be to run the action also when the main branch is updated, so that we get a clang format commit after PRs are merged (assuming they are not already well formatted).

The formatter cannot format everything, so relying on it to correct the errors is going to be incomplete.

I think I would like to see zero "clang format" commits. I am happy if we agree to ask for (and enforce) rebasing the fixup format commit into the history. Let's just do that.

.github/workflows/eamxx-gh-clang-format.yml

bartgol · 2025-05-02T15:11:26Z

I think I would like to see zero "clang format" commits. I am happy if we agree to ask for (and enforce) rebasing the fixup format commit into the history. Let's just do that.

That sounds fine to me.

Copilot

Pull Request Overview

This PR introduces a GitHub Action workflow to enforce clang-format adherence on modified C++ source files using a configuration based on LLVM style.

Updated the clang-format configuration in components/eamxx/.clang-format to switch from Google to LLVM style and include detailed comments with default options.
Added a new GitHub workflow (.github/workflows/eamxx-gh-clang-format.yml) that checks the formatting of changed cpp/hpp files using DoozyX/clang-format-lint-action.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
components/eamxx/.clang-format	Updated the formatting configuration to use LLVM style and added extensive comments.
.github/workflows/eamxx-gh-clang-format.yml	New GitHub Action workflow that runs clang-format checks on changed files.

components/eamxx/.clang-format

jgfouca · 2025-05-02T15:35:31Z

This doesn't force us into <=80 line length, right? That would be super annoying and makes the code look like crap (IMO).

mahf708 · 2025-05-02T15:43:40Z

This doesn't force us into <=80 line length, right? That would be super annoying and makes the code look like crap (IMO).

I think it does... btw, see: #7307

philipwjones · 2025-05-02T16:16:05Z

We restrict to 80 char since that is a typical default terminal/editor window and for many of us, the optimal visual range. Having said that, it's definitely better to make those line breaks yourself in logical places, so even if the formatter catches it and tries to do it for you, you might want to manually edit those to get a better line break.

philipwjones · 2025-05-02T16:21:42Z

Also, 80-char limit is part of the LLVM coding style

mjschmidt271 · 2025-05-02T16:30:59Z

This will check for format but not automatically change the file?

That's my understanding. I think the idea would be that once the PR is good to merge, the author (or maybe the integrator) runs the clang formatter, formats the files, pushes a new commit and then merges. I don't want the action to add a commit when it runs, since if the PR is open for a while, it may run several times, and commit multiple times a "clang format" commit.
The alternative could be to run the action also when the main branch is updated, so that we get a clang format commit after PRs are merged (assuming they are not already well formatted).

The formatter cannot format everything, so relying on it to correct the errors is going to be incomplete.

I think I would like to see zero "clang format" commits. I am happy if we agree to ask for (and enforce) rebasing the fixup format commit into the history. Let's just do that.

@rljacob Others have chimed in, but this does not modify any source files--only returns a pass/fail status for the test and then includes a diff of the theoretical changes.

And I'll have to take a closer look, but the Action does allow the --inplace flag to clang-format, so I'm pretty sure we could set it up to be triggered by the merge commit. And if we want to be extra-fancy we could follow it up with a squash rebase.

mjschmidt271 · 2025-05-02T16:35:35Z

It also occurs to me that we could migrate to autoformatting the source files in the future.

Once most files have been edited and thus formatted, we can add a commit that formats the remaining files and then automate formatting by default

bartgol · 2025-05-02T22:09:28Z

I am going to push back on the 80 chars limit. This is one of the reasons I never pushed for a formatter: everyone likes different lengths, tabs, indentation rules, etc.

Even though 80 chars are common, I find the width too narrow. Yes, one can break long templates/signatures over multiple lines, but sometimes i prefer going a few chars long than breaking the line. And with multiple indentation levels (namespace, class, loop, lambda, nested loops, etc) one is often starting with a few chars already gone.

Not just that, I fear someone may start using abbrev nms just 2 avoid line ovrflw... I really like when we use long, verbose names, but I would like to avoid having every line of code broken over 2-3 lines...

If we really REALLY want a limit, I would request at least 100 (possibly 120?).

jgfouca · 2025-05-03T17:07:47Z

Yeah, I went through this last summer with KokkosKernels. Complex code with heavy templating just looks like crap with an 80 character limit as the formatter chops and squeezes your code. We (KK) bumped it to 120 and it's so much nicer.

.github/workflows/eamxx-gh-clang-format.yml

mam4xxSNL · 2025-05-06T19:00:56Z

Alright, I do believe I've addressed all of the comments. As for style conventions, here's where we're at:

Finally, I would ask to hold off on more comments on the conventions until this week's EAMxx dev meeting, and I'll make sure this topic is on the agenda!

@mahf708 @bartgol @jgfouca @rljacob

mjschmidt271 · 2025-05-06T19:13:40Z

Well, that's strange... switching GH accounts with a partiallly-written message can apparently send it(?)? Anyway...

Alright, I do believe I've addressed all of the comments. As for style conventions, here's where we're at:

BasedOnStyle: LLVM
ColumnLimit: 100
AlignConsecutiveAssignments: true
AlignConsecutiveBitFields: true
AlignConsecutiveMacros: true
AlignEscapedNewlines: true
AlignTrailingComments: true

This is based on requests to expand the 80 column limit for lines, under the reasoning to try out 100, rather than going for 120, as some have mentioned.
Based on feedback from the Omega team, I'd advocate for trying out the alignment settings.
And there's not been much action on other formatting topics, so we will keep them default for now.
In 2 weeks (5/20), I'll remove the commented lines from the .clang-format config.

Finally, I would ask to hold off on more comments on the conventions until this week's EAMxx dev meeting, and I'll make sure this topic is on the agenda!

@mahf708 @bartgol @jgfouca @rljacob

mahf708 · 2025-05-06T20:07:43Z

.github/workflows/eamxx-gh-clang-format.yml

+    # 1. runs clang-format on the files changed by this PR, and returns pass/fail
+    #    error code.
+    # 2. prints the diff to screen (action log), comparing to the changes
+    #    that *would* be made
+    # 3. the mjschmidt fork of DoozyX/clang-format-lint-action@v0.20
+    #    adds a Step summary to the workflow page, and, on failure, lists the
+    #    files that fail the clang-format test
+    - uses: mjschmidt271/clang-format-lint-action@v1.0.0
+      with:
+        source: ${{ steps.changed-files.outputs.all_changed_files }}
+        exclude: ''
+        extensions: 'hpp,cpp'
+        clangFormatVersion: 14
+        style: 'file:components/eamxx/.clang-format'


For my previous comment, where I likely didn't respond to you earlier... I simply meant, can't we achieve this by simply triggering clang-format directly? There's a clang-format available in the image used here. I think it would be easier for maintainability and transparency.

Currently, this step action runs the following command:

/usr/bin/docker run --name cbc12ed501f5cdec47429a8bd95d0f2dcaff04_b2adbf --label cbc12e --workdir /github/workspace --rm -e "INPUT_SOURCE" -e "INPUT_EXCLUDE" -e "INPUT_EXTENSIONS" -e "INPUT_CLANGFORMATVERSION" -e "INPUT_STYLE" -e "INPUT_INPLACE" -e "HOME" -e "GITHUB_JOB" -e "GITHUB_REF" -e "GITHUB_SHA" -e "GITHUB_REPOSITORY" -e "GITHUB_REPOSITORY_OWNER" -e "GITHUB_REPOSITORY_OWNER_ID" -e "GITHUB_RUN_ID" -e "GITHUB_RUN_NUMBER" -e "GITHUB_RETENTION_DAYS" -e "GITHUB_RUN_ATTEMPT" -e "GITHUB_ACTOR_ID" -e "GITHUB_ACTOR" -e "GITHUB_WORKFLOW" -e "GITHUB_HEAD_REF" -e "GITHUB_BASE_REF" -e "GITHUB_EVENT_NAME" -e "GITHUB_SERVER_URL" -e "GITHUB_API_URL" -e "GITHUB_GRAPHQL_URL" -e "GITHUB_REF_NAME" -e "GITHUB_REF_PROTECTED" -e "GITHUB_REF_TYPE" -e "GITHUB_WORKFLOW_REF" -e "GITHUB_WORKFLOW_SHA" -e "GITHUB_REPOSITORY_ID" -e "GITHUB_TRIGGERING_ACTOR" -e "GITHUB_WORKSPACE" -e "GITHUB_ACTION" -e "GITHUB_EVENT_PATH" -e "GITHUB_ACTION_REPOSITORY" -e "GITHUB_ACTION_REF" -e "GITHUB_PATH" -e "GITHUB_ENV" -e "GITHUB_STEP_SUMMARY" -e "GITHUB_STATE" -e "GITHUB_OUTPUT" -e "RUNNER_OS" -e "RUNNER_ARCH" -e "RUNNER_NAME" -e "RUNNER_ENVIRONMENT" -e "RUNNER_TOOL_CACHE" -e "RUNNER_TEMP" -e "RUNNER_WORKSPACE" -e "ACTIONS_RUNTIME_URL" -e "ACTIONS_RUNTIME_TOKEN" -e "ACTIONS_CACHE_URL" -e "ACTIONS_RESULTS_URL" -e GITHUB_ACTIONS=true -e CI=true -v "/var/run/docker.sock":"/var/run/docker.sock" -v "/home/runner/work/_temp/_github_home":"/github/home" -v "/home/runner/work/_temp/_github_workflow":"/github/workflow" -v "/home/runner/work/_temp/_runner_file_commands":"/github/file_commands" -v "/home/runner/work/E3SM/E3SM":"/github/workspace" cbc12e:d501f5cdec47429a8bd95d0f2dcaff04 "--clang-format-executable" "/clang-format/clang-format14" "-r" "--color" "always" "--style" "file:components/eamxx/.clang-format" "--inplace" "false" "--extensions" "hpp,cpp" "--exclude" "" "components/eamxx/src/atmosphere_driver.cpp"

but ofc, we can just use the last part of it, i.e., the cli version of this list of strings

"/clang-format/clang-format14" "-r" "--color" "always" "--style" "file:components/eamxx/.clang-format" "--inplace" "false" "--extensions" "hpp,cpp" "--exclude" "" "components/eamxx/src/atmosphere_driver.cpp"

Personally, I find using composite actions nicer than run: blah steps.

That said, I think e3sm's mantra is to avoid stuff from personal repos. Any interest in forking the clang-format-lint action in e3sm-project?

@bartgol - I do agree that forking the action to e3sm-project would make the most sense. I'll reach out elsewhere about that.

And, having now chatted with @mahf708 about this, I plan to include the clang-format [...] invocation signature in the log output and also the job summary

mahf708 · 2025-05-06T20:08:06Z

components/eamxx/.clang-format

+AlignEscapedNewlines: true
+AlignTrailingComments: true
+---
+# # these are the defaults for the LLVM style, generated by


if we are going to remove all these, why wait?

mahf708 · 2025-05-06T20:08:17Z

components/eamxx/.clang-format

-PointerAlignment:            Right
-DerivePointerAlignment:      false
+BasedOnStyle: LLVM
+ColumnLimit: 100


~~I think both Jim and Luca want 120~~

oops no more comments on options!

.github/workflows/eamxx-gh-clang-format.yml

mahf708

let's go rawrrrrrrrrrr

bartgol · 2025-05-07T20:00:07Z

@mjschmidt271 It appears the action repo is still in your fork. Also, you added back the driver files (I assume to test the action would work, and planned to remove later?)

mjschmidt271 · 2025-05-07T20:27:19Z

@mjschmidt271 It appears the action repo is still in your fork. Also, you added back the driver files (I assume to test the action would work, and planned to remove later?)

@bartgol Exactly correct--I've now switched to the E3SM-Project fork, removed files, and rebased. Should be ready to go!

--- re-enable clang-format-lint-action add .clang-format config for LLVM style and choose clang-format-lint-action remove test files swap to modified GH action, move cpp file for testing, modify format spec for 100 columns

Adds a github workflow, to run on PRs, that determines adherence to clang-format style requirement.

mjschmidt271 added the CI: workflow change approved Allow gh action PR testing on ghci-snl-* machines for PRs that alter a worfklow file label Apr 29, 2025

mjschmidt271 requested a review from jgfouca April 29, 2025 19:29

jgfouca approved these changes Apr 29, 2025

View reviewed changes

mjschmidt271 added EAMxx Issues related to EAMxx BFB PR leaves answers BFB labels Apr 29, 2025

jgfouca approved these changes Apr 29, 2025

View reviewed changes

bartgol reviewed Apr 29, 2025

View reviewed changes

.github/workflows/e3sm-gh-clang-format.yml Outdated Show resolved Hide resolved

rljacob marked this pull request as draft April 30, 2025 11:54

bartgol reviewed Apr 30, 2025

View reviewed changes

components/eamxx/src/atmosphere_driver.hpp Outdated Show resolved Hide resolved

mjschmidt271 changed the title ~~DRAFT: Add a clang-format check to GH actions~~ Add a clang-format check to GH actions May 1, 2025

mjschmidt271 marked this pull request as ready for review May 1, 2025 21:35

mjschmidt271 requested review from AaronDonahue and mahf708 May 1, 2025 21:35

mjschmidt271 force-pushed the mjs/eamxx/clang-format-action branch 2 times, most recently from e3b7b2b to b3e4183 Compare May 1, 2025 21:44

bartgol approved these changes May 1, 2025

View reviewed changes

mahf708 requested changes May 2, 2025

View reviewed changes

.github/workflows/eamxx-gh-clang-format.yml Outdated Show resolved Hide resolved

components/eamxx/.clang-format Show resolved Hide resolved

mahf708 requested a review from rljacob May 2, 2025 00:04

mahf708 reviewed May 2, 2025

View reviewed changes

.github/workflows/eamxx-gh-clang-format.yml Show resolved Hide resolved

mahf708 requested a review from Copilot May 2, 2025 15:24

Copilot AI reviewed May 2, 2025

View reviewed changes

components/eamxx/.clang-format Outdated Show resolved Hide resolved

mahf708 reviewed May 5, 2025

View reviewed changes

.github/workflows/eamxx-gh-clang-format.yml Outdated Show resolved Hide resolved

rljacob assigned bartgol May 5, 2025

bartgol added CI: workflow change approved Allow gh action PR testing on ghci-snl-* machines for PRs that alter a worfklow file and removed CI: workflow change approved Allow gh action PR testing on ghci-snl-* machines for PRs that alter a worfklow file labels May 6, 2025

bartgol approved these changes May 6, 2025

View reviewed changes

mahf708 reviewed May 6, 2025

View reviewed changes

mahf708 self-requested a review May 7, 2025 17:24

mahf708 approved these changes May 7, 2025

View reviewed changes

bartgol approved these changes May 7, 2025

View reviewed changes

mjschmidt271 force-pushed the mjs/eamxx/clang-format-action branch 2 times, most recently from 037a5d9 to d4d7372 Compare May 7, 2025 20:23

bartgol approved these changes May 7, 2025

View reviewed changes

mjschmidt271 force-pushed the mjs/eamxx/clang-format-action branch from d4d7372 to b70e580 Compare May 7, 2025 23:57

add clang-format check to GH actions

d633bd7

--- re-enable clang-format-lint-action add .clang-format config for LLVM style and choose clang-format-lint-action remove test files swap to modified GH action, move cpp file for testing, modify format spec for 100 columns

mjschmidt271 force-pushed the mjs/eamxx/clang-format-action branch from b70e580 to d633bd7 Compare May 8, 2025 16:10

bartgol added a commit that referenced this pull request May 8, 2025

Merge branch 'mjs/eamxx/clang-format-action' into next (PR #7302)

af3e358

Adds a github workflow, to run on PRs, that determines adherence to clang-format style requirement.

bartgol merged commit 22e635f into master May 8, 2025
19 checks passed

bartgol deleted the mjs/eamxx/clang-format-action branch May 8, 2025 19:38

mjschmidt271 mentioned this pull request Sep 4, 2025

[RFC] EAMxx: Feedback regarding the EAMxx autoformatting workflow #7659

Open

Add a clang-format check to GH actions #7302

Add a clang-format check to GH actions #7302

Uh oh!

Conversation

mjschmidt271 commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

EDIT

EDIT 2

DRAFT - PR Must be Open to Trigger Testing

Uh oh!

Uh oh!

Uh oh!

mahf708 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rljacob commented May 2, 2025

Uh oh!

bartgol commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mahf708 commented May 2, 2025

Uh oh!

Uh oh!

bartgol commented May 2, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

jgfouca commented May 2, 2025

Uh oh!

mahf708 commented May 2, 2025

Uh oh!

philipwjones commented May 2, 2025

Uh oh!

philipwjones commented May 2, 2025

Uh oh!

mjschmidt271 commented May 2, 2025

Uh oh!

mjschmidt271 commented May 2, 2025

Uh oh!

bartgol commented May 2, 2025

Uh oh!

jgfouca commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mam4xxSNL commented May 6, 2025

Uh oh!

mjschmidt271 commented May 6, 2025

Uh oh!

mahf708 May 6, 2025

Choose a reason for hiding this comment

Uh oh!

bartgol May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjschmidt271 May 7, 2025

Choose a reason for hiding this comment

Uh oh!

mahf708 May 6, 2025

Choose a reason for hiding this comment

Uh oh!

mahf708 May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mahf708 May 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mahf708 left a comment

Choose a reason for hiding this comment

Uh oh!

mjschmidt271 commented Apr 29, 2025 •

edited

Loading

bartgol commented May 2, 2025 •

edited

Loading

jgfouca commented May 3, 2025 •

edited

Loading

bartgol May 6, 2025 •

edited

Loading

mahf708 May 6, 2025 •

edited

Loading