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

clang-format 13 requirement needs documentation #126

Open
shinymerlyn opened this issue Mar 16, 2024 · 2 comments
Open

clang-format 13 requirement needs documentation #126

shinymerlyn opened this issue Mar 16, 2024 · 2 comments
Labels
documentation Improvements or additions to documentation question Further information is requested

Comments

@shinymerlyn
Copy link
Contributor

shinymerlyn commented Mar 16, 2024

After edits (see rest of thread):
This repo validates that clang-format v13 has been run on the code, and if the formatting doesn't match will mark a commit as being in error. This should be documented in CONTRIBUTING.md.

We may also want to up this requirement to the latest clang-format version, since 13 is a few years old now, and the formatting that clang-format expects changes over time. The latest version is currently 18.

Before edits:
I tried using Tools/format_source.sh on Arch. I had clang-format version 17.0.6 installed
I tried using clang-format from the scoop install llvm package on Windows. Version 18.1.1

Neither of them seem to be matching the clang-format settings that our continuous integration expects.

We should more clearly document whatever our clang-format CI is using. It should be part of CONTRIBUTING.md

@feliwir @redruin1 - I think you may have set it up?

@shinymerlyn
Copy link
Contributor Author

I saw a file <root>/.clang-format, but I ran clang-format with that file on the source tree and ended up with zero corrections, even though the latest main build failed the analysis. So the analyzer doesn't seem to be using that config.

@shinymerlyn
Copy link
Contributor Author

Aha. It was because the version specified in the module is clang-format version 13, and apparently clang-format reserves the right to constantly tweak their formatting or something.

Wondering if we should update to v18 since that's the latest?

Still needs to be documented. I will convert this ticket to a documentation ticket, for CONTRIBUTING.md.

I may also want to check in some binaries for windows and a batch file for running it on the whole repo. It's kind of more of a pain on Windows to get the right version of things, since Windows package management has never really taken off. For example, I had to file bugs against scoop install llvm@13.0.1 since that was broken. Maybe chocolatey is better. But none of them are nearly as well trodden as linux/mac package managers.

@shinymerlyn shinymerlyn added documentation Improvements or additions to documentation question Further information is requested labels Mar 18, 2024
@shinymerlyn shinymerlyn changed the title Clang format verification may be using some weird combo of clang format settings clang-format 13 requirement needs documentation Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation question Further information is requested
Projects
None yet
Development

No branches or pull requests

1 participant