Move clang-format checks to pre-commit

[stephenswat]

This will make them significantly easier for users to use, as well as easier to migrate to future versions.

[paulgessinger]

Looks good. Are you considering using this for other linters?

[stephenswat]

Yes, in time!

[StewMH]

Is it worth keeping the CI check in case it fails locally in some way and something badly formatted creeps in?

[stephenswat]

Is it worth keeping the CI check in case it fails locally in some way and something badly formatted creeps in?

You mean to keep the local check_format.sh file? I would suggest we toss that out as it is no longer necessary. pre-commit can be used locally very simply, e.g.:

$ pip install pre-commit
$ pre-commit run --all-files

Let me know what you think.

[StewMH]

Right I mean if the pre-commit hook fails in some way. But maybe this is obvious/unlikely

[stephenswat]

Right I mean if the pre-commit hook fails in some way. But maybe this is obvious/unlikely

Ah, I think I understand what you mean. Consider, though, that pre-commit will automatically update the files for you so they conform to the clang-format requirements; so in principle if you run pre-commit locally (which doesn't have to be in a pre-commit hook, by the way!) the code should always pass the CI job.

[krasznaa]

I'm very unfamiliar with this... 🤔 How does this work? Does one need to install some extra piece of software? Is it really that a git commit will now forcefully update the code, not allowing formatting at all that clang-format doesn't agree with? 😕

I personally really don't view the current setup we have as so much of a problem. But if this is easy enough to use, we can give it a try, sure...

[krasznaa]

Is it worth keeping the CI check in case it fails locally in some way and something badly formatted creeps in?

You mean to keep the local check_format.sh file? I would suggest we toss that out as it is no longer necessary. pre-commit can be used locally very simply, e.g.:

$ pip install pre-commit
$ pre-commit run --all-files

Let me know what you think.

So you do need to install something? Why is it way better to install this, instead of Docker? 😕

[stephenswat]

I'm very unfamiliar with this... 🤔 How does this work? Does one need to install some extra piece of software? Is it really that a git commit will now forcefully update the code, not allowing formatting at all that clang-format doesn't agree with? 😕

I personally really don't view the current setup we have as so much of a problem. But if this is easy enough to use, we can give it a try, sure...

Even though the tool is called pre-commit, it's not actually required to be in a pre-commit hook at all; it also works as a stand-alone tool and - as shown in the PR - as a CI job. So it's still perfectly possible to commit and push code that is improperly formatted. It will just be rejected by the CI, as it is right now.

The problem with the current setup is that it is a bit of a pain in the rear for users to find the right versions of clang-format; you can download some pre-build binaries from a slightly sketchy git repository or you can use your package manager, but the latter often only supports the latest few versions of clang-format.

So you do need to install something? Why is it way better to install this, instead of Docker? 😕

Well, pre-format makes it much easier for users to use the exact same setup as the CI to reduce version conflicts. Also, spinning up a Docker container every time you want to format the code seems like a lot of unnecessary work. Still, this change doesn't prohibit that at all; users who prefer to use Docker are completely free to do that if they so please.

[stephenswat]

This PR came to be because we talked about updating the clang-format versions on Mattermost. Without pre-commit, this would require users to find a way to install the new version of clang-format; with pre-commit, zero user action would be required.

[krasznaa]

My main point is that you have to write instructions as part of this PR on how to use this pre-commit setup. I myself have no idea for instance.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[stephenswat]

Fair point, I added some documentation to the README. 👍

[StewMH]

It might be good to mention that pre-commit can made to run as a pre-commit hook but does not by default (despite the name). And that you can enable that for your local git checkout with

$ pre-commit install

[StewMH]

Getting the diff from a failed format run is nice - I tested this out here:
https://github.com/StewMH/traccc/actions/runs/12054648976/job/33613171270

clang-format.............................................................Failed
- hook id: clang-format
- files were modified by this hook
pre-commit hook(s) made changes.
If you are seeing this message in CI, reproduce locally with: `pre-commit run --all-files`.
To run `pre-commit` as part of git workflow, use `pre-commit install`.
All changes made by hooks:
diff --git a/tests/alpaka/alpaka_basic.cpp b/tests/alpaka/alpaka_basic.cpp
index a2dbbc6..33dbd6f 100644
--- a/tests/alpaka/alpaka_basic.cpp
+++ b/tests/alpaka/alpaka_basic.cpp
@@ -33,145 +33,145 @@
 
 template <typename Acc>
 ALPAKA_FN_ACC float process(Acc const& acc, uint32_t idx) {
-  return static_cast<float>(alpaka::math::sin(acc, idx));
+    return static_cast<float>(alpaka::math::sin(acc, idx));
 }
...

so from that point of view it is an improvement on our current system: you don't actually have to set up clang-format locally if you're willing to just apply the diff.