From 69b8c9b90af0d1c7af9c90abcea9c7d405a1d968 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Thu, 30 May 2024 07:39:40 +0000 Subject: [PATCH 01/37] clang-tidy --- .pre-commit-config.yaml | 10 +++++++ cpp/scripts/run-clang-tidy.py | 54 +++++++++++++++++++++++++++++------ 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index f861fb57916..c82cff0f6b5 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -125,6 +125,16 @@ repos: language: system pass_filenames: false verbose: true + - id: clang-tidy + name: clang-tidy + entry: find cpp/{src,tests} -name *.cpp | grep -v -E ".*(cpu_unbz2.cpp|brotli_dict.cpp).*" | xargs -n 1 -P 60 clang-tidy -p cpp/build/latest --extra-arg="-Qunused-arguments" > out 2>&1 + language: python + types_or: [c, c++] + # Note that pre-commit autoupdate does not update the versions + # of dependencies, so we'll have to update this manually. + additional_dependencies: + - clang-tidy==19.1.0 + pass_filenames: false - repo: https://github.com/codespell-project/codespell rev: v2.2.6 hooks: diff --git a/cpp/scripts/run-clang-tidy.py b/cpp/scripts/run-clang-tidy.py index e5e57dbf562..bd442755251 100644 --- a/cpp/scripts/run-clang-tidy.py +++ b/cpp/scripts/run-clang-tidy.py @@ -1,4 +1,4 @@ -# Copyright (c) 2021-2023, NVIDIA CORPORATION. +# Copyright (c) 2021-2024, NVIDIA CORPORATION. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -22,30 +22,36 @@ import shutil -EXPECTED_VERSION = "16.0.6" +EXPECTED_VERSION = "19.1.0" VERSION_REGEX = re.compile(r" LLVM version ([0-9.]+)") GPU_ARCH_REGEX = re.compile(r"sm_(\d+)") SPACES = re.compile(r"\s+") SEPARATOR = "-" * 16 +VENDORED_SOURCES = ["cpu_unbz2.cpp", "brotli_dict.cpp"] +VENDORED_HEADERS = ( + ".*(Message_generated.h|Schema_generated.h|brotli_dict.hpp|unbz2.hpp|cxxopts.hpp).*" +) def parse_args(): argparser = argparse.ArgumentParser("Runs clang-tidy on a project") - argparser.add_argument("-cdb", type=str, + argparser.add_argument("cdb", type=str, # TODO This is a hack, needs to be fixed default="cpp/build/cuda-11.5.0/clang-tidy/release/compile_commands.clangd.json", help="Path to cmake-generated compilation database" " file. It is always found inside the root of the " "cmake build folder. So make sure that `cmake` has " "been run once before running this script!") - argparser.add_argument("-exe", type=str, default="clang-tidy", + argparser.add_argument("--exe", type=str, default="clang-tidy", help="Path to clang-tidy exe") - argparser.add_argument("-ignore", type=str, default="[.]cu$|examples/kmeans/", + argparser.add_argument("--ignore", type=str, default="[.]cu$|examples/kmeans/", help="Regex used to ignore files from checking") - argparser.add_argument("-select", type=str, default=None, + argparser.add_argument("--select", type=str, default=None, help="Regex used to select files for checking") argparser.add_argument("-j", type=int, default=-1, help="Number of parallel jobs to launch.") + argparser.add_argument("--fix", action="store_true", + help="Apply fixes to the files. Default is False.") args = argparser.parse_args() if args.j <= 0: args.j = mp.cpu_count() @@ -105,13 +111,36 @@ def remove_item_plus_one(arr, item): def get_clang_includes(exe): + includes = [] + for compiler_var, language in ("CC", "c"), ("CXX", "c++"): + if (compiler := os.getenv(compiler_var)) is not None: + in_includes = False + result = subprocess.run( + [f"{compiler}", "-E", "-Wp,-v", f"-x{language}", "/dev/null"], + check=True, + capture_output=True, + ) + for line in result.stderr.decode().splitlines(): + if "include" in line and "search starts here" in line: + in_includes = True + continue + if line.startswith("End of search list."): + in_includes = False + continue + if in_includes: + includes.append("-isystem") + includes.append(line.strip()) + dir = os.getenv("CONDA_PREFIX") if dir is None: ret = subprocess.check_output("which %s 2>&1" % exe, shell=True) ret = ret.decode("utf-8") dir = os.path.dirname(os.path.dirname(ret)) header = os.path.join(dir, "include", "ClangHeaders") - return ["-I", header] + includes.append("-isystem") + includes.append(header) + + return includes def get_tidy_args(cmd, exe): @@ -159,7 +188,12 @@ def run_clang_tidy(cmd, args): command, is_cuda = get_tidy_args(cmd, args.exe) tidy_cmd = [args.exe, "-header-filter='.*cudf/cpp/(src|include|bench|comms).*'", - cmd["file"], "--", ] + f"-exclude-header-filter='{VENDORED_HEADERS}'", + "--extra-arg='-Qunused-arguments'", + cmd["file"]] + if args.fix: + tidy_cmd.append("--fix-errors") + tidy_cmd.append("--") tidy_cmd.extend(command) status = True out = "" @@ -214,7 +248,9 @@ def run_tidy_for_all_files(args, all_files): pool = None if args.j == 1 else mp.Pool(args.j) # actual tidy checker for cmd in all_files: - # skip files that we don't want to look at + # skip files in the build directory + if '/_deps/' in cmd["file"] or any(f in cmd["file"] for f in VENDORED_SOURCES): + continue if args.ignore_compiled is not None and \ re.search(args.ignore_compiled, cmd["file"]) is not None: continue From c985bcafdbff4182bb628ef4ea502366f58c1297 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Mon, 30 Sep 2024 23:57:42 +0000 Subject: [PATCH 02/37] Set up pre-commit --- .pre-commit-config.yaml | 3 ++- cpp/scripts/run-clang-tidy.sh | 40 +++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100755 cpp/scripts/run-clang-tidy.sh diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index c82cff0f6b5..72936eb8fff 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -127,7 +127,7 @@ repos: verbose: true - id: clang-tidy name: clang-tidy - entry: find cpp/{src,tests} -name *.cpp | grep -v -E ".*(cpu_unbz2.cpp|brotli_dict.cpp).*" | xargs -n 1 -P 60 clang-tidy -p cpp/build/latest --extra-arg="-Qunused-arguments" > out 2>&1 + entry: ./cpp/scripts/run-clang-tidy.sh language: python types_or: [c, c++] # Note that pre-commit autoupdate does not update the versions @@ -135,6 +135,7 @@ repos: additional_dependencies: - clang-tidy==19.1.0 pass_filenames: false + stages: [manual] - repo: https://github.com/codespell-project/codespell rev: v2.2.6 hooks: diff --git a/cpp/scripts/run-clang-tidy.sh b/cpp/scripts/run-clang-tidy.sh new file mode 100755 index 00000000000..6dd3f3bb332 --- /dev/null +++ b/cpp/scripts/run-clang-tidy.sh @@ -0,0 +1,40 @@ +#!/bin/bash +# Copyright (c) 2024, NVIDIA CORPORATION. + +# This script is a wrapper for clang-tidy for use with pre-commit. The +# wrapping serves a few purposes: +# 1. It allows us to find the files to run clang-tidy on rather than relying on pre-commit to pass them in. +# 2. It allows us to fail gracefully if no compile commands database is found. +# +# This script can be invoked directly anywhere within the project repository. +# Alternatively, it may be invoked as a pre-commit hook via +# `pre-commit run clang-tidy`. +# +# Usage: +# bash run-clang-tidy.sh + +status=0 +if [ -z ${CUDF_ROOT:+PLACEHOLDER} ]; then + CUDF_BUILD_DIR=$(git rev-parse --show-toplevel 2>&1)/cpp/build + status=$? +else + CUDF_BUILD_DIR=${CUDF_ROOT} +fi + +if ! [ ${status} -eq 0 ]; then + if [[ ${CUDF_BUILD_DIR} == *"not a git repository"* ]]; then + echo "This script must be run inside the cudf repository, or the CUDF_ROOT environment variable must be set." + else + echo "Script failed with unknown error attempting to determine project root:" + echo ${CUDF_BUILD_DIR} + fi + exit 1 +fi + +if [ ! -f "${CUDF_BUILD_DIR}/compile_commands.json" ]; then + echo "No compile commands database found. Please run CMake with -DCMAKE_EXPORT_COMPILE_COMMANDS=ON." + exit 1 +fi + + +find cpp/{src,tests} -name *.cpp | grep -v -E ".*(cpu_unbz2.cpp|brotli_dict.cpp).*" | head -1 | xargs -n 1 clang-tidy -p ${CUDF_BUILD_DIR} --extra-arg="-Qunused-arguments" > out 2>&1 From 01da077d57638ae781d4e6a3ab41afa8cf874464 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 1 Oct 2024 00:08:22 +0000 Subject: [PATCH 03/37] Update CI --- .github/workflows/pr.yaml | 7 +++++ ci/clang_tidy.sh | 26 +++++++++++++++++++ .../all_cuda-118_arch-x86_64.yaml | 2 -- .../all_cuda-125_arch-x86_64.yaml | 2 -- dependencies.yaml | 9 +++++-- 5 files changed, 40 insertions(+), 6 deletions(-) create mode 100644 ci/clang_tidy.sh diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index bc237cc73b0..e6c783638db 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -14,6 +14,7 @@ jobs: needs: - changed-files - checks + - clang_tidy - conda-cpp-build - conda-cpp-checks - conda-cpp-tests @@ -94,6 +95,12 @@ jobs: uses: rapidsai/shared-workflows/.github/workflows/checks.yaml@branch-24.12 with: enable_check_generated_files: false + clang_tidy: + secrets: inherit + uses: rapidsai/shared-workflows/.github/workflows/custom-job.yaml@branch-24.12 + with: + build_type: pull-request + run_script: "ci/clang_tidy.sh" conda-cpp-build: needs: checks secrets: inherit diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh new file mode 100644 index 00000000000..31ab98ff119 --- /dev/null +++ b/ci/clang_tidy.sh @@ -0,0 +1,26 @@ +#!/bin/bash +# Copyright (c) 2020-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 +conda activate clang_tidy + +RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)" + +# Run the CMake configure step and set the build directory for clang-tidy. +cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON +export CUDF_ROOT="${PWD}/cpp/build" + +# Run pre-commit checks +pre-commit run --all-files --show-diff-on-failure --hook-stage manual diff --git a/conda/environments/all_cuda-118_arch-x86_64.yaml b/conda/environments/all_cuda-118_arch-x86_64.yaml index bd5e6c3d569..af9f4e65274 100644 --- a/conda/environments/all_cuda-118_arch-x86_64.yaml +++ b/conda/environments/all_cuda-118_arch-x86_64.yaml @@ -13,8 +13,6 @@ dependencies: - breathe>=4.35.0 - c-compiler - cachetools -- clang-tools=16.0.6 -- clang==16.0.6 - cmake>=3.26.4,!=3.30.0 - cramjam - cubinlinker diff --git a/conda/environments/all_cuda-125_arch-x86_64.yaml b/conda/environments/all_cuda-125_arch-x86_64.yaml index 565a3ebfa3c..ec2df5a11e8 100644 --- a/conda/environments/all_cuda-125_arch-x86_64.yaml +++ b/conda/environments/all_cuda-125_arch-x86_64.yaml @@ -13,8 +13,6 @@ dependencies: - breathe>=4.35.0 - c-compiler - cachetools -- clang-tools=16.0.6 -- clang==16.0.6 - cmake>=3.26.4,!=3.30.0 - cramjam - cuda-cudart-dev diff --git a/dependencies.yaml b/dependencies.yaml index ca17917c905..475d94adfdf 100644 --- a/dependencies.yaml +++ b/dependencies.yaml @@ -86,6 +86,13 @@ files: includes: - develop - py_version + clang_tidy: + output: none + includes: + - build_all + - build_base + - develop + - py_version docs: output: none includes: @@ -555,8 +562,6 @@ dependencies: - identify>=2.5.20 - output_types: conda packages: - - clang==16.0.6 - - clang-tools=16.0.6 - &doxygen doxygen=1.9.1 # pre-commit hook needs a specific version. docs: common: From 2b846c7eaaf46e376b78792de3d8369f46d57616 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 1 Oct 2024 00:09:53 +0000 Subject: [PATCH 04/37] Make hook suitable for testing --- .pre-commit-config.yaml | 1 + cpp/scripts/run-clang-tidy.sh | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 72936eb8fff..1d5b59f880b 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -136,6 +136,7 @@ repos: - clang-tidy==19.1.0 pass_filenames: false stages: [manual] + verbose: true - repo: https://github.com/codespell-project/codespell rev: v2.2.6 hooks: diff --git a/cpp/scripts/run-clang-tidy.sh b/cpp/scripts/run-clang-tidy.sh index 6dd3f3bb332..de432bfeb99 100755 --- a/cpp/scripts/run-clang-tidy.sh +++ b/cpp/scripts/run-clang-tidy.sh @@ -37,4 +37,4 @@ if [ ! -f "${CUDF_BUILD_DIR}/compile_commands.json" ]; then fi -find cpp/{src,tests} -name *.cpp | grep -v -E ".*(cpu_unbz2.cpp|brotli_dict.cpp).*" | head -1 | xargs -n 1 clang-tidy -p ${CUDF_BUILD_DIR} --extra-arg="-Qunused-arguments" > out 2>&1 +find cpp/{src,tests} -name *.cpp | grep -v -E ".*(cpu_unbz2.cpp|brotli_dict.cpp).*" | xargs -n 1 -P 10 clang-tidy -p ${CUDF_BUILD_DIR} --extra-arg="-Qunused-arguments" From 034fcc229c462a661ada3f32a37c66da3bfa639f Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 1 Oct 2024 00:13:50 +0000 Subject: [PATCH 05/37] Fix permissions --- ci/clang_tidy.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 ci/clang_tidy.sh diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh old mode 100644 new mode 100755 From 64f0ae453d14015f9f10b5ac47fc3babc69e4a93 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 1 Oct 2024 00:18:07 +0000 Subject: [PATCH 06/37] Allow unbound variables --- ci/clang_tidy.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh index 31ab98ff119..576c0a32060 100755 --- a/ci/clang_tidy.sh +++ b/ci/clang_tidy.sh @@ -14,7 +14,11 @@ rapids-dependency-file-generator \ --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)" From e17c70b56a1030813f6e2aa5f26d4340d70f6c99 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 1 Oct 2024 00:22:41 +0000 Subject: [PATCH 07/37] Configure with ninja --- ci/clang_tidy.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh index 576c0a32060..a8bb814d76c 100755 --- a/ci/clang_tidy.sh +++ b/ci/clang_tidy.sh @@ -23,7 +23,7 @@ set -u RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)" # Run the CMake configure step and set the build directory for clang-tidy. -cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON +cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -GNinja export CUDF_ROOT="${PWD}/cpp/build" # Run pre-commit checks From 44ef5acf169de67395be71d7b16a7fd18d73d1d9 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 1 Oct 2024 00:30:54 +0000 Subject: [PATCH 08/37] Only run clang-tidy --- ci/clang_tidy.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh index a8bb814d76c..403be2c58c2 100755 --- a/ci/clang_tidy.sh +++ b/ci/clang_tidy.sh @@ -27,4 +27,4 @@ cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMM export CUDF_ROOT="${PWD}/cpp/build" # Run pre-commit checks -pre-commit run --all-files --show-diff-on-failure --hook-stage manual +pre-commit run --all-files --show-diff-on-failure --hook-stage manual clang-tidy From a6008a64ed38258907af142aceb1f080ce4fd34d Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 1 Oct 2024 04:57:57 +0000 Subject: [PATCH 09/37] Up the parallelism --- cpp/scripts/run-clang-tidy.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/scripts/run-clang-tidy.sh b/cpp/scripts/run-clang-tidy.sh index de432bfeb99..b894b6dc5b5 100755 --- a/cpp/scripts/run-clang-tidy.sh +++ b/cpp/scripts/run-clang-tidy.sh @@ -37,4 +37,4 @@ if [ ! -f "${CUDF_BUILD_DIR}/compile_commands.json" ]; then fi -find cpp/{src,tests} -name *.cpp | grep -v -E ".*(cpu_unbz2.cpp|brotli_dict.cpp).*" | xargs -n 1 -P 10 clang-tidy -p ${CUDF_BUILD_DIR} --extra-arg="-Qunused-arguments" +find cpp/{src,tests} -name *.cpp | grep -v -E ".*(cpu_unbz2.cpp|brotli_dict.cpp).*" | xargs -n 1 -P 16 clang-tidy -p ${CUDF_BUILD_DIR} --extra-arg="-Qunused-arguments" From 64046a3ddff7ce064bcecbf64a1daca8deb303c8 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 1 Oct 2024 05:23:51 +0000 Subject: [PATCH 10/37] Update for CI failure --- cpp/tests/interop/nanoarrow_utils.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/tests/interop/nanoarrow_utils.hpp b/cpp/tests/interop/nanoarrow_utils.hpp index a961f73d955..8be7e087b6d 100644 --- a/cpp/tests/interop/nanoarrow_utils.hpp +++ b/cpp/tests/interop/nanoarrow_utils.hpp @@ -256,7 +256,8 @@ std::enable_if_t, nanoarrow::UniqueArray> get_nanoarrow_ ArrowBitmap out; ArrowBitmapInit(&out); NANOARROW_THROW_NOT_OK(ArrowBitmapResize(&out, b.size(), 1)); - std::memset(out.buffer.data, 0, out.buffer.size_bytes); + // TODO: Investigate clang-tidy issue further after nanoarrow is made compliant + std::memset(out.buffer.data, 0, out.buffer.size_bytes); // NOLINT for (size_t i = 0; i < b.size(); ++i) { ArrowBitSetTo(out.buffer.data, i, static_cast(b[i])); From e70a75217a9cdb2c26fa21ef026fb4cdfc295b5e Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 1 Oct 2024 05:24:11 +0000 Subject: [PATCH 11/37] Up the parallelism again to see --- cpp/scripts/run-clang-tidy.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/scripts/run-clang-tidy.sh b/cpp/scripts/run-clang-tidy.sh index b894b6dc5b5..8bbe28858eb 100755 --- a/cpp/scripts/run-clang-tidy.sh +++ b/cpp/scripts/run-clang-tidy.sh @@ -37,4 +37,4 @@ if [ ! -f "${CUDF_BUILD_DIR}/compile_commands.json" ]; then fi -find cpp/{src,tests} -name *.cpp | grep -v -E ".*(cpu_unbz2.cpp|brotli_dict.cpp).*" | xargs -n 1 -P 16 clang-tidy -p ${CUDF_BUILD_DIR} --extra-arg="-Qunused-arguments" +find cpp/{src,tests} -name *.cpp | grep -v -E ".*(cpu_unbz2.cpp|brotli_dict.cpp).*" | xargs -n 1 -P 24 clang-tidy -p ${CUDF_BUILD_DIR} --extra-arg="-Qunused-arguments" From 87547f0a60ab511d5273c352add74564f6027347 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 1 Oct 2024 14:57:30 +0000 Subject: [PATCH 12/37] Make sure nvrtc is installed --- dependencies.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/dependencies.yaml b/dependencies.yaml index 475d94adfdf..44e738a9666 100644 --- a/dependencies.yaml +++ b/dependencies.yaml @@ -91,6 +91,7 @@ files: includes: - build_all - build_base + - cuda - develop - py_version docs: From 5561faf7542592ff5cf83305c0392731a883ec80 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 1 Oct 2024 14:58:03 +0000 Subject: [PATCH 13/37] Reduce parallelism --- cpp/scripts/run-clang-tidy.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/scripts/run-clang-tidy.sh b/cpp/scripts/run-clang-tidy.sh index 8bbe28858eb..b894b6dc5b5 100755 --- a/cpp/scripts/run-clang-tidy.sh +++ b/cpp/scripts/run-clang-tidy.sh @@ -37,4 +37,4 @@ if [ ! -f "${CUDF_BUILD_DIR}/compile_commands.json" ]; then fi -find cpp/{src,tests} -name *.cpp | grep -v -E ".*(cpu_unbz2.cpp|brotli_dict.cpp).*" | xargs -n 1 -P 24 clang-tidy -p ${CUDF_BUILD_DIR} --extra-arg="-Qunused-arguments" +find cpp/{src,tests} -name *.cpp | grep -v -E ".*(cpu_unbz2.cpp|brotli_dict.cpp).*" | xargs -n 1 -P 16 clang-tidy -p ${CUDF_BUILD_DIR} --extra-arg="-Qunused-arguments" From 26aa432a4f044270c3fb8b0d4f944171151f6066 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 1 Oct 2024 14:31:02 -0700 Subject: [PATCH 14/37] Generate headers from jitify so that they're available --- ci/clang_tidy.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh index 403be2c58c2..aa8f72e622b 100755 --- a/ci/clang_tidy.sh +++ b/ci/clang_tidy.sh @@ -22,9 +22,12 @@ set -u RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)" -# Run the CMake configure step and set the build directory for clang-tidy. -cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -GNinja +# Run the CMake configure step and set the build directory for clang-tidy. We +# also have to build the jitify generated files or they won't exist for +# clang-tidy to discover. export CUDF_ROOT="${PWD}/cpp/build" +cmake -S cpp -B "${CUDF_ROOT}" -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -GNinja +cmake --build "${CUDF_ROOT}" --target jitify_preprocess_run # Run pre-commit checks pre-commit run --all-files --show-diff-on-failure --hook-stage manual clang-tidy From 00dcbf0b187b801f313eb6951c51dc53cf0dc313 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 8 Oct 2024 23:05:51 +0000 Subject: [PATCH 15/37] Enable clang-tidy checking via CMake --- cpp/.clang-tidy | 2 +- cpp/CMakeLists.txt | 32 ++++++++++++++++++++++++++++++++ cpp/tests/CMakeLists.txt | 3 +++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/cpp/.clang-tidy b/cpp/.clang-tidy index 2d4f8c0d80e..12120a5c6d1 100644 --- a/cpp/.clang-tidy +++ b/cpp/.clang-tidy @@ -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 diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index f7a5dd2f2fb..b8d6ea8f362 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -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}") @@ -144,6 +145,36 @@ 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 + ) +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. + 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() + # ################################################################################################## # * conda environment ----------------------------------------------------------------------------- rapids_cmake_support_conda_env(conda_env MODIFY_PREFIX_PATH) @@ -713,6 +744,7 @@ target_compile_options( cudf PRIVATE "$<$:${CUDF_CXX_FLAGS}>" "$<$:${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 diff --git a/cpp/tests/CMakeLists.txt b/cpp/tests/CMakeLists.txt index 4596ec65ce7..7c79c797022 100644 --- a/cpp/tests/CMakeLists.txt +++ b/cpp/tests/CMakeLists.txt @@ -76,6 +76,9 @@ function(ConfigureTest CMAKE_TEST_NAME) "GTEST_CUDF_STREAM_MODE=new_${_CUDF_TEST_STREAM_MODE}_default;LD_PRELOAD=$" ) endif() + if(CUDF_CLANG_TIDY) + enable_clang_tidy(${CMAKE_TEST_NAME}) + endif() endfunction() # ################################################################################################## From 1b2debe15b364d0bff0e97b9b5f9f18cf9b0169d Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 8 Oct 2024 23:06:26 +0000 Subject: [PATCH 16/37] Remove now obsolete script --- cpp/scripts/run-clang-tidy.py | 289 ---------------------------------- 1 file changed, 289 deletions(-) delete mode 100644 cpp/scripts/run-clang-tidy.py diff --git a/cpp/scripts/run-clang-tidy.py b/cpp/scripts/run-clang-tidy.py deleted file mode 100644 index bd442755251..00000000000 --- a/cpp/scripts/run-clang-tidy.py +++ /dev/null @@ -1,289 +0,0 @@ -# Copyright (c) 2021-2024, NVIDIA CORPORATION. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -# - -import re -import os -import subprocess -import argparse -import json -import multiprocessing as mp -import shutil - - -EXPECTED_VERSION = "19.1.0" -VERSION_REGEX = re.compile(r" LLVM version ([0-9.]+)") -GPU_ARCH_REGEX = re.compile(r"sm_(\d+)") -SPACES = re.compile(r"\s+") -SEPARATOR = "-" * 16 -VENDORED_SOURCES = ["cpu_unbz2.cpp", "brotli_dict.cpp"] -VENDORED_HEADERS = ( - ".*(Message_generated.h|Schema_generated.h|brotli_dict.hpp|unbz2.hpp|cxxopts.hpp).*" -) - - -def parse_args(): - argparser = argparse.ArgumentParser("Runs clang-tidy on a project") - argparser.add_argument("cdb", type=str, - # TODO This is a hack, needs to be fixed - default="cpp/build/cuda-11.5.0/clang-tidy/release/compile_commands.clangd.json", - help="Path to cmake-generated compilation database" - " file. It is always found inside the root of the " - "cmake build folder. So make sure that `cmake` has " - "been run once before running this script!") - argparser.add_argument("--exe", type=str, default="clang-tidy", - help="Path to clang-tidy exe") - argparser.add_argument("--ignore", type=str, default="[.]cu$|examples/kmeans/", - help="Regex used to ignore files from checking") - argparser.add_argument("--select", type=str, default=None, - help="Regex used to select files for checking") - argparser.add_argument("-j", type=int, default=-1, - help="Number of parallel jobs to launch.") - argparser.add_argument("--fix", action="store_true", - help="Apply fixes to the files. Default is False.") - args = argparser.parse_args() - if args.j <= 0: - args.j = mp.cpu_count() - args.ignore_compiled = re.compile(args.ignore) if args.ignore else None - args.select_compiled = re.compile(args.select) if args.select else None - ret = subprocess.check_output("%s --version" % args.exe, shell=True) - ret = ret.decode("utf-8") - version = VERSION_REGEX.search(ret) - if version is None: - raise Exception("Failed to figure out clang-tidy version!") - version = version.group(1) - if version != EXPECTED_VERSION: - raise Exception("clang-tidy exe must be v%s found '%s'" % \ - (EXPECTED_VERSION, version)) - if not os.path.exists(args.cdb): - raise Exception("Compilation database '%s' missing" % args.cdb) - return args - - -def get_all_commands(cdb): - with open(cdb) as fp: - return json.load(fp) - - -def get_gpu_archs(command): - archs = [] - for loc in range(len(command)): - if command[loc] != "-gencode": - continue - arch_flag = command[loc + 1] - match = GPU_ARCH_REGEX.search(arch_flag) - if match is not None: - archs.append("--cuda-gpu-arch=sm_%s" % match.group(1)) - return archs - - -def get_index(arr, item): - try: - return arr.index(item) - except: - return -1 - - -def remove_item(arr, item): - loc = get_index(arr, item) - if loc >= 0: - del arr[loc] - return loc - - -def remove_item_plus_one(arr, item): - loc = get_index(arr, item) - if loc >= 0: - del arr[loc + 1] - del arr[loc] - return loc - - -def get_clang_includes(exe): - includes = [] - for compiler_var, language in ("CC", "c"), ("CXX", "c++"): - if (compiler := os.getenv(compiler_var)) is not None: - in_includes = False - result = subprocess.run( - [f"{compiler}", "-E", "-Wp,-v", f"-x{language}", "/dev/null"], - check=True, - capture_output=True, - ) - for line in result.stderr.decode().splitlines(): - if "include" in line and "search starts here" in line: - in_includes = True - continue - if line.startswith("End of search list."): - in_includes = False - continue - if in_includes: - includes.append("-isystem") - includes.append(line.strip()) - - dir = os.getenv("CONDA_PREFIX") - if dir is None: - ret = subprocess.check_output("which %s 2>&1" % exe, shell=True) - ret = ret.decode("utf-8") - dir = os.path.dirname(os.path.dirname(ret)) - header = os.path.join(dir, "include", "ClangHeaders") - includes.append("-isystem") - includes.append(header) - - return includes - - -def get_tidy_args(cmd, exe): - command, file = cmd["command"], cmd["file"] - is_cuda = file.endswith(".cu") - command = re.split(SPACES, command) - # compiler is always clang++! - command[0] = "clang++" - # remove compilation and output targets from the original command - remove_item_plus_one(command, "-c") - remove_item_plus_one(command, "-o") - if is_cuda: - # replace nvcc's "-gencode ..." with clang's "--cuda-gpu-arch ..." - archs = get_gpu_archs(command) - command.extend(archs) - while True: - loc = remove_item_plus_one(command, "-gencode") - if loc < 0: - break - # "-x cuda" is the right usage in clang - loc = get_index(command, "-x") - if loc >= 0: - command[loc + 1] = "cuda" - remove_item_plus_one(command, "-ccbin") - remove_item(command, "--expt-extended-lambda") - remove_item(command, "--diag_suppress=unrecognized_gcc_pragma") - command.extend(get_clang_includes(exe)) - return command, is_cuda - - -def run_clang_tidy_command(tidy_cmd): - cmd = " ".join(tidy_cmd) - result = subprocess.run(cmd, check=False, shell=True, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT) - status = result.returncode == 0 - if status: - out = "" - else: - out = "CMD: " + cmd - out += result.stdout.decode("utf-8").rstrip() - return status, out - - -def run_clang_tidy(cmd, args): - command, is_cuda = get_tidy_args(cmd, args.exe) - tidy_cmd = [args.exe, - "-header-filter='.*cudf/cpp/(src|include|bench|comms).*'", - f"-exclude-header-filter='{VENDORED_HEADERS}'", - "--extra-arg='-Qunused-arguments'", - cmd["file"]] - if args.fix: - tidy_cmd.append("--fix-errors") - tidy_cmd.append("--") - tidy_cmd.extend(command) - status = True - out = "" - if is_cuda: - tidy_cmd.append("--cuda-device-only") - tidy_cmd.append(cmd["file"]) - ret, out1 = run_clang_tidy_command(tidy_cmd) - out += out1 - out += "%s" % SEPARATOR - if not ret: - status = ret - tidy_cmd[-2] = "--cuda-host-only" - ret, out1 = run_clang_tidy_command(tidy_cmd) - if not ret: - status = ret - out += out1 - else: - tidy_cmd.append(cmd["file"]) - ret, out1 = run_clang_tidy_command(tidy_cmd) - if not ret: - status = ret - out += out1 - return status, out, cmd["file"] - - -# yikes! global var :( -results = [] -def collect_result(result): - global results - results.append(result) - - -def print_result(passed, stdout, file): - status_str = "PASSED" if passed else "FAILED" - print(f"{SEPARATOR} File:{file} {status_str} {SEPARATOR}") - if stdout: - print(stdout) - print(f"{SEPARATOR} File:{file} ENDS {SEPARATOR}") - - -def print_results(): - global results - status = True - for passed, stdout, file in results: - print_result(passed, stdout, file) - if not passed: - status = False - return status - - -def run_tidy_for_all_files(args, all_files): - pool = None if args.j == 1 else mp.Pool(args.j) - # actual tidy checker - for cmd in all_files: - # skip files in the build directory - if '/_deps/' in cmd["file"] or any(f in cmd["file"] for f in VENDORED_SOURCES): - continue - if args.ignore_compiled is not None and \ - re.search(args.ignore_compiled, cmd["file"]) is not None: - continue - if args.select_compiled is not None and \ - re.search(args.select_compiled, cmd["file"]) is None: - continue - if pool is not None: - pool.apply_async(run_clang_tidy, args=(cmd, args), - callback=collect_result) - else: - passed, stdout, file = run_clang_tidy(cmd, args) - collect_result((passed, stdout, file)) - if pool is not None: - pool.close() - pool.join() - return print_results() - - -def main(): - args = parse_args() - # Attempt to making sure that we run this script from root of repo always - if not os.path.exists(".git"): - raise Exception("This needs to always be run from the root of repo") - # Check whether clang-tidy exists - # print(args) - if "exe" not in args and shutil.which("clang-tidy") is not None: - print("clang-tidy not found. Exiting...") - return - all_files = get_all_commands(args.cdb) - status = run_tidy_for_all_files(args, all_files) - if not status: - raise Exception("clang-tidy failed! Refer to the errors above.") - - -if __name__ == "__main__": - main() From 7f98c52c96fff73b7a2cd1181e3a970535f9e34d Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 8 Oct 2024 23:08:34 +0000 Subject: [PATCH 17/37] Make clang-tidy available in the conda environment --- dependencies.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/dependencies.yaml b/dependencies.yaml index 44e738a9666..45fe4ae1f2b 100644 --- a/dependencies.yaml +++ b/dependencies.yaml @@ -6,6 +6,9 @@ files: cuda: ["11.8", "12.5"] arch: [x86_64] includes: + # Note that clang-tidy is not included here because cudf's preferred + # version conflicts with the rest of RAPIDS and that would break any + # unified environment. - build_base - build_all - build_cpp @@ -91,6 +94,7 @@ files: includes: - build_all - build_base + - clang_tidy - cuda - develop - py_version @@ -564,6 +568,12 @@ dependencies: - output_types: conda packages: - &doxygen doxygen=1.9.1 # pre-commit hook needs a specific version. + clang_tidy: + common: + - output_types: conda + packages: + - clang==19.1.0 + - clang-tools==19.1.0 docs: common: - output_types: [conda] From 051ded80d6b5711744c5efb12660fe11363fb2f1 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 8 Oct 2024 23:11:02 +0000 Subject: [PATCH 18/37] Remove the pre-commit hook --- .pre-commit-config.yaml | 12 ----------- cpp/scripts/run-clang-tidy.sh | 40 ----------------------------------- 2 files changed, 52 deletions(-) delete mode 100755 cpp/scripts/run-clang-tidy.sh diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 1d5b59f880b..f861fb57916 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -125,18 +125,6 @@ repos: language: system pass_filenames: false verbose: true - - id: clang-tidy - name: clang-tidy - entry: ./cpp/scripts/run-clang-tidy.sh - language: python - types_or: [c, c++] - # Note that pre-commit autoupdate does not update the versions - # of dependencies, so we'll have to update this manually. - additional_dependencies: - - clang-tidy==19.1.0 - pass_filenames: false - stages: [manual] - verbose: true - repo: https://github.com/codespell-project/codespell rev: v2.2.6 hooks: diff --git a/cpp/scripts/run-clang-tidy.sh b/cpp/scripts/run-clang-tidy.sh deleted file mode 100755 index b894b6dc5b5..00000000000 --- a/cpp/scripts/run-clang-tidy.sh +++ /dev/null @@ -1,40 +0,0 @@ -#!/bin/bash -# Copyright (c) 2024, NVIDIA CORPORATION. - -# This script is a wrapper for clang-tidy for use with pre-commit. The -# wrapping serves a few purposes: -# 1. It allows us to find the files to run clang-tidy on rather than relying on pre-commit to pass them in. -# 2. It allows us to fail gracefully if no compile commands database is found. -# -# This script can be invoked directly anywhere within the project repository. -# Alternatively, it may be invoked as a pre-commit hook via -# `pre-commit run clang-tidy`. -# -# Usage: -# bash run-clang-tidy.sh - -status=0 -if [ -z ${CUDF_ROOT:+PLACEHOLDER} ]; then - CUDF_BUILD_DIR=$(git rev-parse --show-toplevel 2>&1)/cpp/build - status=$? -else - CUDF_BUILD_DIR=${CUDF_ROOT} -fi - -if ! [ ${status} -eq 0 ]; then - if [[ ${CUDF_BUILD_DIR} == *"not a git repository"* ]]; then - echo "This script must be run inside the cudf repository, or the CUDF_ROOT environment variable must be set." - else - echo "Script failed with unknown error attempting to determine project root:" - echo ${CUDF_BUILD_DIR} - fi - exit 1 -fi - -if [ ! -f "${CUDF_BUILD_DIR}/compile_commands.json" ]; then - echo "No compile commands database found. Please run CMake with -DCMAKE_EXPORT_COMPILE_COMMANDS=ON." - exit 1 -fi - - -find cpp/{src,tests} -name *.cpp | grep -v -E ".*(cpu_unbz2.cpp|brotli_dict.cpp).*" | xargs -n 1 -P 16 clang-tidy -p ${CUDF_BUILD_DIR} --extra-arg="-Qunused-arguments" From 74ef32c094482a85383dcb7f9ab2f50da0cd8098 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 8 Oct 2024 23:12:24 +0000 Subject: [PATCH 19/37] Update the CI script --- ci/clang_tidy.sh | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh index aa8f72e622b..fea0dc4697f 100755 --- a/ci/clang_tidy.sh +++ b/ci/clang_tidy.sh @@ -22,12 +22,6 @@ set -u RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)" -# Run the CMake configure step and set the build directory for clang-tidy. We -# also have to build the jitify generated files or they won't exist for -# clang-tidy to discover. -export CUDF_ROOT="${PWD}/cpp/build" -cmake -S cpp -B "${CUDF_ROOT}" -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -GNinja -cmake --build "${CUDF_ROOT}" --target jitify_preprocess_run - -# Run pre-commit checks -pre-commit run --all-files --show-diff-on-failure --hook-stage manual clang-tidy +# Run the build via CMake, which will run clang-tidy when CUDF_CLANG_TIDY is enabled. +cmake -S cpp -B "${CUDF_ROOT}" -DCMAKE_BUILD_TYPE=Release -DCUDF_CLANG_TIDY=ON -GNinja +cmake --build "${CUDF_ROOT}" From a8dbb83826ba0740477398bc4a25a8e1f5122db1 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 8 Oct 2024 23:12:55 +0000 Subject: [PATCH 20/37] Test commit introducing a clang-tidy error --- cpp/src/io/utilities/data_sink.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/io/utilities/data_sink.cpp b/cpp/src/io/utilities/data_sink.cpp index 0b76f3d3e8f..f4f5d3df2f3 100644 --- a/cpp/src/io/utilities/data_sink.cpp +++ b/cpp/src/io/utilities/data_sink.cpp @@ -51,7 +51,7 @@ class file_sink : public data_sink { } // Marked as NOLINT because we are calling a virtual method in the destructor - ~file_sink() override { flush(); } // NOLINT + ~file_sink() override { flush(); } void host_write(void const* data, size_t size) override { From 17f3ce9a9c1bcb529725dd381932b3a91b7a162b Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 8 Oct 2024 23:18:10 +0000 Subject: [PATCH 21/37] Redefine variable --- ci/clang_tidy.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh index fea0dc4697f..ec25d477ce4 100755 --- a/ci/clang_tidy.sh +++ b/ci/clang_tidy.sh @@ -23,5 +23,6 @@ set -u RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)" # Run the build via CMake, which will run clang-tidy when CUDF_CLANG_TIDY is enabled. +export CUDF_ROOT="${PWD}/cpp/build" cmake -S cpp -B "${CUDF_ROOT}" -DCMAKE_BUILD_TYPE=Release -DCUDF_CLANG_TIDY=ON -GNinja cmake --build "${CUDF_ROOT}" From 4b97bd1c6f7844451ea5291cf205537278ca2f57 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 8 Oct 2024 23:18:33 +0000 Subject: [PATCH 22/37] Just inline it --- ci/clang_tidy.sh | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh index ec25d477ce4..bf004cd025d 100755 --- a/ci/clang_tidy.sh +++ b/ci/clang_tidy.sh @@ -23,6 +23,5 @@ set -u RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)" # Run the build via CMake, which will run clang-tidy when CUDF_CLANG_TIDY is enabled. -export CUDF_ROOT="${PWD}/cpp/build" -cmake -S cpp -B "${CUDF_ROOT}" -DCMAKE_BUILD_TYPE=Release -DCUDF_CLANG_TIDY=ON -GNinja -cmake --build "${CUDF_ROOT}" +cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCUDF_CLANG_TIDY=ON -GNinja +cmake --build cpp/build From c25da567d14788588f2494c63bb9907602376f68 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Tue, 8 Oct 2024 23:21:59 +0000 Subject: [PATCH 23/37] Set up sccache to avoid recompilation --- ci/clang_tidy.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh index bf004cd025d..fa65c173557 100755 --- a/ci/clang_tidy.sh +++ b/ci/clang_tidy.sh @@ -22,6 +22,8 @@ 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 From c95ce448eb98903d165a1b9138ee12ad14eea5d2 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 00:39:35 +0000 Subject: [PATCH 24/37] Revert "Test commit introducing a clang-tidy error" This reverts commit e679f4bb961d33e5e64ff33304142c578e36d057. --- cpp/src/io/utilities/data_sink.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/io/utilities/data_sink.cpp b/cpp/src/io/utilities/data_sink.cpp index f4f5d3df2f3..0b76f3d3e8f 100644 --- a/cpp/src/io/utilities/data_sink.cpp +++ b/cpp/src/io/utilities/data_sink.cpp @@ -51,7 +51,7 @@ class file_sink : public data_sink { } // Marked as NOLINT because we are calling a virtual method in the destructor - ~file_sink() override { flush(); } + ~file_sink() override { flush(); } // NOLINT void host_write(void const* data, size_t size) override { From 443df306c5f2f7a5eaf6ef2188cecd062b8725f0 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 01:05:12 +0000 Subject: [PATCH 25/37] Add a version check to the CMake --- cpp/CMakeLists.txt | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index b8d6ea8f362..accb5d00d3e 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -153,6 +153,24 @@ if(CUDF_CLANG_TIDY) 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 + string(REPLACE "." ";" LLVM_VERSION_LIST ${LLVM_VERSION}) + list(SUBLIST LLVM_VERSION_LIST 0 2 LLVM_VERSION_LIST) + list(JOIN LLVM_VERSION_LIST "." LLVM_VERSION) + 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. From 6bcacc6d6e7036b26aab34c25ddfb203fa89d549 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 15:56:58 +0000 Subject: [PATCH 26/37] Make sure to run in parallel, it's unclear if clang-tidy is serializing --- ci/clang_tidy.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh index fa65c173557..93661bce5e8 100755 --- a/ci/clang_tidy.sh +++ b/ci/clang_tidy.sh @@ -26,4 +26,4 @@ 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 +cmake --build cpp/build --parallel `nproc` From 435d2b9f49d2403e6d974d822076fdd107b2e579 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 17:06:53 +0000 Subject: [PATCH 27/37] Revert "Make sure to run in parallel, it's unclear if clang-tidy is serializing" This reverts commit 6bcacc6d6e7036b26aab34c25ddfb203fa89d549. --- ci/clang_tidy.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh index 93661bce5e8..fa65c173557 100755 --- a/ci/clang_tidy.sh +++ b/ci/clang_tidy.sh @@ -26,4 +26,4 @@ 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 --parallel `nproc` +cmake --build cpp/build From 1c51256ee505ec7e32107c720a8d62aec8a644d4 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 17:07:18 +0000 Subject: [PATCH 28/37] Add note about moving the check to nightly --- .github/workflows/pr.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index e6c783638db..ab55fde1cb7 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -14,6 +14,7 @@ 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 From 4a0c50b863fc0bbc0e7ee67d1ce83bf19bdda7b0 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 17:09:17 +0000 Subject: [PATCH 29/37] Add some notes --- cpp/CMakeLists.txt | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index accb5d00d3e..84284edf90f 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -160,7 +160,10 @@ if(CUDF_CLANG_TIDY) 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 + # 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) @@ -183,7 +186,8 @@ function(enable_clang_tidy target) ) if(CUDF_CLANG_TIDY) - # clang will complain about unused link libraries on the compile line. + # 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" ) From 3ba5e83fdfd525e1ff03cbe6018fb7e2eb61e1dd Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 10:37:12 -0700 Subject: [PATCH 30/37] Apply suggestions from code review Co-authored-by: Bradley Dice --- .github/workflows/pr.yaml | 2 +- ci/clang_tidy.sh | 2 +- dependencies.yaml | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index ab55fde1cb7..64308bc4303 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -96,7 +96,7 @@ jobs: uses: rapidsai/shared-workflows/.github/workflows/checks.yaml@branch-24.12 with: enable_check_generated_files: false - clang_tidy: + clang-tidy: secrets: inherit uses: rapidsai/shared-workflows/.github/workflows/custom-job.yaml@branch-24.12 with: diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh index fa65c173557..93e5624e43b 100755 --- a/ci/clang_tidy.sh +++ b/ci/clang_tidy.sh @@ -1,5 +1,5 @@ #!/bin/bash -# Copyright (c) 2020-2024, NVIDIA CORPORATION. +# Copyright (c) 2024, NVIDIA CORPORATION. set -euo pipefail diff --git a/dependencies.yaml b/dependencies.yaml index 45fe4ae1f2b..85810f1f9bc 100644 --- a/dependencies.yaml +++ b/dependencies.yaml @@ -96,6 +96,7 @@ files: - build_base - clang_tidy - cuda + - cuda_version - develop - py_version docs: From aa33ba0bf7b7deac5b1922b08d38db1c35d656aa Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 18:21:06 +0000 Subject: [PATCH 31/37] Add clang back to develop environment --- conda/environments/all_cuda-118_arch-x86_64.yaml | 2 ++ conda/environments/all_cuda-125_arch-x86_64.yaml | 2 ++ dependencies.yaml | 2 ++ 3 files changed, 6 insertions(+) diff --git a/conda/environments/all_cuda-118_arch-x86_64.yaml b/conda/environments/all_cuda-118_arch-x86_64.yaml index af9f4e65274..bd5e6c3d569 100644 --- a/conda/environments/all_cuda-118_arch-x86_64.yaml +++ b/conda/environments/all_cuda-118_arch-x86_64.yaml @@ -13,6 +13,8 @@ dependencies: - breathe>=4.35.0 - c-compiler - cachetools +- clang-tools=16.0.6 +- clang==16.0.6 - cmake>=3.26.4,!=3.30.0 - cramjam - cubinlinker diff --git a/conda/environments/all_cuda-125_arch-x86_64.yaml b/conda/environments/all_cuda-125_arch-x86_64.yaml index ec2df5a11e8..565a3ebfa3c 100644 --- a/conda/environments/all_cuda-125_arch-x86_64.yaml +++ b/conda/environments/all_cuda-125_arch-x86_64.yaml @@ -13,6 +13,8 @@ dependencies: - breathe>=4.35.0 - c-compiler - cachetools +- clang-tools=16.0.6 +- clang==16.0.6 - cmake>=3.26.4,!=3.30.0 - cramjam - cuda-cudart-dev diff --git a/dependencies.yaml b/dependencies.yaml index 85810f1f9bc..bec0b919a15 100644 --- a/dependencies.yaml +++ b/dependencies.yaml @@ -568,6 +568,8 @@ dependencies: - identify>=2.5.20 - output_types: conda packages: + - clang==16.0.6 + - clang-tools=16.0.6 - &doxygen doxygen=1.9.1 # pre-commit hook needs a specific version. clang_tidy: common: From a0f60e7c19b076799b09de97030e3ca5173ec3d9 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 18:29:56 +0000 Subject: [PATCH 32/37] Fix pr-builder --- .github/workflows/pr.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index 64308bc4303..4119c5f3a28 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -15,7 +15,7 @@ jobs: - changed-files - checks # TODO: Before merging this PR, this check needs to be moved to nightly.yaml - - clang_tidy + - clang-tidy - conda-cpp-build - conda-cpp-checks - conda-cpp-tests From 25e1a958810b7bd00e166442361affe5c6887e75 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 18:39:29 +0000 Subject: [PATCH 33/37] Fix dependency groups to avoid conflicts --- dependencies.yaml | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/dependencies.yaml b/dependencies.yaml index bec0b919a15..8088d461ae0 100644 --- a/dependencies.yaml +++ b/dependencies.yaml @@ -7,12 +7,17 @@ files: arch: [x86_64] includes: # Note that clang-tidy is not included here because cudf's preferred - # version conflicts with the rest of RAPIDS and that would break any - # unified environment. + # version conflicts with the rest of RAPIDS as well as its own + # clang-format version. Until we update our clang-format version we will + # not be able to install both into the same environment. Moreover, using + # this version will break compatibility with other RAPIDS libraries that + # are still using 16.0.6, and as such will and that would break any + # unified environment like that used in unified devcontainers. - build_base - build_all - build_cpp - build_python_common + - clang_format - cuda - cuda_version - depends_on_cupy @@ -571,6 +576,12 @@ dependencies: - clang==16.0.6 - clang-tools=16.0.6 - &doxygen doxygen=1.9.1 # pre-commit hook needs a specific version. + clang_format: + common: + - output_types: conda + packages: + - clang==16.0.6 + - clang-tools=16.0.6 clang_tidy: common: - output_types: conda From 7536de67dcddd2dbeabefb470708d192fd06997d Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 18:45:20 +0000 Subject: [PATCH 34/37] typo --- dependencies.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/dependencies.yaml b/dependencies.yaml index 8088d461ae0..ff97b67f0ce 100644 --- a/dependencies.yaml +++ b/dependencies.yaml @@ -573,8 +573,6 @@ dependencies: - identify>=2.5.20 - output_types: conda packages: - - clang==16.0.6 - - clang-tools=16.0.6 - &doxygen doxygen=1.9.1 # pre-commit hook needs a specific version. clang_format: common: From 600b406a1a7c1fb690e198f87876b7e94ff67144 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 12:10:44 -0700 Subject: [PATCH 35/37] Apply suggestions from code review Co-authored-by: Kyle Edwards --- cpp/CMakeLists.txt | 6 ++---- cpp/tests/CMakeLists.txt | 4 +--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index 84284edf90f..c8d853b4f30 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -159,14 +159,12 @@ if(CUDF_CLANG_TIDY) OUTPUT_VARIABLE CLANG_TIDY_OUTPUT OUTPUT_STRIP_TRAILING_WHITESPACE ) - string(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+" LLVM_VERSION "${CLANG_TIDY_OUTPUT}") + string(REGEX MATCH "LLVM version ([0-9]+\\.[0-9]+)\\.[0-9]+" LLVM_VERSION_MATCH "${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) + set(LLVM_VERSION "${CMAKE_MATCH_1}") set(expected_clang_tidy_version 19.1) if(NOT expected_clang_tidy_version VERSION_EQUAL LLVM_VERSION) message( diff --git a/cpp/tests/CMakeLists.txt b/cpp/tests/CMakeLists.txt index 7c79c797022..def1a68ca23 100644 --- a/cpp/tests/CMakeLists.txt +++ b/cpp/tests/CMakeLists.txt @@ -76,9 +76,7 @@ function(ConfigureTest CMAKE_TEST_NAME) "GTEST_CUDF_STREAM_MODE=new_${_CUDF_TEST_STREAM_MODE}_default;LD_PRELOAD=$" ) endif() - if(CUDF_CLANG_TIDY) - enable_clang_tidy(${CMAKE_TEST_NAME}) - endif() + enable_clang_tidy(${CMAKE_TEST_NAME}) endfunction() # ################################################################################################## From c4691f8a5e2890b079ebf9b603f57d3ae97989dd Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 9 Oct 2024 20:33:36 +0000 Subject: [PATCH 36/37] Fix style --- cpp/CMakeLists.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index c8d853b4f30..5114d2e6acf 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -159,7 +159,9 @@ if(CUDF_CLANG_TIDY) OUTPUT_VARIABLE CLANG_TIDY_OUTPUT OUTPUT_STRIP_TRAILING_WHITESPACE ) - string(REGEX MATCH "LLVM version ([0-9]+\\.[0-9]+)\\.[0-9]+" LLVM_VERSION_MATCH "${CLANG_TIDY_OUTPUT}") + string(REGEX MATCH "LLVM version ([0-9]+\\.[0-9]+)\\.[0-9]+" LLVM_VERSION_MATCH + "${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 From 8b7a356e01fba4110dcf118c0fdd291d006654d1 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Mon, 14 Oct 2024 16:26:39 +0000 Subject: [PATCH 37/37] Move workflow to nightly and fix static configure parameters --- .github/workflows/pr.yaml | 8 -------- .github/workflows/test.yaml | 14 +++++++++++++- ci/clang_tidy.sh | 2 +- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index 4119c5f3a28..bc237cc73b0 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -14,8 +14,6 @@ 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 @@ -96,12 +94,6 @@ jobs: uses: rapidsai/shared-workflows/.github/workflows/checks.yaml@branch-24.12 with: enable_check_generated_files: false - clang-tidy: - secrets: inherit - uses: rapidsai/shared-workflows/.github/workflows/custom-job.yaml@branch-24.12 - with: - build_type: pull-request - run_script: "ci/clang_tidy.sh" conda-cpp-build: needs: checks secrets: inherit diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index a22d3c5b9cc..1275aad757c 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -47,11 +47,23 @@ jobs: secrets: inherit uses: rapidsai/shared-workflows/.github/workflows/custom-job.yaml@branch-24.12 with: - build_type: pull-request + build_type: nightly + branch: ${{ inputs.branch }} + date: ${{ inputs.date }} + sha: ${{ inputs.sha }} # Use the wheel container so we can skip conda solves and since our # primary static consumers (Spark) are not in conda anyway. container_image: "rapidsai/ci-wheel:latest" run_script: "ci/configure_cpp_static.sh" + clang-tidy: + secrets: inherit + uses: rapidsai/shared-workflows/.github/workflows/custom-job.yaml@branch-24.12 + with: + build_type: nightly + branch: ${{ inputs.branch }} + date: ${{ inputs.date }} + sha: ${{ inputs.sha }} + run_script: "ci/clang_tidy.sh" conda-python-cudf-tests: secrets: inherit uses: rapidsai/shared-workflows/.github/workflows/conda-python-tests.yaml@branch-24.12 diff --git a/ci/clang_tidy.sh b/ci/clang_tidy.sh index 93e5624e43b..4d5d3fc3136 100755 --- a/ci/clang_tidy.sh +++ b/ci/clang_tidy.sh @@ -3,7 +3,7 @@ set -euo pipefail -rapids-logger "Create checks conda environment" +rapids-logger "Create clang-tidy conda environment" . /opt/conda/etc/profile.d/conda.sh ENV_YAML_DIR="$(mktemp -d)"