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

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

Merged
merged 10 commits into from
Feb 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .clang-tidy
Original file line number Diff line number Diff line change
@@ -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 }
Expand Down
90 changes: 69 additions & 21 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -151,14 +149,19 @@ 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})
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)

Expand All @@ -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()

Expand All @@ -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()


Expand All @@ -203,21 +239,20 @@ endif()

add_library(objxgboost OBJECT ${SOURCES})


# building shared library for R package
if(R_LIB)
find_package(LibR REQUIRED)

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 "")
Expand All @@ -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)
Expand All @@ -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
hcho3 marked this conversation as resolved.
Show resolved Hide resolved
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()


Expand All @@ -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)
Expand Down
21 changes: 20 additions & 1 deletion Jenkinsfile
Original file line number Diff line number Diff line change
Expand Up @@ -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() } ])
}
}
}
Expand Down Expand Up @@ -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
"""
}
}

26 changes: 26 additions & 0 deletions doc/contribute.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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`_
Expand Down Expand Up @@ -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
Expand Down
45 changes: 45 additions & 0 deletions tests/ci_build/Dockerfile.clang_tidy
Original file line number Diff line number Diff line change
@@ -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"]
12 changes: 12 additions & 0 deletions tests/ci_build/clang_tidy.sh
Original file line number Diff line number Diff line change
@@ -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
Loading