utils: sync grass_clang_format script to pre-commit config

[nilason]

To keep ./utils/grass_clang_format.sh shell script in sync with pre-commit configuration, the ClangFormat version is taken from .pre-commit-config.yaml.

[echoix]

A conceptual question here: Where is this internal script used, and by who? It isn't used anymore in our day to day development as I can see from the last year. So, if it isn't maintained nor used, we should simply remove it. It seems to be a utility script, purpose based at the time.
Less to maintain is better, the project is already huge for our active maintaining team size!

[nilason]

As long as using pre-commit is not a requirement this script may have its use.

[neteler]

It seems to be a utility script, purpose based at the time.

I would not remove it as in the past (talking about 20+ years) we occasionally formatted incoming code contributions easily. Likely incoming new code is not formatted according to our standards. It is true that it isn't frequently used but still it more of a hassle to reinvent it/dig in old versions once the script is gone.

Bonus: we can also format the "main" addons and even GRASS GIS related code in other repos easily.

[echoix]

We enforce the style in CI anyways, so all code is required to follow the format defined by the clang-format tool at XX version with the config file. It doesn't matter from where is done.

We already offer suggestions in CI (not just failing without saying what should be), we offer to download the diff as a patch to apply, we offer to download all the changed files to unzip in the repo and commit, we maintain pre-commit config.
And even then, you can configure your IDE as required, or even run the tool manually.

So, who still uses that script daily (or even monthly)?

[neteler]

Random example: we have many addon repos here: https://github.com/mundialis/ - all waiting to be reformatted. This is best done before sending it to CI, through the script.

[echoix]

A problem with a script like this one, when we enforce a specific version for the complete repo, is that there is no simple way to have the correct clang-format for everyone, everywhere, in every distro + distro release + arch, without changing distro release. For example, Ubuntu noble (24.04) only had clang-format 16 or clang-format 18, and not the clang-format 17 we were using in the last couple of months. Ubuntu jammy (22.04) only has clang format 14, that didn't even match the clang-format 15 that we required for a long time before I was more active here.

Precommit solves this by using a pip-bundled statically linked version for use of a single version.

[nilason]

This script exists for corner cases. This updates it to use the version stated in pre-commits configs (= minimal to zero maintenance), it is up to the user to install the version needed (any version is easily installed via pip).

[echoix]

I see how for corner cases, external uses and posterity it might be useful to keep. There wasn't a problem with the PR itself, but an opportunity to discuss its maintenance and relevance.

So only question left is: Should the fallback version "18" absolutely kept in sync in the future, or it can end up outdated a bit? If it is needed to keep updated, we can wire it up with renovate (a regex, we could use an adaptation of the default one used for docker/regex manager, but only indicate to check in this file too + change the regex to not look for ARG or ENV, but req_cf_v=)

[echoix]

Right after knowing that I'll place my approval :)

[nilason]

As long as the .pre-commit-config.yaml doesn't change the structure, the grep | sed retrieving the version should work. In this case I don't think it is necessary to put any effort to make too much of a fuss for the fallback version to be correct (then we wouldn't need to read .pre-commit-config.yaml anyway).
Although there is different output between versions of ClangFormat, it is not huge; I'd say better to (maybe) have a slight deviation (in case fallback version falls out of sync) than to exit the script with an error.

Before we merge this, the script need to tested on a non-BSD system (i.e. GNU, e.g. linux).

[echoix]

You use BSD daily?

[echoix]

Once tested on non-BSD system, it is ok to merge

[nilason]

You use BSD daily?

The mac flavour of it, yes.

[nilason]

Once tested on non-BSD system, it is ok to merge

Thanks for testing!

[echoix]

Once tested on non-BSD system, it is ok to merge

Thanks for testing!

I didn't test it myself. Once it was tested, it would have been ok to merge.

[nilason]

Once tested on non-BSD system, it is ok to merge

Thanks for testing!

I didn't test it myself. Once it was tested, it would have been ok to merge.

Oh dear, I misunderstood.

[wenzeslaus]

BTW, I don't use it and don't plan to. pre-commit works well. Nevertheless, I had to do --no-verify the other day because I was not able to get pre-commit working. So, I can imagine, I may use it in that case, but I can also spend more time getting pre-commit working.

This is best done before sending it to CI, through the script.

pre-commit is really the better solution for that. It goes over many things, not just formatting.