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

Add clang-tidy to CI #16958

Merged
merged 38 commits into from
Oct 14, 2024
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
69b8c9b
clang-tidy
vyasr May 30, 2024
c985bca
Set up pre-commit
vyasr Sep 30, 2024
01da077
Update CI
vyasr Oct 1, 2024
2b846c7
Make hook suitable for testing
vyasr Oct 1, 2024
034fcc2
Fix permissions
vyasr Oct 1, 2024
64f0ae4
Allow unbound variables
vyasr Oct 1, 2024
e17c70b
Configure with ninja
vyasr Oct 1, 2024
44ef5ac
Only run clang-tidy
vyasr Oct 1, 2024
a6008a6
Up the parallelism
vyasr Oct 1, 2024
64046a3
Update for CI failure
vyasr Oct 1, 2024
e70a752
Up the parallelism again to see
vyasr Oct 1, 2024
87547f0
Make sure nvrtc is installed
vyasr Oct 1, 2024
5561faf
Reduce parallelism
vyasr Oct 1, 2024
26aa432
Generate headers from jitify so that they're available
vyasr Oct 1, 2024
00dcbf0
Enable clang-tidy checking via CMake
vyasr Oct 8, 2024
1b2debe
Remove now obsolete script
vyasr Oct 8, 2024
7f98c52
Make clang-tidy available in the conda environment
vyasr Oct 8, 2024
051ded8
Remove the pre-commit hook
vyasr Oct 8, 2024
74ef32c
Update the CI script
vyasr Oct 8, 2024
a8dbb83
Test commit introducing a clang-tidy error
vyasr Oct 8, 2024
17f3ce9
Redefine variable
vyasr Oct 8, 2024
4b97bd1
Just inline it
vyasr Oct 8, 2024
c25da56
Set up sccache to avoid recompilation
vyasr Oct 8, 2024
c95ce44
Revert "Test commit introducing a clang-tidy error"
vyasr Oct 9, 2024
443df30
Add a version check to the CMake
vyasr Oct 9, 2024
6bcacc6
Make sure to run in parallel, it's unclear if clang-tidy is serializing
vyasr Oct 9, 2024
435d2b9
Revert "Make sure to run in parallel, it's unclear if clang-tidy is s…
vyasr Oct 9, 2024
1c51256
Add note about moving the check to nightly
vyasr Oct 9, 2024
4a0c50b
Add some notes
vyasr Oct 9, 2024
3ba5e83
Apply suggestions from code review
vyasr Oct 9, 2024
aa33ba0
Add clang back to develop environment
vyasr Oct 9, 2024
a0f60e7
Fix pr-builder
vyasr Oct 9, 2024
25e1a95
Fix dependency groups to avoid conflicts
vyasr Oct 9, 2024
7536de6
typo
vyasr Oct 9, 2024
600b406
Apply suggestions from code review
vyasr Oct 9, 2024
c4691f8
Fix style
vyasr Oct 9, 2024
8b7a356
Move workflow to nightly and fix static configure parameters
vyasr Oct 14, 2024
432265d
Merge branch 'branch-24.12' into feat/clang_tidy_ci
vyasr Oct 14, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .github/workflows/pr.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ jobs:
needs:
- changed-files
- checks
# TODO: Before merging this PR, this check needs to be moved to nightly.yaml
- clang-tidy
- conda-cpp-build
- conda-cpp-checks
- conda-cpp-tests
Expand Down Expand Up @@ -94,6 +96,12 @@ jobs:
uses: rapidsai/shared-workflows/.github/workflows/[email protected]
with:
enable_check_generated_files: false
clang-tidy:
secrets: inherit
uses: rapidsai/shared-workflows/.github/workflows/[email protected]
with:
build_type: pull-request
run_script: "ci/clang_tidy.sh"
conda-cpp-build:
needs: checks
secrets: inherit
Expand Down
29 changes: 29 additions & 0 deletions ci/clang_tidy.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#!/bin/bash
# Copyright (c) 2024, NVIDIA CORPORATION.

set -euo pipefail

rapids-logger "Create checks conda environment"
. /opt/conda/etc/profile.d/conda.sh

ENV_YAML_DIR="$(mktemp -d)"

rapids-dependency-file-generator \
--output conda \
--file-key clang_tidy \
--matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION}" | tee "${ENV_YAML_DIR}/env.yaml"

rapids-mamba-retry env create --yes -f "${ENV_YAML_DIR}/env.yaml" -n clang_tidy

# Temporarily allow unbound variables for conda activation.
set +u
conda activate clang_tidy
set -u

RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)"

source rapids-configure-sccache

# Run the build via CMake, which will run clang-tidy when CUDF_CLANG_TIDY is enabled.
cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCUDF_CLANG_TIDY=ON -GNinja
cmake --build cpp/build
2 changes: 1 addition & 1 deletion cpp/.clang-tidy
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ Checks:
-clang-analyzer-optin.core.EnumCastOutOfRange,
-clang-analyzer-optin.cplusplus.UninitializedObject'

