Integrate clang-tidy into CI#112785
Conversation
Ivorforce
force-pushed
the
clang-tidy-ci
branch
from
a9468da to
7a1a95c
Compare
Adding 15 min of build time to CI for every single workflow might end up costing a lot of time. And it sounds like it's a minimum and it would grow once we enable more checks and can integrate more files in the testing. Based on the name At least judging by the minimal changes in this PR after months of not having run clang-tidy, I don't think paying 15 min per workflow to catch this kind of rare issues would be a good tradeoff. |
|
The 15 minute runtime is only needed for running it against the entire codebase. PRs will only need to check changed files, which is usually completed in a few seconds. Besides, this is only the minimum of functionality we might want, kept simple to be mergable. We'll probably want to integrate more of clang-tidy after. |
Ivorforce
force-pushed
the
clang-tidy-ci
branch
3 times, most recently
from
46b1515 to
99745ae
Compare
|
Alright, it should be ready. That being said, I have discovered I was somewhat wrong. While running I might be OK with moving this to a cron job instead. But we should be aware that, if we decide to make heavy use of |
That's indeed what's happening. iirc, |
This comment was marked as outdated.
This comment was marked as outdated.
|
Running with I fixed them with: diff --git a/.clang-tidy b/.clang-tidy
index 7e43549f9c..6fb4741919 100644
--- a/.clang-tidy
+++ b/.clang-tidy
@@ -12,7 +12,7 @@ ImplementationFileExtensions: [c, cc, cpp, cxx, m, mm, java]
HeaderFilterRegex: (core|doc|drivers|editor|main|modules|platform|scene|servers|tests)/
FormatStyle: file
CheckOptions:
- modernize-deprecated-headers.CheckHeaderFile: true
- modernize-use-bool-literals.IgnoreMacros: false
- modernize-use-default-member-init.IgnoreMacros: false
- modernize-use-default-member-init.UseAssignment: true
+ - modernize-deprecated-headers.CheckHeaderFile: true
+ - modernize-use-bool-literals.IgnoreMacros: false
+ - modernize-use-default-member-init.IgnoreMacros: false
+ - modernize-use-default-member-init.UseAssignment: trueIt's weird though as the syntax used before is the one shown in the After these it seems to run, though it's super slow. It's been running over 40 min on a single core and not done yet. Edit: I tried to remove |
| platform: linuxbsd | ||
| target: ${{ matrix.target }} | ||
|
|
||
| - name: Style checks via pre-commit | ||
| if: matrix.clang-tidy | ||
| uses: pre-commit/action@v3.0.1 |
There was a problem hiding this comment.
We can keep it general so we potentially get patch releases automatically.
| uses: pre-commit/action@v3.0.1 | |
| uses: pre-commit/action@v3 |
There was a problem hiding this comment.
I tried this but it didn't work.
| # TODO Platform-specific subfolders currently fail, we should try to include them | ||
| files: ^(core|main|scene)/.*\.(c|h|cpp|hpp|cc|hh|cxx|hxx|m|mm|java)$ | ||
| # No unknown warning suppression used for easier compatibility with gcc | ||
| args: [--fix, --quiet, --use-color, -p=compile_commands.json, -extra-arg=-Wno-unknown-warning-option] | ||
| types_or: [text] | ||
| additional_dependencies: [clang-tidy==20.1.0] |
There was a problem hiding this comment.
We could bump the version to 21.1.8 which is the current latest.
There was a problem hiding this comment.
The latest my pre-commit can find is 21.1.6, which we already use (after rebase).
There was a problem hiding this comment.
Specifically: pre-commit pulls from the pip upload of clang-tidy, which is indeed 21.1.6 at time of writing
Ivorforce
force-pushed
the
clang-tidy-ci
branch
4 times, most recently
from
a2eb271 to
bf0407b
Compare
|
I rebased the branch onto latest master and tested the build again (one new issue came up which I fixed). @akien-mga I don't get the "CheckOptions" issue. It's especially confusing because pre-commit enforces a specific Regarding require-serial, |
|
It's very weird that it's so much slower for me. What OS are you on and which compiler and compile options are you using (in case this impacts what clang-tidy makes of the compiledb)? I'm on Linux with GCC, on an AMD Ryzen 7 8845HS with 64 GiB RAM, which shouldn't be too shabby. |
|
Running with the latest version of this PR, with no local modifications, I get: |
Ivorforce
force-pushed
the
clang-tidy-ci
branch
from
bf0407b to
04441bb
Compare
I'm using macOS Tahoe (26), on an M1 Max with builtin apple clang.
I don't get this one either. I don't know much about clang-tidy but going by the name, I'd expect it to use |
|
Note that #112850 makes it possible for scons to generate only a compilation database via |
I just tested now with a compiledb generated using clang ( So it seems to be choking on GCC specific stuff maybe that slows it down? Maybe the errors I ran into also come from GCC specific flags? I'm not sure how clang-tidy uses the compiledb to set its own parameters. I remember playing with include-what-you-use years ago and it basically acted like a compiler, but clang-tidy doesn't seem to work exactly like that. Here's an example of flags used for Here's the diff between our GCC and Clang build flags (in this config): and gcc: Edit: Tested further with This shouldn't be a blocker for this PR. |
| # Keep in sync with static_checks.yml | ||
| - name: Get changed files | ||
| if: matrix.clang-tidy | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| if [ "${{ github.event_name }}" == "pull_request" ]; then | ||
| files=$(git diff-tree --no-commit-id --name-only -r HEAD^1..HEAD 2> /dev/null || true) | ||
| elif [ "${{ github.event_name }}" == "push" -a "${{ github.event.forced }}" == "false" -a "${{ github.event.created }}" == "false" ]; then | ||
| files=$(git diff-tree --no-commit-id --name-only -r ${{ github.event.before }}..${{ github.event.after }} 2> /dev/null || true) | ||
| fi | ||
| files=$(echo "$files" | xargs -I {} sh -c 'echo "\"./{}\""' | tr '\n' ' ') | ||
| echo "CHANGED_FILES=$files" >> $GITHUB_ENV |
There was a problem hiding this comment.
So now that we have compiledb_gen_only (once you rebase on latest master), maybe the clang-tidy job could be moved to static_checks.yml?
There was a problem hiding this comment.
That would require the static checks pipeline to install build tools, I think. Another reason I added it to the linux runner is that I expect it to trigger rarely (hopefully?), so normally there should be no reason to delay all the other runners for it.
Repiteo
left a comment
Repiteo
left a comment
There was a problem hiding this comment.
A rock-solid baseline for finally integrating this into the CI proper. I'd much rather have something implemented than being stuck in this limbo of waiting for an "ideal" integration (my b). After one nitpick is dealt with, this is good to go!
For now, it's integrated into core, main and scene only. Fix a few superficial clang-tidy failures.
Ivorforce
force-pushed
the
clang-tidy-ci
branch
from
04441bb to
6e345f8
Compare
|
Thanks! |
3 participants
Clang-tidy is the bigger, more powerful sibling of
clang-format. The main difference is thatclang-formatparses files in isolation, and focuses on linting features, whileclang-tidytrudges into the first steps of compilation, and can use more advanced information.We already use
clang-format. This PR introducesclang-tidy(in minimal form).Motivation
I think we want (need!)
clang-tidyfor its many great codebase-quality related features, such as:nullptroverNULL).clang-tidy Initial State
Building on Repiteo's initial implementation, I focused on getting the "simplest" version of clang-tidy integrated that I could.
However, I needed to nerf it severely for it to run:
With everything nerfed, and problems fixed, the following command succeeded (after about 15m of runtime):
I also removed
cppcoreguidelines-pro-type-member-initfor good, because it just generated false positives (as far as i can judge - it suggested usingClass some_class{}instead ofClass some_classfor using the default initializer.CI implementation
clang-tidymakes use ofcompile_commands.jsonto run properly. That means users will have to have it up-to-date forpre-committo run as expected.This is probably not acceptable in the standard workflow, so I kept the
clang-tidypre-commithook as manual.To integrate it, I add
compiledb=yesto the build command of Linux builders, and let the fastest linux builder (Minimal) take the burden of running it. It should not make a difference on the actual runtime of CI.Changes made
All necessary because of
clang-tidyfailures:staticfree functions changed toinline.staticforces compile units to keep their own version, leading to unnecessary engine bloat (and an "unused function" warning)#include <type_traits>changed to#include <utility>#include <stdint.h>changed to C++ header#include <cstdint>. In theory, this should reduce the number of ambiguous/error-prone free functions (but in practice, it doesn't make a difference in stdlib implementations)