CI: Add custom GitHub Actions job to run clang-tidy #5235

csadorf · 2023-02-14T13:39:02Z

Follow-up from #4983 ; should be merged after that.

Adjust clang-tidy script to only run CMAKE to generate the compile-commands db and not to actually build.
Update build dependencies #5496

ci/run_clang_tidy.sh

bdice · 2023-02-16T00:07:39Z

Hmm. I tried adding cudatoolkit to fix this problem, but to no avail. I wonder if we need to do something with include paths? I also tried adding gcc_linux-64 to make sure <omp.h> is found. We'll see. FYI @csadorf, I won't be able to look at this for about another week.

---------------- File:/__w/cuml/cuml/cpp/test/c_api/svm_api_test.c FAILED ----------------
CMD: clang-tidy -header-filter='.*cuml/cpp/(src|include|bench|comms).*' /__w/cuml/cuml/cpp/test/c_api/svm_api_test.c -- clang++ -DCUTLASS_NAMESPACE=raft_cutlass -DDISABLE_CUSPARSE_DEPRECATED -DFMT_HEADER_ONLY=1 -DFMT_SHARED -DRAFT_DISTANCE_COMPILED -DRAFT_NN_COMPILED -DRAFT_SYSTEM_LITTLE_ENDIAN=1 -DSPDLOG_FMT_EXTERNAL -DTHRUST_DEVICE_SYSTEM=THRUST_DEVICE_SYSTEM_CUDA -DTHRUST_HOST_SYSTEM=THRUST_HOST_SYSTEM_CPP -Dcuml_test_EXPORTS -I/__w/cuml/cuml/cpp/include -I/opt/conda/envs/clang_tidy/include/rapids -I/opt/conda/envs/clang_tidy/include/rapids/libcudacxx -isystem /opt/conda/envs/clang_tidy/include -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /opt/conda/envs/clang_tidy/include -O3 -DNDEBUG -fPIC -pthread -I /opt/conda/envs/clang_tidy/include/ClangHeaders /__w/cuml/cuml/cpp/test/c_api/svm_api_test.c1 error generated.
Error while processing /__w/cuml/cuml/cpp/test/c_api/svm_api_test.c.
/__w/cuml/cuml/cpp/include/cuml/cuml_api.h:21:10: error: 'cuda_runtime_api.h' file not found [clang-diagnostic-error]
#include <cuda_runtime_api.h>
         ^~~~~~~~~~~~~~~~~~~~
Found compiler error(s).
---------------- File:/__w/cuml/cuml/cpp/test/c_api/svm_api_test.c ENDS ----------------

dantegd · 2023-02-16T01:20:21Z

ci/run_clang_tidy.sh

+cmake -DGPU_ARCHS=70 \
+      -DBLAS_LIBRARIES=${CONDA_PREFIX}/lib/libopenblas.so.0 \
+      ..
+make -j treelite


why do we need to build treelite?

I had the same question for @bdice who supposed that it is likely to put some header files into place. However, I don't understand well enough how clang-tidy works to have a better explanation.

This was changed from benchmark to treelite in 52320b0 .

So, no, we do not actually build anything to run clang-tidy, but we do have to invoke CMake to create the compile command database. That's why I added a --nobuild option to our build script in 2c28995 . I assume that the previous targets where an attempt at reducing the build time, but I think not building anything is the better (and faster) approach.

Needed by default unless the --nolibcumltest option is provided to the build.sh script.

csadorf · 2023-07-11T20:25:37Z

Tagging @wphicks who previously expressed interest in this PR.

bdice · 2023-07-11T22:43:19Z

build.sh

@@ -46,6 +46,7 @@ HELP="$0 [<target> ...] [<flag> ...]
   --codecov         - Enable code coverage support by compiling with Cython linetracing
                       and profiling enabled (WARNING: Impacts performance)
   --ccache          - Use ccache to cache previous compilations
+   --nobuild         - Invoke CMake without actually building


Another name might be --configure-only. Just a proposal, no strong feelings.

Also, I prefer --no-build over --nobuild, if we choose to keep the current name.

Me, too, but this was more consistent with the other option names. Regardless, I changed it to --configure-only now.

bdice · 2023-07-11T22:46:20Z

dependencies.yaml

