search

zmkfirmware / zmk

ZMK Firmware Repository
https://zmk.dev/
MIT License
2.67k stars 2.74k forks source link

pre-commit: use versioned clang-format hook #2315

Closed maximevince closed 2 weeks ago

maximevince commented 4 months ago

slight change to the pre-commit config. the version of clang-format used determined the way the code is formatted.

I was having a lot of trouble because my local version of clang-format (Arch Linux) is more recent than the one running on Github Actions. Therefore, the pre-commit action would almost always fail because of difference in clang-format output.

Using this version of the pre-commit config, it specifies the exact version of clang-format to be used, which I believe matches what is currently using on Github.

caksoylar commented 4 months ago

There was some investigation on Discord by @joelspadin on the versioning: https://discord.com/channels/719497620560543766/719544230497878116/1203479919821328395. In case you don't have access, his conclusion was that the CI seemed to be running 14.0.0.

maximevince commented 4 months ago

I would ask the question differently: what would you want the clang-version to be? @petejohanson or whoever is maintaining the CI / code formatting.

joelspadin commented 4 months ago

I think specifying a version is a good change, though I don't have strong feelings on what that version should be. If we are going to upgade from 14, it should ideally be to something that is easy to install on a majority of OSes.

Ubuntu 22.04's llvm package is version 14.0.0. Version 15.0.7 is also available.

Ubuntu 24.04 isn't available for WSL yet, so I don't know what versions it provides.

I'm not sure which other distros/versions of Linux are commonly used.

On Windows, winget has every release up through 18.1.6.

I don't have a Mac, so no idea what versions are easily installable there.

maximevince commented 4 months ago

A lot of the popular Linux distro's are based on Ubuntu, so that's definitely a big one.

Here's an overview of what I could find:

So I would suggest moving to clang-format v18, if everyone agrees?

petejohanson commented 3 months ago

A lot of the popular Linux distro's are based on Ubuntu, so that's definitely a big one.

Here's an overview of what I could find:

* Ubuntu 24.04LTS (noble) has clang-format 18, it seems: https://packages.ubuntu.com/noble/clang-format

* Fedora 40 seems to have v18 as well: https://packages.fedoraproject.org/pkgs/clang/clang/

* Arch Linux, still seems to be stuck on version 17 for some reason: https://archlinux.org/packages/extra/x86_64/clang/

* On MacOS `brew` gives you v18 as well:  https://formulae.brew.sh/formula/clang-format

So I would suggest moving to clang-format v18, if everyone agrees?

I'd be happy to move to v18 and standardize there.

petejohanson commented 3 weeks ago

@maximevince Do you have any interest in finishing this up with a move to v18? If not, are you okay if someone takes this over?

Thanks again for the PR!

maximevince commented 3 weeks ago

@petejohanson absolutely! I chose v18.1.8 now, which is the latest release for clang-format. Are you okay with using that? It might require some reformatting of existing files.

PR updated.

petejohanson commented 3 weeks ago

@petejohanson absolutely! I chose v18.1.8 now, which is the latest release for clang-format. Are you okay with using that? It might require some reformatting of existing files.

PR updated.

Yes, and yes. Will review this weekend. Thanks.

petejohanson commented 2 weeks ago

@maximevince Ok, after approving the GHA, it finds all the code that needs fixing after the update, indeed. Can you roll that into this PR, and we'll pull the bandaid and merge this ASAP?

Thanks!

maximevince commented 2 weeks ago

Yay! Seems green!