WarningsAsErrors: ''
WarningsAsErrors: '*'
HeaderFilterRegex: '.*cudf/cpp/(src|include|tests).*'
ExcludeHeaderFilterRegex: '.*(Message_generated.h|Schema_generated.h|brotli_dict.hpp|unbz2.hpp|cxxopts.hpp).*'
FormatStyle: none
Expand Down
54 changes: 54 additions & 0 deletions cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ option(
${DEFAULT_CUDF_BUILD_STREAMS_TEST_UTIL}
)
mark_as_advanced(CUDF_BUILD_STREAMS_TEST_UTIL)
option(CUDF_CLANG_TIDY "Enable clang-tidy checking" OFF)

message(VERBOSE "CUDF: Build with NVTX support: ${USE_NVTX}")
message(VERBOSE "CUDF: Configure CMake to build tests: ${BUILD_TESTS}")
Expand Down Expand Up @@ -144,6 +145,58 @@ if(NOT CUDF_GENERATED_INCLUDE_DIR)
set(CUDF_GENERATED_INCLUDE_DIR ${CUDF_BINARY_DIR})
endif()

# ##################################################################################################
# * clang-tidy configuration ----------------------------------------------------------------------
if(CUDF_CLANG_TIDY)
find_program(
CLANG_TIDY_EXE
NAMES "clang-tidy"
DOC "Path to clang-tidy executable" REQUIRED
)

execute_process(
COMMAND ${CLANG_TIDY_EXE} --version
OUTPUT_VARIABLE CLANG_TIDY_OUTPUT
OUTPUT_STRIP_TRAILING_WHITESPACE
)
string(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+" LLVM_VERSION "${CLANG_TIDY_OUTPUT}")
# Discard the patch version and allow it to float. Empirically, results between patch versions are
# mostly stable, and looking at available packages on some package managers sometimes patch
# versions are skipped so we don't want to constrain to a patch version that the user can't
# install.
string(REPLACE "." ";" LLVM_VERSION_LIST ${LLVM_VERSION})
list(SUBLIST LLVM_VERSION_LIST 0 2 LLVM_VERSION_LIST)
list(JOIN LLVM_VERSION_LIST "." LLVM_VERSION)
vyasr marked this conversation as resolved.
Show resolved Hide resolved
set(expected_clang_tidy_version 19.1)
if(NOT expected_clang_tidy_version VERSION_EQUAL LLVM_VERSION)
message(
FATAL_ERROR
"clang-tidy version ${expected_clang_tidy_version} is required, but found ${LLVM_VERSION}"
)
endif()
endif()

# Turn on the clang-tidy property for a target excluding the files specified in SKIPPED_FILES.
function(enable_clang_tidy target)
set(_tidy_options)
set(_tidy_one_value)
set(_tidy_multi_value SKIPPED_FILES)
cmake_parse_arguments(
_TIDY "${_tidy_options}" "${_tidy_one_value}" "${_tidy_multi_value}" ${ARGN}
)

if(CUDF_CLANG_TIDY)
# clang will complain about unused link libraries on the compile line unless we specify
# -Qunused-arguments.
set_target_properties(
${target} PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_EXE};--extra-arg=-Qunused-arguments"
)
foreach(file IN LISTS _TIDY_SKIPPED_FILES)
set_source_files_properties(${file} PROPERTIES SKIP_LINTING ON)
endforeach()
endif()
endfunction()
KyleFromNVIDIA marked this conversation as resolved.
Show resolved Hide resolved

# ##################################################################################################
# * conda environment -----------------------------------------------------------------------------
rapids_cmake_support_conda_env(conda_env MODIFY_PREFIX_PATH)
Expand Down Expand Up @@ -713,6 +766,7 @@ target_compile_options(
cudf PRIVATE "$<$<COMPILE_LANGUAGE:CXX>:${CUDF_CXX_FLAGS}>"
"$<$<COMPILE_LANGUAGE:CUDA>:${CUDF_CUDA_FLAGS}>"
)
enable_clang_tidy(cudf SKIPPED_FILES src/io/comp/cpu_unbz2.cpp src/io/comp/brotli_dict.cpp)

if(CUDF_BUILD_STACKTRACE_DEBUG)
# Remove any optimization level to avoid nvcc warning "incompatible redefinition for option
Expand Down
253 changes: 0 additions & 253 deletions cpp/scripts/run-clang-tidy.py

This file was deleted.

3 changes: 3 additions & 0 deletions cpp/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ function(ConfigureTest CMAKE_TEST_NAME)
"GTEST_CUDF_STREAM_MODE=new_${_CUDF_TEST_STREAM_MODE}_default;LD_PRELOAD=$<TARGET_FILE:cudf_identify_stream_usage_mode_${_CUDF_TEST_STREAM_MODE}>"
)
endif()
if(CUDF_CLANG_TIDY)
vyasr marked this conversation as resolved.
Show resolved Hide resolved
enable_clang_tidy(${CMAKE_TEST_NAME})
endif()
vyasr marked this conversation as resolved.
Show resolved Hide resolved
endfunction()

# ##################################################################################################
Expand Down
Loading
Loading