+        packages:
+          - clang==15.0.7
+          - clang-tools==15.0.7
+          - make


Can you use ninja instead? Tends to be faster.

csadorf · 2023-07-11T22:53:05Z

cpp/scripts/run-clang-tidy.py

@@ -239,7 +265,7 @@ def main():
    if not os.path.exists(".git"):
        raise Exception("This needs to always be run from the root of repo")
    # Check whether clang-tidy exists
-    if shutil.which("clang-tidy") is not None:
+    if shutil.which("clang-tidy") is None:
        print("clang-tidy not found. Exiting...")


Should we maybe raise an exception here instead? It seems to me that a silent pass could be problematic.

Sounds fine to me.

dantegd

One quick request

dantegd · 2023-07-11T22:31:07Z

ci/run_clang_tidy.sh

@@ -0,0 +1,22 @@
+#!/bin/bash
+# Copyright (c) 2020-2023, NVIDIA CORPORATION.


Suggested change

# Copyright (c) 2020-2023, NVIDIA CORPORATION.

# Copyright (c) 2023, NVIDIA CORPORATION.

dantegd

Forgot to submit this comment in the prior review

dantegd · 2023-07-11T22:55:42Z

ci/run_clang_tidy.sh

+
+rapids-logger "Run clang-tidy"
+
+python cpp/scripts/run-clang-tidy.py -ignore '[.]cu$|_deps|examples/kmeans/'


Can we get a section in https://github.com/rapidsai/cuml/blob/branch-23.08/CONTRIBUTING.md#code-formatting that quickly describes how to run this locally for devs?

Addressed in c4e6565 .

ajschmidt8

Approving ops-codeowner file changes

wphicks

Looks very nice! Nothing further to add beyond what other reviewers have mentioned.

To make it easier for developers to reproducibly run it.

Adds dedicated conda environment.

csadorf · 2023-07-13T21:31:19Z

/merge

csadorf added feature request New feature or request non-breaking Non-breaking change labels Feb 14, 2023

github-actions bot added ci CUDA/C++ labels Feb 14, 2023

csadorf changed the title ~~Ci: Addd custom GitHub Actions job to run clang tidy~~ Ci: Addd custom GitHub Actions job to run clang-tidy Feb 14, 2023

csadorf changed the title ~~Ci: Addd custom GitHub Actions job to run clang-tidy~~ CI: Addd custom GitHub Actions job to run clang-tidy Feb 14, 2023

csadorf self-assigned this Feb 14, 2023

csadorf mentioned this pull request Feb 14, 2023

Add pre-commit configuration #4983

Merged

bdice reviewed Feb 14, 2023

View reviewed changes

ci/run_clang_tidy.sh Outdated Show resolved Hide resolved

csadorf changed the title ~~CI: Addd custom GitHub Actions job to run clang-tidy~~ CI: Ad custom GitHub Actions job to run clang-tidy Feb 14, 2023

csadorf changed the title ~~CI: Ad custom GitHub Actions job to run clang-tidy~~ CI: Add custom GitHub Actions job to run clang-tidy Feb 14, 2023

dantegd reviewed Feb 16, 2023

View reviewed changes

csadorf mentioned this pull request Apr 14, 2023

Update clang-format to 16.0.1. #5361

Merged

csadorf mentioned this pull request Jul 3, 2023

[BUG] check for clang-tiny in the script run-clang-format.py #5485

Closed

csadorf linked an issue Jul 3, 2023 that may be closed by this pull request

[BUG] check for clang-tiny in the script run-clang-format.py #5485

Closed

csadorf changed the base branch from branch-23.04 to branch-23.08 July 3, 2023 19:49

csadorf force-pushed the ci-add-custom-ga-job-to-run-clang-tidy branch 3 times, most recently from 8a94377 to d12d082 Compare July 5, 2023 23:01

github-actions bot added the conda conda issue label Jul 6, 2023

csadorf added 2 commits July 10, 2023 12:23

Add gtest to common_build dependencies.

6019606

Needed by default unless the --nolibcumltest option is provided to the build.sh script.

Add nvcc to common_build dependencies.

ec724b3

csadorf force-pushed the ci-add-custom-ga-job-to-run-clang-tidy branch from 5e44948 to ac4bb40 Compare July 10, 2023 19:33

