Clang tidy separate ci job #1801

bavulapati · 2025-07-02T07:37:11Z

We use a CMAKE flag to decide if we want to run clang-tidy.
We can run the clang-tidy checks locally using the flag SOURCEMETA_CORE_CLANG_TIDY.

Tested this locally on Mac too. We just need to make sure we are using llvm instead of apple clang++.
Clang-tidy is friendly with clang++ but not with gcc or apple clang++.

This PR makes sure that it runs on a single machine, as we can't have clang used on all the machines.
Choosing ubuntu as its availability and cost on CI might be less(subject to correction).

We are using a single .clang-tidy configuration file. And the configuration file path is explicitly specified.

We are enabling the clang-tidy checks only for sourcemeta source targets. Disabling it for vendor, tests, etc.,

New CI Job installs the llvm package.

Using modernize-use-nodiscard check to demonstrate the proper clang-tidy working.

See: sourcemeta/blaze#429 Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati · 2025-07-02T13:38:25Z

@jviotti it's ready for your review.

bavulapati · 2025-07-02T13:53:29Z

This supersedes #1754

bavulapati · 2025-07-02T13:58:39Z

Also supersedes #1640

bavulapati · 2025-07-02T13:59:25Z

Also supersedes #1702

bavulapati · 2025-07-03T16:16:33Z

@jviotti PTAL

jviotti · 2025-07-03T18:09:12Z

.github/workflows/ci.yml

+      - uses: actions/checkout@v4
+      - run: cmake --version
+      - name: Install llvm
+        run: sudo bash -c "$(wget -O - https://apt.llvm.org/llvm.sh)"


Why do we need this? I thought we tweaked the CMake scripts to conveniently download ClangTidy binaries when configuring?

jviotti · 2025-07-03T18:10:55Z

cmake/common/targets/clang-tidy.cmake

-       OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE)
-  message(STATUS "Installed `clang-tidy` pre-built binary to ${CLANG_TIDY_BINARY_OUTPUT}")
+function(sourcemeta_enable_clang_tidy)
+  find_program(CLANG_TIDY_BIN NAMES clang-tidy)


We could still download ClangTidy on demand and set CMAKE_CXX_CLANG_TIDY to the downloaded files?

Having clang-tidy without rest of the llvm is causing trouble

Also keep in mind that this find_program invocation succeeds even if clang-tidy was not found. Maybe worth putting REQUIRED? Otherwise you might end up setting a non-sense CMAKE_CXX_CLANG_TIDY?

Having clang-tidy without rest of the llvm is causing trouble

@bavulapati Even when installing clang-tidy from the Ubuntu packages? Because in theory Ubuntu consumes upstream LLVM.

We can try that

cmake/common/targets/clang-tidy.cmake

jviotti · 2025-07-03T18:11:52Z

cmake/common/targets/clang-tidy.cmake

