Vulkan-Samples
Vulkan-Samples copied to clipboard
KhronosGroup
Use fixed clang-format version in scripts/clang_format.py
Description
Using different local versions of clang format can produce varying results. We should make sure that people are using the same version as the CI. This change provides more feedback to the user about which version we require
General Checklist
N/A - helper script only
Although this gives more feedback on why clang format might not be working as expected. It is still up to the user to install the correct version of clang format
We could use tools like pre-commit here which will pull the correct version before running clang format. But this does introduce more things that a contributor will need to install before they can contribute
Can we somehow make this work with "clang-format at least 15" instead of exactly 15.x? MSVC 2022 has clang-format 16, so I would actually have to downgrade my clang-format installation of MSVC which will probably break with every update.
We could bump to 16? I am not against that. Small change in the CI and a blanked PR to update all files. The issue with clang format is the small discrepancies between versions.
Another alternative is to use a tool like pre-commit which supports the correct versions out of the box without having to manually manage dependencies. Heres some of the QA plugins they have out of the box pre-commit plugins
I'm not sure if requiring a fixed version is the right way to go. Not everyone can go with recent VS versions. Is there no way to make this work with different clang-format versions? Maybe we could ease some of the requirements to make it work across different versions.
Any update on this. I did another small PR and that one always fails with clang format. It's currently my bigges pain point, as this affects all of the work I'm currently doing for the samples (except for documentation).
Maybe we should remove that check completely as a stop-gap?
Maybe we can add a CI step, that actually formats the files? See https://github.com/marketplace/actions/clang-format-action, for example.
That could be a stop-gap until we find a proper fix. The clang-format issue is a great deal of annoynance for me, and makes a lot of my changes or additions fail :(
Gave it a shot. Our limitation is the way permissions work in forks. If all branches where in this repository then the permission model would allow for auto commits. This is also outlined in the git-auto-commit-action docs
Here is the permission error i got in my tests on this branch
I will create a branch demonstrating how pre-commit can help here. You will need to install this with python (which is already a project dependency) but it will download the correct versions of software for checks locally and run on commit