KhronosGroup / Vulkan-Samples

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

feat: Add pre-commit #939

Closed tomadamatkinson closed 8 months ago

tomadamatkinson commented 8 months ago

Description

Many contributors have grown tired of the QA process in the repository. I don't blame them... its not ideal!! I attempted to resolve this by automatic commits but as most contributions come from forks this is not viable (See #895). This PR takes a second approach using git hooks. pre-commit adds a config file driven approach which pulls the tools needed for each check. This should help resolve the version discrepancies found with clang_format.py.

Adds pre-commit a QA tool that runs checks on commit. The clang-format check makes sure that files are formatted correct before a commit is created.

image

If files are not correctly formatted pre-commit will reject the commit

image

The formatted changes are given to the user as an unstaged change

image

Adding the files and created the commit will then pass the checks. As i made no change other than the format issues (which pre-commit fixed) the checks did not need to run the second time around

image

Note, in the future we could auto resolve copyright check issues using the same solution. Pre-commit also comes with a large selection of supported hooks so there is room for expansion with this approach

tomadamatkinson commented 8 months ago

I cant add you as a reviewer but @jherico I would appreciate your feedback on this approach also!

SaschaWillems commented 8 months ago

Just a quick question before trying this: What version of clang format will this use? The one installed locally? And if so, what's with MSVC users where clang-format isn't available via path by default?

tomadamatkinson commented 8 months ago

@SaschaWillems this should download the correct version. I have clang-17 locally but this check runs clang-15

tomadamatkinson commented 8 months ago

If we adopt this tool i will create a second PR running pre-commit run --all-files. Currently this changes quite a few as it adds whitespace at the end of a lot of files. I would also want to add the large file check and some others in that second PR

jherico commented 8 months ago

Looks good.

tomadamatkinson commented 8 months ago

pre-commit can also be installed in other ways. Python is likely the simplest for most users. But yeah this small hiccup can happen with python due to the many different ways python can be installed on a system.

If we used hooks without pre-commit or another tool we will run into the same issue of incorrectly installed tools. The benefit of pre-commit is its ability to pull the correct tools when needed. A general hook written by ourselves would also take a lot more time to maintain and would not be extensible

Pre-commit also does not need to be installed for a user to be able to commit. We can run pre-commit in the CI on PRs (as a check, not an auto commit). This means for users that want a smoother contribution experience, installing and using pre-commit could save them a lot of pain in the long run

I use pre-commit across many repositories and tech stacks professionally. It has hooks for most things you need and you can add local scripts just as easily (copyright fixes could be made automatic).

SaschaWillems commented 8 months ago

This works, and as a stop-gap this might be an option. It just feels odd, esp. if you commit from IDEs like Visual Studio, where it looks like this on commit:

image

SaschaWillems commented 8 months ago

Or could we (also) add this as e.g. a pre- or post-build event?

tomadamatkinson commented 8 months ago

You should be able to run pre-commit at any point. So we could automate this further than just a git hook

marty-johnson59 commented 8 months ago

Merging - 3 approvals