-  message(STATUS "Installed `clang-tidy` pre-built binary to ${CLANG_TIDY_BINARY_OUTPUT}")
+function(sourcemeta_enable_clang_tidy)
+  find_program(CLANG_TIDY_BIN NAMES clang-tidy)
+  set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_BIN};--config-file=${CMAKE_CURRENT_SOURCE_DIR}/.clang-tidy;-header-filter=${CMAKE_CURRENT_SOURCE_DIR}/src/*" PARENT_SCOPE)


It's probably fine for all projects right now, but maybe worth taking the -header-filter as an option to the CMake function? At least so that it is generic enough out of the box for all possible uses?

jviotti · 2025-07-03T18:13:59Z

CMakeLists.txt

 endif()

+sourcemeta_disable_clang_tidy()


Interesting. Disabling looks a bit weird though. Now that I see this I wonder if it is possible to set CMAKE_CXX_CLANG_TIDY on a per-target level. If so, we could have a CLANG_TIDY option when declaring a library with sourcemeta_library (or executable with sourcemeta_executable). Then we don't have to worry about unsetting it, as we literally opt into it for specific things we are about?

Yes, we can do it at the target level

jviotti · 2025-07-03T18:17:26Z

cmake/common/targets/clang-tidy.cmake

-  message(STATUS "Installed `clang-tidy` pre-built binary to ${CLANG_TIDY_BINARY_OUTPUT}")
+function(sourcemeta_enable_clang_tidy)
+  find_program(CLANG_TIDY_BIN NAMES clang-tidy)
+  set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_BIN};--config-file=${CMAKE_CURRENT_SOURCE_DIR}/.clang-tidy;-header-filter=${CMAKE_CURRENT_SOURCE_DIR}/src/*" PARENT_SCOPE)


Also, does this work across compilers? i.e. can I enable this if I have ClangTidy available even if using MSVC? Or does it also require the Clang compiler? If so, it might be worth checking for this, so that nobody can accidentally enable it

jviotti · 2025-07-03T18:23:20Z

We just need to make sure we are using llvm instead of apple clang++.Clang-tidy is friendly with clang++ but not with gcc or apple clang++.

It seems to work fine for me with AppleClang. Can you confirm it fails with non-Clang compilers? If so, we should have a check in the CMake function to error out if somebody tries to enable it with an incompatible compiler

bavulapati · 2025-07-03T18:26:08Z

It works but misses the required context like std headers location etc.,

Co-authored-by: Juan Cruz Viotti <jv@jviotti.com> Signed-off-by: Balakrishna Avulapati <bavulapati@gmail.com>

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

…re into clang-tidy-separate-ci-job

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati · 2025-07-04T10:54:08Z

@jviotti Addressed critical review comments.
Focusing on the critical parts and keeping the following list of optimisations, let me know if there's any other critical parts.

passing header-filter as parameter to cmake function
enabling clang-tidy at target level

jviotti and others added 16 commits July 2, 2025 18:21

Run ClangTidy on GitHub Actions

55ea0c0

See: sourcemeta/blaze#429 Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>

use llvm

8c698ea

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

run llvm installation as root

e48ea52

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

use sudo

c845be0

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

remove redundant target call

6ee9722

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

use macos instead of ubuntu

0c9e0d8

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

use brew

f7a9f53

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

use llvm path

291bc33

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

fix path

12b3e3d

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

use a step to set path

70b925b

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

disable other jobs while you test the changes

1bf2001

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

use bash

593833d

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

debug path

d334af4

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

use Github path

996c687

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

use ubuntu

81bc9ab

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

fix modernize-use-nodiscard checks

bdac96c

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati force-pushed the clang-tidy-separate-ci-job branch from 89ec116 to bdac96c Compare July 2, 2025 12:53

bavulapati added 3 commits July 2, 2025 18:27

remove verbose steps

dace288

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

use clang compiler

e1203af

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

re-enable the ci jobs

eef5869

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

jviotti reviewed Jul 3, 2025

View reviewed changes

cmake/common/targets/clang-tidy.cmake Outdated Show resolved Hide resolved

jviotti reviewed Jul 3, 2025

View reviewed changes

bavulapati and others added 5 commits July 4, 2025 16:00

Update cmake/common/targets/clang-tidy.cmake

cbd24f8

Co-authored-by: Juan Cruz Viotti <jv@jviotti.com> Signed-off-by: Balakrishna Avulapati <bavulapati@gmail.com>

install llvm via ubuntu packages

bc7cdda

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

Merge branch 'clang-tidy-separate-ci-job' of github.com:bavulapati/co…

9c7d718

…re into clang-tidy-separate-ci-job

make clang-tidy required on ci job

5a00eca

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

Error out if clang-tidy is invoked without clang tool chain

e9369fe

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

		endif()

		sourcemeta_disable_clang_tidy()

Uh oh!

Clang tidy separate ci job #1801

Are you sure you want to change the base?

Clang tidy separate ci job #1801

Uh oh!

Conversation

bavulapati commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bavulapati commented Jul 2, 2025

Uh oh!

bavulapati commented Jul 2, 2025

Uh oh!

bavulapati commented Jul 2, 2025

Uh oh!

bavulapati commented Jul 2, 2025

Uh oh!

bavulapati commented Jul 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jviotti commented Jul 3, 2025

Uh oh!

bavulapati commented Jul 3, 2025

Uh oh!

bavulapati commented Jul 4, 2025

Uh oh!

Uh oh!

bavulapati commented Jul 2, 2025 •

edited

Loading