Perform clang-tidy on both cpp and cuda source. (#4034)

* Basic script for using compilation database. * Add `GENERATE_COMPILATION_DATABASE' to CMake. * Rearrange CMakeLists.txt. * Add basic python clang-tidy script. * Remove modernize-use-auto. * Add clang-tidy to Jenkins * Refine logic for correct path detection In Jenkins, the project root is of form /home/ubuntu/workspace/xgboost_PR-XXXX * Run clang-tidy in CUDA 9.2 container * Use clang_tidy container
2019-02-05 16:07:43 +08:00 · 2019-02-05 16:07:43 +08:00 · 8905df4a18
commit 8905df4a18
parent 1088dff42c
7 changed files with 328 additions and 23 deletions
--- a/.clang-tidy
+++ b/.clang-tidy
@ -1,4 +1,4 @@
-Checks: 'modernize-*,-modernize-make-*,-modernize-raw-string-literal,google-*,-google-default-arguments,-clang-diagnostic-#pragma-messages,readability-identifier-naming'
+Checks: 'modernize-*,-modernize-make-*,-modernize-use-auto,-modernize-raw-string-literal,google-*,-google-default-arguments,-clang-diagnostic-#pragma-messages,readability-identifier-naming'
 CheckOptions:
  - { key: readability-identifier-naming.ClassCase,           value: CamelCase  }
  - { key: readability-identifier-naming.StructCase,          value: CamelCase  }
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@ -123,8 +123,6 @@ file(GLOB_RECURSE SOURCES
 # Only add main function for executable target
 list(REMOVE_ITEM SOURCES ${PROJECT_SOURCE_DIR}/src/cli_main.cc)

-file(GLOB_RECURSE TEST_SOURCES "tests/cpp/*.cc")
-
 file(GLOB_RECURSE CUDA_SOURCES
    src/*.cu
    src/*.cuh
@ -140,7 +138,7 @@ if(PLUGIN_DENSE_PARSER)
 endif()

 # rabit
-# TODO: Create rabit cmakelists.txt
+# TODO: Use CMakeLists.txt from rabit.
 set(RABIT_SOURCES
    rabit/src/allreduce_base.cc
    rabit/src/allreduce_robust.cc
@ -151,6 +149,7 @@ set(RABIT_EMPTY_SOURCES
    rabit/src/engine_empty.cc
    rabit/src/c_api.cc
 )
+
 if(MINGW OR R_LIB)
  # build a dummy rabit library
  add_library(rabit STATIC ${RABIT_EMPTY_SOURCES})
@ -158,7 +157,11 @@ else()
  add_library(rabit STATIC ${RABIT_SOURCES})
 endif()

-if(USE_CUDA)
+if (GENERATE_COMPILATION_DATABASE)
+  set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
+endif (GENERATE_COMPILATION_DATABASE)
+
+if(USE_CUDA AND (NOT GENERATE_COMPILATION_DATABASE))
  find_package(CUDA 8.0 REQUIRED)
  cmake_minimum_required(VERSION 3.5)

@ -168,7 +171,7 @@ if(USE_CUDA)

  if(USE_NCCL)
    find_package(Nccl REQUIRED)
-    include_directories(${NCCL_INCLUDE_DIR})
+    cuda_include_directories(${NCCL_INCLUDE_DIR})
    add_definitions(-DXGBOOST_USE_NCCL)
  endif()

@ -188,6 +191,39 @@ if(USE_CUDA)
    target_link_libraries(gpuxgboost ${NCCL_LIB_NAME})
  endif()
  list(APPEND LINK_LIBRARIES gpuxgboost)
+
+elseif (USE_CUDA AND GENERATE_COMPILATION_DATABASE)
+  # Enable CUDA language to generate a compilation database.
+  cmake_minimum_required(VERSION 3.8)
+
+  find_package(CUDA 8.0 REQUIRED)
+  enable_language(CUDA)
+  set(CMAKE_CUDA_COMPILER clang++)
+  set(CUDA_SEPARABLE_COMPILATION ON)
+  if (NOT CLANG_CUDA_GENCODE)
+    set(CLANG_CUDA_GENCODE "--cuda-gpu-arch=sm_35")
+  endif (NOT CLANG_CUDA_GENCODE)
+  set(CMAKE_CUDA_FLAGS " -Wno-deprecated  ${CLANG_CUDA_GENCODE} -fPIC ${GENCODE} -std=c++11 -x cuda")
+  message(STATUS "CMAKE_CUDA_FLAGS: ${CMAKE_CUDA_FLAGS}")
+
+  add_library(gpuxgboost STATIC ${CUDA_SOURCES})
+
+  if(USE_NCCL)
+    find_package(Nccl REQUIRED)
+    target_include_directories(gpuxgboost PUBLIC ${NCCL_INCLUDE_DIR})
+    target_compile_definitions(gpuxgboost PUBLIC -DXGBOOST_USE_NCCL)
+    target_link_libraries(gpuxgboost PUBLIC ${NCCL_LIB_NAME})
+  endif()
+
+  target_compile_definitions(gpuxgboost PUBLIC -DXGBOOST_USE_CUDA)
+  # A hack for CMake to make arguments valid for clang++
+  string(REPLACE "-x cu" "-x cuda" CMAKE_CUDA_COMPILE_PTX_COMPILATION
+    ${CMAKE_CUDA_COMPILE_PTX_COMPILATION})
+  string(REPLACE "-x cu" "-x cuda" CMAKE_CUDA_COMPILE_WHOLE_COMPILATION
+    ${CMAKE_CUDA_COMPILE_WHOLE_COMPILATION})
+  string(REPLACE "-x cu" "-x cuda" CMAKE_CUDA_COMPILE_SEPARABLE_COMPILATION
+    ${CMAKE_CUDA_COMPILE_SEPARABLE_COMPILATION})
+  target_include_directories(gpuxgboost PUBLIC cub)
 endif()


@ -203,7 +239,6 @@ endif()

 add_library(objxgboost OBJECT ${SOURCES})

-
 # building shared library for R package
 if(R_LIB)
  find_package(LibR REQUIRED)
@ -211,13 +246,13 @@ if(R_LIB)
  list(APPEND LINK_LIBRARIES "${LIBR_CORE_LIBRARY}")
  MESSAGE(STATUS "LIBR_CORE_LIBRARY " ${LIBR_CORE_LIBRARY})

-  include_directories(
+  # Shared library target for the R package
+  add_library(xgboost SHARED $<TARGET_OBJECTS:objxgboost>)
+  include_directories(xgboost
    "${LIBR_INCLUDE_DIRS}"
    "${PROJECT_SOURCE_DIR}"
  )

-  # Shared library target for the R package
-  add_library(xgboost SHARED $<TARGET_OBJECTS:objxgboost>)
  target_link_libraries(xgboost ${LINK_LIBRARIES})
  # R uses no lib prefix in shared library names of its packages
  set_target_properties(xgboost PROPERTIES PREFIX "")
@ -229,7 +264,7 @@ if(R_LIB)
  # use a dummy location for any other remaining installs
  set(CMAKE_INSTALL_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/dummy_inst")

-# main targets: shared library & exe
+  # main targets: shared library & exe
 else()
  # Executable
  add_executable(runxgboost $<TARGET_OBJECTS:objxgboost> src/cli_main.cc)
@ -252,20 +287,20 @@ else()
  add_dependencies(xgboost runxgboost)
 endif()

-
 # JVM
 if(JVM_BINDINGS)
    find_package(JNI QUIET REQUIRED)

-    include_directories(${JNI_INCLUDE_DIRS} jvm-packages/xgboost4j/src/native)
-
    add_library(xgboost4j SHARED
-        $<TARGET_OBJECTS:objxgboost>
-        jvm-packages/xgboost4j/src/native/xgboost4j.cpp)
-    set_output_directory(xgboost4j ${PROJECT_SOURCE_DIR}/lib)
+      $<TARGET_OBJECTS:objxgboost>
+      jvm-packages/xgboost4j/src/native/xgboost4j.cpp)
+    target_include_directories(xgboost4j
+      PRIVATE ${JNI_INCLUDE_DIRS}
+      PRIVATE jvm-packages/xgboost4j/src/native)
    target_link_libraries(xgboost4j
-        ${LINK_LIBRARIES}
-        ${JAVA_JVM_LIBRARY})
+      ${LINK_LIBRARIES}
+      ${JAVA_JVM_LIBRARY})
+    set_output_directory(xgboost4j ${PROJECT_SOURCE_DIR}/lib)
 endif()


@ -274,18 +309,31 @@ if(GOOGLE_TEST)
  enable_testing()
  find_package(GTest REQUIRED)

+  file(GLOB_RECURSE TEST_SOURCES "tests/cpp/*.cc")
  auto_source_group("${TEST_SOURCES}")
-  include_directories(${GTEST_INCLUDE_DIRS})

-  if(USE_CUDA)
+  if(USE_CUDA AND (NOT GENERATE_COMPILATION_DATABASE))
    file(GLOB_RECURSE CUDA_TEST_SOURCES "tests/cpp/*.cu")
+    cuda_include_directories(${GTEST_INCLUDE_DIRS})
    cuda_compile(CUDA_TEST_OBJS ${CUDA_TEST_SOURCES})
+  elseif (USE_CUDA AND GENERATE_COMPILATION_DATABASE)
+    file(GLOB_RECURSE CUDA_TEST_SOURCES "tests/cpp/*.cu")
  else()
    set(CUDA_TEST_OBJS "")
  endif()

-  add_executable(testxgboost ${TEST_SOURCES} ${CUDA_TEST_OBJS} $<TARGET_OBJECTS:objxgboost>)
+  if (USE_CUDA AND GENERATE_COMPILATION_DATABASE)
+    add_executable(testxgboost ${TEST_SOURCES} ${CUDA_TEST_SOURCES}
+      $<TARGET_OBJECTS:objxgboost>)
+    target_include_directories(testxgboost PRIVATE cub)
+  else ()
+    add_executable(testxgboost ${TEST_SOURCES} ${CUDA_TEST_OBJS}
+      $<TARGET_OBJECTS:objxgboost>)
+  endif ()
+
  set_output_directory(testxgboost ${PROJECT_SOURCE_DIR})
+  target_include_directories(testxgboost
+    PRIVATE ${GTEST_INCLUDE_DIRS})
  target_link_libraries(testxgboost ${GTEST_LIBRARIES} ${LINK_LIBRARIES})

  add_test(TestXGBoost testxgboost)
--- a/21
+++ b/21
@ -53,7 +53,7 @@ pipeline {
                    parallel (buildMatrix.findAll{it['enabled']}.collectEntries{ c ->
                        def buildName = utils.getBuildName(c)
                        utils.buildFactory(buildName, c, false, this.&buildPlatformCmake)
-                    })
+                    } + [ "clang-tidy" : { buildClangTidyJob() } ])
                }
            }
        }
@ -106,3 +106,22 @@ def buildPlatformCmake(buildName, conf, nodeReq, dockerTarget) {
        }
    }
 }
+
+/**
+ * Run a clang-tidy job on a GPU machine
+ */
+def buildClangTidyJob() {
+    def nodeReq = "linux && gpu && unrestricted"
+    node(nodeReq) {
+        unstash name: 'srcs'
+        echo "Running clang-tidy job..."
+        // Invoke command inside docker
+        // Install Google Test and Python yaml
+        dockerTarget = "clang_tidy"
+        dockerArgs = "--build-arg CUDA_VERSION=9.2"
+        sh """
+        ${dockerRun} ${dockerTarget} ${dockerArgs} tests/ci_build/clang_tidy.sh
+        """
+      }
+  }
+
--- a/doc/contribute.rst
+++ b/doc/contribute.rst
@ -19,6 +19,7 @@ Everyone is more than welcome to contribute. It is a way to make the project bet
 * `Documents`_
 * `Testcases`_
 * `Sanitizers`_
+* `clang-tidy`_
 * `Examples`_
 * `Core Library`_
 * `Python Package`_
@ -169,6 +170,31 @@ environment variable:

 For details, please consult `official documentation <https://github.com/google/sanitizers/wiki>`_ for sanitizers.

+**********
+clang-tidy
+**********
+To run clang-tidy on both C++ and CUDA source code,  run the following command
+from the top level source tree:
+
+  .. code-black:: bash
+    cd /path/to/xgboost/
+    python3 tests/ci_build/tidy.py --gtest-path=/path/to/google-test
+
+The script requires the full path of Google Test library via the ``--gtest-path`` argument.
+
+Also, the script accepts two optional integer arguments, namely ``--cpp`` and ``--cuda``.
+By default they are both set to 1.  If you want to exclude CUDA source from
+linting, use:
+
+  .. code-black:: bash
+    cd /path/to/xgboost/
+    python3 tests/ci_build/tidy.py --cuda=0
+
+Similarly, if you want to exclude C++ source from linting:
+
+  .. code-black:: bash
+    cd /path/to/xgboost/
+    python3 tests/ci_build/tidy.py --cpp=0

 ********
 Examples
--- a/tests/ci_build/Dockerfile.clang_tidy
+++ b/tests/ci_build/Dockerfile.clang_tidy
@ -0,0 +1,45 @@
+ARG CUDA_VERSION
+FROM nvidia/cuda:$CUDA_VERSION-devel-ubuntu18.04
+
+# Environment
+ENV DEBIAN_FRONTEND noninteractive
+
+# Install all basic requirements
+RUN \
+    apt-get update && \
+    apt-get install -y tar unzip wget git build-essential cmake python3 python3-pip llvm-7 clang-tidy-7 clang-7
+
+# Set default clang-tidy version
+RUN \
+    update-alternatives --install /usr/bin/clang-tidy clang-tidy /usr/bin/clang-tidy-7 100 && \
+    update-alternatives --install /usr/bin/clang clang /usr/bin/clang-7 100
+
+# NCCL2 (License: https://docs.nvidia.com/deeplearning/sdk/nccl-sla/index.html)
+RUN \
+    export CUDA_SHORT=`echo $CUDA_VERSION | egrep -o '[0-9]+\.[0-9]'` && \
+    if [ "${CUDA_SHORT}" != "10.0" ]; then \
+    wget https://developer.download.nvidia.com/compute/redist/nccl/v2.2/nccl_2.2.13-1%2Bcuda${CUDA_SHORT}_x86_64.txz && \
+    tar xf "nccl_2.2.13-1+cuda${CUDA_SHORT}_x86_64.txz" && \
+    cp nccl_2.2.13-1+cuda${CUDA_SHORT}_x86_64/include/nccl.h /usr/include && \
+    cp nccl_2.2.13-1+cuda${CUDA_SHORT}_x86_64/lib/* /usr/lib && \
+    rm -f nccl_2.2.13-1+cuda${CUDA_SHORT}_x86_64.txz && \
+    rm -r nccl_2.2.13-1+cuda${CUDA_SHORT}_x86_64; fi
+
+# Install Python packages
+RUN \
+    pip3 install pyyaml
+
+ENV GOSU_VERSION 1.10
+
+# Install lightweight sudo (not bound to TTY)
+RUN set -ex; \
+    wget -O /usr/local/bin/gosu "https://github.com/tianon/gosu/releases/download/$GOSU_VERSION/gosu-amd64" && \
+    chmod +x /usr/local/bin/gosu && \
+    gosu nobody true
+
+# Default entry-point to use if running locally
+# It will preserve attributes of created files
+COPY entrypoint.sh /scripts/
+
+WORKDIR /workspace
+ENTRYPOINT ["/scripts/entrypoint.sh"]
--- a/tests/ci_build/clang_tidy.sh
+++ b/tests/ci_build/clang_tidy.sh
@ -0,0 +1,12 @@
+#!/bin/bash
+
+rm -rf gtest googletest-release-1.7.0
+wget -nc https://github.com/google/googletest/archive/release-1.7.0.zip
+unzip -n release-1.7.0.zip
+mv googletest-release-1.7.0 gtest && cd gtest
+cmake . && make
+mkdir lib && mv libgtest.a lib
+cd ..
+rm -rf release-1.7.0.zip*
+
+python3 tests/ci_build/tidy.py --gtest-path=${PWD}/gtest
--- a/tests/ci_build/tidy.py
+++ b/tests/ci_build/tidy.py
@ -0,0 +1,155 @@
+#!/usr/bin/env python
+import subprocess
+import yaml
+import json
+from multiprocessing import Pool, cpu_count
+import shutil
+import os
+import re
+import argparse
+
+
+def call(args):
+    '''Subprocess run wrapper.'''
+    completed = subprocess.run(args, stdout=subprocess.PIPE,
+                               stderr=subprocess.DEVNULL)
+    error_msg = completed.stdout.decode('utf-8')
+    matched = re.match('.*xgboost.*warning.*', error_msg,
+                       re.MULTILINE | re.DOTALL)
+    if matched is None:
+        return_code = 0
+    else:
+        print(error_msg, '\n')
+        return_code = 1
+    return completed.returncode | return_code
+
+
+class ClangTidy(object):
+    '''
+    clang tidy wrapper.
+    Args:
+      gtest_path: Full path of Google Test library.
+      cpp_lint: Run linter on C++ source code.
+      cuda_lint: Run linter on CUDA source code.
+    '''
+    def __init__(self, gtest_path, cpp_lint, cuda_lint):
+        self.gtest_path = gtest_path
+        self.cpp_lint = cpp_lint
+        self.cuda_lint = cuda_lint
+        print('Using Google Test from {}'.format(self.gtest_path))
+        print('Run linter on CUDA: ', self.cuda_lint)
+        print('Run linter on C++:', self.cpp_lint)
+        if not self.cpp_lint and not self.cuda_lint:
+            raise ValueError('Both --cpp and --cuda are set to 0.')
+        self.root_path = os.path.abspath(os.path.curdir)
+        print('Project root:', self.root_path)
+        self.cdb_path = os.path.join(self.root_path, 'cdb')
+
+    def __enter__(self):
+        if os.path.exists(self.cdb_path):
+            shutil.rmtree(self.cdb_path)
+        self._generate_cdb()
+        return self
+
+    def __exit__(self, *args):
+        if os.path.exists(self.cdb_path):
+            shutil.rmtree(self.cdb_path)
+
+    def _generate_cdb(self):
+        '''Run CMake to generate compilation database.'''
+        os.mkdir(self.cdb_path)
+        os.chdir(self.cdb_path)
+        cmake_args = ['cmake', '..', '-DGENERATE_COMPILATION_DATABASE=ON',
+                      '-DGOOGLE_TEST=ON', '-DGTEST_ROOT={}'.format(
+                          self.gtest_path)]
+        if self.cuda_lint:
+            cmake_args.extend(['-DUSE_CUDA=ON', '-DUSE_NCCL=ON'])
+        subprocess.run(cmake_args)
+        os.chdir(self.root_path)
+
+    def _configure_flags(self, path, command):
+        common_args = ['clang-tidy',
+                       # "-header-filter='(xgboost\\/src|xgboost\\/include)'",
+                       '-config='+str(self.clang_tidy)]
+        common_args.append(path)
+        common_args.append('--')
+
+        command = command.split()[1:]  # remove clang/c++/g++
+        # filter out not used flags
+        if '-fuse-ld=gold' in command:
+            command.remove('-fuse-ld=gold')
+        if '-rdynamic' in command:
+            command.remove('-rdynamic')
+        if '-Xcompiler=-fPIC' in command:
+            command.remove('-Xcompiler=-fPIC')
+        if '-Xcompiler=-fPIE' in command:
+            command.remove('-Xcompiler=-fPIE')
+        if '-c' in command:
+            index = command.index('-c')
+            del command[index+1]
+            command.remove('-c')
+        if '-o' in command:
+            index = command.index('-o')
+            del command[index+1]
+            command.remove('-o')
+
+        common_args.extend(command)
+
+        # Two passes, one for device code another for host code.
+        if path.endswith('cu'):
+            args = [common_args.copy(), common_args.copy()]
+            args[0].append('--cuda-host-only')
+            args[1].append('--cuda-device-only')
+        else:
+            args = [common_args.copy()]
+        for a in args:
+            a.append('-Wno-unused-command-line-argument')
+        return args
+
+    def _configure(self):
+        '''Load and configure compile_commands and clang_tidy.'''
+
+        def should_lint(path):
+            if not self.cpp_lint and path.endswith('.cc'):
+                return False
+            return True
+
+        cdb_file = os.path.join(self.cdb_path, 'compile_commands.json')
+        with open(cdb_file, 'r') as fd:
+            self.compile_commands = json.load(fd)
+        tidy_file = os.path.join(self.root_path, '.clang-tidy')
+        with open(tidy_file) as fd:
+            self.clang_tidy = yaml.load(fd)
+        all_files = []
+        for entry in self.compile_commands:
+            path = entry['file']
+            if should_lint(path):
+                print(path)
+                args = self._configure_flags(path, entry['command'])
+                all_files.extend(args)
+        return all_files
+
+    def run(self):
+        '''Run clang-tidy.'''
+        all_files = self._configure()
+        with Pool(cpu_count()) as pool:
+            results = pool.map(call, all_files)
+        passed = True
+        if 1 in results:
+            print('Please correct clang-tidy warnings.')
+            passed = False
+        return passed
+
+
+if __name__ == '__main__':
+    parser = argparse.ArgumentParser(description='Run clang-tidy.')
+    parser.add_argument('--cpp', type=int, default=1)
+    parser.add_argument('--cuda', type=int, default=1)
+    parser.add_argument('--gtest-path', required=True,
+                        help='Full path of Google Test library directory')
+    args = parser.parse_args()
+    with ClangTidy(args.gtest_path, args.cpp, args.cuda) as linter:
+        passed = linter.run()
+    # Uncomment it once the code base is clang-tidy conformant.
+    # if not passed:
+    #     sys.exit(1)