Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

tomadamatkinson
Copy link
Collaborator

@tomadamatkinson tomadamatkinson commented Feb 7, 2024

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
Copy link
Collaborator Author

tomadamatkinson commented Feb 7, 2024

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
Copy link
Collaborator

SaschaWillems commented Feb 7, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor

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

@SaschaWillems
Copy link
Collaborator

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 tomadamatkinson force-pushed the clang-format-fixes branch 10 times, most recently from 1427a47 to 7da0101 Compare February 24, 2024 15:50
@tomadamatkinson
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Closing as not a viable solution for #937

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants