Add IWYU to CI

.github/workflows/pr.yaml

@@ -14,6 +14,9 @@ jobs:
     needs:
       - changed-files
       - checks
+      # TODO: Remove this check before merging this PR and make sure the copy
+      # in test.yaml is up-to-date.
+      - clang-tidy
       - conda-cpp-build
       - conda-cpp-checks
       - conda-cpp-tests
@@ -94,6 +97,13 @@ 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/cpp_linters.sh"
+      file_to_upload: iwyu_results.txt
   conda-cpp-build:
     needs: checks
     secrets: inherit

.github/workflows/test.yaml

@@ -63,7 +63,8 @@ jobs:
       branch: ${{ inputs.branch }}
       date: ${{ inputs.date }}
       sha: ${{ inputs.sha }}
-      run_script: "ci/clang_tidy.sh"
+      run_script: "ci/cpp_linters.sh"
+      file_to_upload: iwyu_results.txt
   conda-python-cudf-tests:
     secrets: inherit
     uses: rapidsai/shared-workflows/.github/workflows/[email protected]

ci/cpp_linters.sh

@@ -0,0 +1,47 @@
+#!/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
+
+# TODO: For testing purposes, clone and build IWYU. We can switch to a release
+# once a clang 19-compatible version is available, which should be soon
+# (https://github.com/include-what-you-use/include-what-you-use/issues/1641).
+git clone https://github.com/include-what-you-use/include-what-you-use.git
+pushd include-what-you-use
+# IWYU's CMake build uses some Python scripts that assume that the cwd is
+# importable, so support that legacy behavior.
+export PYTHONPATH=${PWD}:${PYTHONPATH:-}
+cmake -S . -B build -GNinja --install-prefix=${CONDA_PREFIX}
+cmake --build build
+cmake --install build
+popd
+
+# Run the build via CMake, which will run clang-tidy when CUDF_STATIC_LINTERS is enabled.
+cmake -S cpp -B cpp/build -DCMAKE_BUILD_TYPE=Release -DCUDF_STATIC_LINTERS=ON -GNinja
+cmake --build cpp/build 2>&1 | python cpp/scripts/parse_iwyu_output.py
+
+# Remove invalid components of the path for local usage. The path below is
+# valid in the CI due to where the project is cloned, but presumably the fixes
+# will be applied locally from inside a clone of cudf.
+sed -i 's/\/__w\/cudf\/cudf\///' iwyu_results.txt

cpp/.clang-tidy

@@ -39,8 +39,8 @@ Checks:
        -clang-analyzer-optin.core.EnumCastOutOfRange,
        -clang-analyzer-optin.cplusplus.UninitializedObject'
 
-WarningsAsErrors: '*'
-HeaderFilterRegex: '.*cudf/cpp/(src|include|tests).*'
+WarningsAsErrors: ''
+HeaderFilterRegex: '.*cudf/cpp/(src|include).*'
 ExcludeHeaderFilterRegex: '.*(Message_generated.h|Schema_generated.h|brotli_dict.hpp|unbz2.hpp|cxxopts.hpp).*'
 FormatStyle:     none
 CheckOptions:

cpp/CMakeLists.txt

@@ -88,7 +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)
+option(CUDF_STATIC_LINTERS "Enable static linters during compilation" OFF)
 
 message(VERBOSE "CUDF: Build with NVTX support: ${USE_NVTX}")
 message(VERBOSE "CUDF: Configure CMake to build tests: ${BUILD_TESTS}")
@@ -146,8 +146,10 @@ if(NOT CUDF_GENERATED_INCLUDE_DIR)
 endif()
 
 # ##################################################################################################
-# * clang-tidy configuration ----------------------------------------------------------------------
-if(CUDF_CLANG_TIDY)
+# * linter configuration ---------------------------------------------------------------------------
+if(CUDF_STATIC_LINTERS)
+  # For simplicity, for now we assume that all linters can be installed into an environment where
+  # any linter is being run. We could relax this requirement if desired.
   find_program(
     CLANG_TIDY_EXE
     NAMES "clang-tidy"
@@ -174,24 +176,48 @@ if(CUDF_CLANG_TIDY)
         "clang-tidy version ${expected_clang_tidy_version} is required, but found ${LLVM_VERSION}"
     )
   endif()
+
+  find_program(IWYU_EXE NAMES include-what-you-use iwyu 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)
+function(enable_static_checkers target)
+  set(_tidy_options IWYU CLANG_TIDY)
   set(_tidy_one_value)
   set(_tidy_multi_value SKIPPED_FILES)
   cmake_parse_arguments(
-    _TIDY "${_tidy_options}" "${_tidy_one_value}" "${_tidy_multi_value}" ${ARGN}
+    _LINT "${_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)
+  if(CUDF_STATIC_LINTERS)
+    if(_LINT_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"
+      )
+    endif()
+    if(_LINT_IWYU)
+      # A few extra warnings pop up when building with IWYU. I'm not sure why, but they are not
+      # relevant since they don't show up in any other build so it's better to suppress them until
+      # we can figure out the cause. Setting this as part of CXX_INCLUDE_WHAT_YOU_USE does not
+      # appear to be sufficient, we must also ensure that it is set to the underlying target's CXX
+      # compile flags. To do this completely cleanly we should modify the flags on the target rather
+      # than the global CUDF_CXX_FLAGS, but this solution is good enough for now since we never run
+      # the linters on real builds.
+      foreach(_flag -Wno-missing-braces -Wno-absolute-value -Wunneeded-internal-declaration)
+        list(FIND CUDF_CXX_FLAGS "${flag}" _flag_index)
+        if(_flag_index EQUAL -1)
+          list(APPEND CUDF_CXX_FLAGS ${flag})
+        endif()
+      endforeach()
+      set(CUDF_CXX_FLAGS
+          "${CUDF_CXX_FLAGS}"
+          PARENT_SCOPE
+      )
+      set_target_properties(${target} PROPERTIES CXX_INCLUDE_WHAT_YOU_USE "${IWYU_EXE}")
+    endif()
+    foreach(file IN LISTS _LINT_SKIPPED_FILES)
       set_source_files_properties(${file} PROPERTIES SKIP_LINTING ON)
     endforeach()
   endif()
@@ -771,11 +797,15 @@ set_target_properties(
              INTERFACE_POSITION_INDEPENDENT_CODE ON
 )
 
+# Note: This must come before the target_compile_options below so that the function can modify the
+# flags if necessary.
+enable_static_checkers(
+  cudf SKIPPED_FILES src/io/comp/cpu_unbz2.cpp src/io/comp/brotli_dict.cpp CLANG_TIDY IWYU
+)
 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

cpp/scripts/parse_iwyu_output.py

@@ -0,0 +1,168 @@
+# Copyright (c) 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.
+#
+
+"""Helper script to modify IWYU output to only include removals.
+
+Lines that are not from include-what-you-use are removed from the output.
+"""
+
+import argparse
+import re
+from enum import Enum
+
+
+class Mode(Enum):
+    NORMAL = 0
+    ADD = 1
+    REMOVE = 2
+    FULL_INCLUDE_LIST = 3
+
+
+def extract_include_file(include_line):
+    """Extract the core file path from an #include directive."""
+    match = re.search(r'#include\s+[<"]([^">]+)[">]', include_line)
+    if match:
+        return match.group(1)
+    return None
+
+
+def parse_output(input_stream):
+    include_modifications = {}
+    current_file = None
+    mode = Mode.NORMAL
+
+    for line in input_stream:
+        if match := re.match(r"(\/\S+) should add these lines:", line):
+            current_file = match.group(1)
+            include_modifications.setdefault(
+                current_file,
+                {
+                    "add_includes": [],
+                    "remove_includes": [],
+                    "full_include_list": [],
+                },
+            )
+            mode = Mode.ADD
+        elif match := re.match(r"(\/\S+) should remove these lines:", line):
+            mode = Mode.REMOVE
+        elif match := re.match(r"The full include-list for (\/\S+):", line):
+            mode = Mode.FULL_INCLUDE_LIST
+        elif line.strip() == "---":
+            current_file = None
+            mode = Mode.NORMAL
+        else:
+            if current_file:
+                if mode == Mode.ADD:
+                    include_modifications[current_file]["add_includes"].append(
+                        line.strip()
+                    )
+                elif mode == Mode.REMOVE:
+                    include_modifications[current_file][
+                        "remove_includes"
+                    ].append(line.strip())
+                elif mode == Mode.FULL_INCLUDE_LIST:
+                    include_modifications[current_file][
+                        "full_include_list"
+                    ].append(line.strip())
+            else:
+                if (
+                    line.strip()
+                    and "include-what-you-use reported diagnostics" not in line
+                    and "In file included from" not in line
+                    and "has correct #includes/fwd-decls" not in line
+                ):
+                    print(line, end="")
+
+    return include_modifications
+
+
+def post_process_includes(include_modifications):
+    """Deduplicate and remove redundant entries from add and remove includes."""
+    for mods in include_modifications.values():
+        # Deduplicate add_includes and remove_includes
+        mods["add_includes"] = list(set(mods["add_includes"]))
+        mods["remove_includes"] = list(set(mods["remove_includes"]))
+
+        # Extract file paths from add_includes and remove_includes
+        add_files = {
+            extract_include_file(line) for line in mods["add_includes"]
+        }
+        remove_files = {
+            extract_include_file(line) for line in mods["remove_includes"]
+        }
+
+        # Remove entries that exist in both add_includes and remove_includes
+        common_files = add_files & remove_files
+        mods["add_includes"] = [
+            line
+            for line in mods["add_includes"]
+            if extract_include_file(line) not in common_files
+        ]
+        mods["remove_includes"] = [
+            line
+            for line in mods["remove_includes"]
+            if extract_include_file(line) not in common_files
+        ]
+
+        # Remove entries that exist in add_includes from full_include_list
+        mods["full_include_list"] = [
+            include
+            for include in mods["full_include_list"]
+            if extract_include_file(include) not in add_files
+        ]
+
+
+def write_output(include_modifications, output_stream):
+    for filename, mods in include_modifications.items():
+        if mods["remove_includes"]:
+            output_stream.write(f"{filename} should add these lines:\n\n")
+
+            output_stream.write(f"{filename} should remove these lines:\n")
+            for line in mods["remove_includes"]:
+                output_stream.write(line + "\n")
+            output_stream.write("\n")
+
+            output_stream.write(f"The full include-list for {filename}:\n")
+            for line in mods["full_include_list"]:
+                output_stream.write(line + "\n")
+            output_stream.write("---\n")
+
+
+def main():
+    parser = argparse.ArgumentParser(
+        description="Process include modifications from a build output log."
+    )
+    parser.add_argument(
+        "input",
+        nargs="?",
+        type=argparse.FileType("r"),
+        default="-",
+        help="Input file to read (default: stdin)",
+    )
+    parser.add_argument(
+        "--output",
+        type=argparse.FileType("w"),
+        default="iwyu_results.txt",
+        help="Output file to write (default: iwyu_output.txt)",
+    )
+    args = parser.parse_args()
+
+    include_modifications = parse_output(args.input)
+    post_process_includes(include_modifications)
+    write_output(include_modifications, args.output)
+
+
+if __name__ == "__main__":
+    main()

cpp/tests/CMakeLists.txt

@@ -83,7 +83,6 @@ 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()
-  enable_clang_tidy(${CMAKE_TEST_NAME})
 endfunction()
 
 # ##################################################################################################