KhronosGroup / Vulkan-Samples

One stop solution for all Vulkan samples
Apache License 2.0
4.34k stars 648 forks source link

Use fixed clang-format version in scripts/clang_format.py #895

Closed tomadamatkinson closed 8 months ago

tomadamatkinson commented 9 months ago

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

tomadamatkinson commented 9 months ago

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

SaschaWillems commented 9 months ago

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.

tomadamatkinson commented 9 months ago

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

SaschaWillems commented 9 months ago

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.

SaschaWillems commented 9 months ago

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?

asuessenbach commented 8 months ago

Maybe we can add a CI step, that actually formats the files? See https://github.com/marketplace/actions/clang-format-action, for example.

SaschaWillems commented 8 months ago

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 :(

tomadamatkinson commented 8 months ago

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

image

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

tomadamatkinson commented 8 months ago

Closing as not a viable solution for #937