csadorf and others added 6 commits July 10, 2023 15:13

fixup! Add gtest to common_build dependencies.

900cf1f

Fix bug in cpp/scripts/run-clang-tidy.py script.

836cab3

Add custom GA job to run clang-tidy.

c42854c

Update clang-tools.

e1eb1f4

Add cudatoolkit to clang-tidy for cuda_runtime_api.h.

7ee6d68

Try adding gcc to get omp.h.

4657d77

csadorf marked this pull request as ready for review July 11, 2023 20:21

csadorf requested review from a team as code owners July 11, 2023 20:21

csadorf requested review from dantegd and bdice July 11, 2023 20:24

bdice reviewed Jul 11, 2023

View reviewed changes

csadorf commented Jul 11, 2023

View reviewed changes

dantegd requested changes Jul 11, 2023

View reviewed changes

ajschmidt8 approved these changes Jul 12, 2023

View reviewed changes

wphicks reviewed Jul 12, 2023

View reviewed changes

csadorf added 7 commits July 12, 2023 13:27

Fix copyright year range on run_clang_tidy.sh.

b468891

Use ninja instead of make for clang-tidy.

26cbc00

Rename --nobuild build option to --configure-only.

058bd31

Allow configuration of run-clang-tidy via pyproject.toml.

6c7d284

To make it easier for developers to reproducibly run it.

Add instructions on how to run clang-tidy locally.

c4e6565

Adds dedicated conda environment.

Return with non-zero exit code if clang-tidy is not found.

403ff5c

Adjust import order.

c3828a1

github-actions bot added the conda conda issue label Jul 12, 2023

csadorf requested a review from dantegd July 12, 2023 20:38

csadorf and others added 4 commits July 12, 2023 22:38

Merge branch 'branch-23.08' into ci-add-custom-ga-job-to-run-clang-tidy

c1da58c

Merge branch 'branch-23.08' into ci-add-custom-ga-job-to-run-clang-tidy

b8f8aac

Update environment file.

3af96ce

Add SCCACHE_S3_NO_CREDENTIALS=1 to docker run instructions.

9ff16dd

dantegd approved these changes Jul 13, 2023

View reviewed changes

rapids-bot bot merged commit 3e59b65 into rapidsai:branch-23.08 Jul 13, 2023
47 checks passed

csadorf deleted the ci-add-custom-ga-job-to-run-clang-tidy branch July 13, 2023 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: Add custom GitHub Actions job to run clang-tidy #5235

CI: Add custom GitHub Actions job to run clang-tidy #5235

csadorf commented Feb 14, 2023 •

edited

Loading

bdice commented Feb 16, 2023

dantegd Feb 16, 2023

csadorf Feb 16, 2023

csadorf Jul 11, 2023

csadorf commented Jul 11, 2023

bdice Jul 11, 2023

bdice Jul 11, 2023

csadorf Jul 12, 2023

csadorf Jul 12, 2023

bdice Jul 11, 2023

csadorf Jul 12, 2023

csadorf Jul 11, 2023

bdice Jul 11, 2023

csadorf Jul 12, 2023

dantegd left a comment

dantegd Jul 11, 2023

csadorf Jul 12, 2023

dantegd left a comment

dantegd Jul 11, 2023

csadorf Jul 12, 2023

ajschmidt8 left a comment

wphicks left a comment

csadorf commented Jul 13, 2023

		@@ -0,0 +1,22 @@
		#!/bin/bash
		# Copyright (c) 2020-2023, NVIDIA CORPORATION.

	# Copyright (c) 2020-2023, NVIDIA CORPORATION.
	# Copyright (c) 2023, NVIDIA CORPORATION.


		rapids-logger "Run clang-tidy"

		python cpp/scripts/run-clang-tidy.py -ignore '[.]cu$\|_deps\|examples/kmeans/'

CI: Add custom GitHub Actions job to run clang-tidy #5235

CI: Add custom GitHub Actions job to run clang-tidy #5235

Conversation

csadorf commented Feb 14, 2023 • edited Loading

bdice commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadorf commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantegd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantegd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajschmidt8 left a comment

Choose a reason for hiding this comment

wphicks left a comment

Choose a reason for hiding this comment

csadorf commented Jul 13, 2023

csadorf commented Feb 14, 2023 •

edited

Loading