Enforce clang-format style for files in this repository #692
Conversation
This commit adds necessary CMake logic to use `clang-format` to format source code contained in this repository. This notably excludes files such as musl and other upstream source code which isn't primarily authored in this repository, so it's intended to only apply to code that is primarily developed here.
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Nice! clang-format for the win!
| with: | ||
| submodules: true | ||
| - run: cmake -S . -B build -DBUILD_TESTS=ON -G Ninja -DCMAKE_C_COMPILER=clang | ||
| - run: ninja -C build format-check |
There was a problem hiding this comment.
Sometimes I find its good to only enforce formatting for that specificly changed lines.
This can be done using git clang-format origin/main.
But I guess if the whole codebase is already clang-format clean then just checking the whole codebase could be ok too.
One other downside here is that if/when clang-format itself changes its rules, even slightly, then we will see failures here that are not related to the lines of code changed in the given PR, which can be annoying/confusing.
There was a problem hiding this comment.
I'd prefer to format the whole codebase yeah to ensure consistent style across files. It also helps avoid PRs being larger-than-necessary where if you touch a file for the first time it happens to then require a whole bunch of formatting changes which can be a confusing experience for contributors.
I do expect versioning to be a problem and I'm not sure how best to work around that. Turns out github actions has clang-format 18, but locally I have 19, so I had to download a binary for 18 to get it to match CI. With a CI check here though it'll be required for all contributors to use the same version, though. The failure mode will be "frustrating to make a PR that passes CI" instead of something like "different formatting in the codebase due to different versions".
I've got little experience myself running clang-format. If you (or others!) have more experience I'd be happy to tweak things here to something that has a different set of "best practices" or something like that. For example it might be sufficient to dump clang-format-18's .clang-format here and then the formatting will be "mostly the same" across versions, but I'm not entirely sure.
There was a problem hiding this comment.
I'd prefer to format the whole codebase yeah to ensure consistent style across files. It also helps avoid PRs being larger-than-necessary where if you touch a file for the first time it happens to then require a whole bunch of formatting changes which can be a confusing experience for contributors.
Thats not how git clang-format origin/main works, it literally just formats the lines you touch.
But yes, I agree its better keep the whole codebase formatting.
How about clang-formatting the entire codebase now (and the periodically as needed) but only enforcing formatting for the lines changes in the given PR?
What I would specifically what to avoid is PR authors being forced to changes files that they did not touch. I guess in that case we could always want to do full-reformat before the PR in question lands.
There was a problem hiding this comment.
One possible problem with whole-codebase-clang-format is that it makes the check highly sensitive to every single clang-format change.
If you are only running clang-format on changed lines then you are much less likely to hit strange edge cases.
Also, when should not be recommending that PR authors ever clang-format the whole codebase themseleves. PR authors should only ever run git clang-format origin/main,right? Whole codebase formatting should be reserved for specific PRs like this one, right?
There was a problem hiding this comment.
I may be misunderstanding, but if the codebase is continually verified in CI to always be formatted, git clang-format should always work? Other than this PR I expect whole-codebase formatting to happen when clang-format is updated in CI, but otherwise all contributions will always be verified in CI to have the entire codebased to be formatted, which should be the same as "just make sure what you edited stays formatted"
There was a problem hiding this comment.
Yes, as long clang-format itself is pinned in some way then whole-codebase-formatting should be the same as patch-only-formatting.
The risk is that clang-format might change over time, but if its pinned its not so risky, just a little wasteful of CPU.
If clang-format ever does change its mind about something then we do risk PR contains false positives.
patch-only-formatting is nice because you never get false positives, but its not a big deal as long we are aware I guess.
There was a problem hiding this comment.
patch-only-formatting is good for codebases that want to upgrade clang-format from time to time without doing a whole-codebase reformat each time. But I think this codebase is small enough that we maybe don't care about the odd reformat.
There was a problem hiding this comment.
Ok makes sense. And yeah this is small enough with few enough contributors that the occasional format-the-world PR I don't think will be very disruptive. If it is then it's always possible to revisit this.
| - uses: actions/[email protected] | ||
| with: | ||
| submodules: true | ||
| - run: cmake -S . -B build -DBUILD_TESTS=ON -G Ninja -DCMAKE_C_COMPILER=clang |
There was a problem hiding this comment.
It seems rather odd (and a waste of resources) to have to build stuff just to run clang-format. Can't we just run clang-format directly here with a list of files?
There was a problem hiding this comment.
Maybe you could write a small python script that just runs clang-format and has a bunch of regexs.
Having to run cmake at all seem somewhat backwards/annoying.
There was a problem hiding this comment.
I'd disagree myself in that if we've got cmake I don't see why it wouldn't be used. This doesn't actually build anything, it just runs the formatter. The configure step is necessary to generate the build system which runs the formatting. The -DBUILD_TESTS=ON part here is to ensure that tests are formatted as well as libc source.
There was a problem hiding this comment.
Fair enough. I think a simple script would be more straight forward but you are the one putting in all the work here, which is great. LGTM!
2 participants
This commit adds necessary CMake logic to use
clang-formatto formatsource code contained in this repository. This notably excludes files
such as musl and other upstream source code which isn't primarily
authored in this repository, so it's intended to only apply to code that
is primarily developed here.
Procedurally this PR is split into two commits: the first is the CMake-logic necessary to add a
formatandformat-checktarget. The second is the actual formatting applied to this repository. I'll note that personally I find it primarily important to have a style enforced in CI, but I don't have a preference as to which style is enforced. There are quite a few options with.clang-format, which I'm happy to tweak/apply as others see fit.