From a095fbf22a1b86dc293184be91b003508edf51fd Mon Sep 17 00:00:00 2001 From: Oleksandr Pryimak Date: Thu, 26 Sep 2019 15:02:39 -0700 Subject: [PATCH 1/3] Suggest to use gtest bundled with dmlc --- CMakeLists.txt | 2 +- doc/contrib/unit_tests.rst | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d498bb217628..2ffff89f5a29 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -29,7 +29,7 @@ option(JVM_BINDINGS "Build JVM bindings" OFF) option(R_LIB "Build shared library for R package" OFF) ## Dev option(GOOGLE_TEST "Build google tests" OFF) -option(USE_DMLC_GTEST "Use google tests bundled with dmlc-core submodule (EXPERIMENTAL)" OFF) +option(USE_DMLC_GTEST "Use google tests bundled with dmlc-core submodule" OFF) option(USE_NVTX "Build with cuda profiling annotations. Developers only." OFF) set(NVTX_HEADER_DIR "" CACHE PATH "Path to the stand-alone nvtx header") option(RABIT_MOCK "Build rabit with mock" OFF) diff --git a/doc/contrib/unit_tests.rst b/doc/contrib/unit_tests.rst index 992e65371cd1..fa7ec00b0bde 100644 --- a/doc/contrib/unit_tests.rst +++ b/doc/contrib/unit_tests.rst @@ -104,14 +104,13 @@ In addition, to test CUDA code, run: C++: Google Test ================ -To build and run C++ unit tests, install `Google Test `_ library with headers -and then enable tests while running CMake: +To build and run C++ unit tests enable tests while running CMake: .. code-block:: bash mkdir build cd build - cmake -DGOOGLE_TEST=ON -DGTEST_ROOT=/path/to/google-test .. + cmake -DGOOGLE_TEST=ON -DUSE_DMLC_GTEST=ON .. make make test @@ -121,7 +120,7 @@ To enable tests for CUDA code, add ``-DUSE_CUDA=ON`` and ``-DUSE_NCCL=ON`` (CUDA mkdir build cd build - cmake -DGOOGLE_TEST=ON -DGTEST_ROOT=/path/to/google-test -DUSE_CUDA=ON -DUSE_NCCL=ON .. + cmake -DGOOGLE_TEST=ON -DUSE_DMLC_GTEST=ON -DUSE_CUDA=ON -DUSE_NCCL=ON .. make make test From 018280b707495e1fe4a8374633e662ca0afe124a Mon Sep 17 00:00:00 2001 From: Oleksandr Pryimak Date: Thu, 26 Sep 2019 15:12:59 -0700 Subject: [PATCH 2/3] Use dmlc bundled gtest in all CI scripts --- tests/ci_build/build_via_cmake.sh | 12 +----------- tests/travis/run_test.sh | 11 +---------- 2 files changed, 2 insertions(+), 21 deletions(-) diff --git a/tests/ci_build/build_via_cmake.sh b/tests/ci_build/build_via_cmake.sh index cbfc9725548d..98808141b0e8 100755 --- a/tests/ci_build/build_via_cmake.sh +++ b/tests/ci_build/build_via_cmake.sh @@ -1,20 +1,10 @@ #!/usr/bin/env bash set -e -# Build gtest via cmake -rm -rf gtest -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* - rm -rf build mkdir build cd build -cmake .. "$@" -DGOOGLE_TEST=ON -DGTEST_ROOT=$PWD/../gtest -DCMAKE_VERBOSE_MAKEFILE=ON +cmake .. "$@" -DGOOGLE_TEST=ON -DUSE_DMLC_GTEST=ON -DCMAKE_VERBOSE_MAKEFILE=ON make clean make -j$(nproc) cd .. diff --git a/tests/travis/run_test.sh b/tests/travis/run_test.sh index 3d9602fd134d..8890c15bd66a 100755 --- a/tests/travis/run_test.sh +++ b/tests/travis/run_test.sh @@ -33,20 +33,11 @@ fi if [ ${TASK} == "cmake_test" ]; then set -e - # Build gtest via cmake - 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 - CC=gcc-7 CXX=g++-7 cmake . && make - mkdir lib && mv libgtest.a lib - cd .. - rm -rf release-1.7.0.zip - # Build/test rm -rf build mkdir build && cd build PLUGINS="-DPLUGIN_LZ4=ON -DPLUGIN_DENSE_PARSER=ON" - CC=gcc-7 CXX=g++-7 cmake .. -DGOOGLE_TEST=ON -DGTEST_ROOT=$PWD/../gtest/ ${PLUGINS} + CC=gcc-7 CXX=g++-7 cmake .. -DGOOGLE_TEST=ON -DUSE_DMLC_GTEST=ON ${PLUGINS} make ./testxgboost cd .. From 91d53fa3e59be2c61f651d757e34df0ee1e44e94 Mon Sep 17 00:00:00 2001 From: Oleksandr Pryimak Date: Thu, 26 Sep 2019 15:18:43 -0700 Subject: [PATCH 3/3] Make clang-tidy to use dmlc embedded gtest --- Jenkinsfile | 2 +- doc/contrib/coding_guide.rst | 8 +++----- tests/ci_build/clang_tidy.sh | 18 ------------------ tests/ci_build/tidy.py | 15 ++++++--------- tests/cpp/common/test_enum_class_param.cc | 2 +- 5 files changed, 11 insertions(+), 34 deletions(-) delete mode 100755 tests/ci_build/clang_tidy.sh diff --git a/Jenkinsfile b/Jenkinsfile index 6eab50fe678d..91d6fc35fd03 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -116,7 +116,7 @@ def ClangTidy() { def docker_binary = "docker" def dockerArgs = "--build-arg CUDA_VERSION=9.2" sh """ - ${dockerRun} ${container_type} ${docker_binary} ${dockerArgs} tests/ci_build/clang_tidy.sh + ${dockerRun} ${container_type} ${docker_binary} ${dockerArgs} python3 tests/ci_build/tidy.py """ deleteDir() } diff --git a/doc/contrib/coding_guide.rst b/doc/contrib/coding_guide.rst index d3dc35532606..9eff456354c8 100644 --- a/doc/contrib/coding_guide.rst +++ b/doc/contrib/coding_guide.rst @@ -118,21 +118,19 @@ To run this check locally, run the following command from the top level source t .. code-block:: bash cd /path/to/xgboost/ - python3 tests/ci_build/tidy.py --gtest-path=/path/to/google-test - -where ``--gtest-path`` option specifies the full path of Google Test library. + python3 tests/ci_build/tidy.py Also, the script accepts two optional integer arguments, namely ``--cpp`` and ``--cuda``. By default they are both set to 1, meaning that both C++ and CUDA code will be checked. If the CUDA toolkit is not installed on your machine, you'll encounter an error. To exclude CUDA source from linting, use: .. code-block:: bash cd /path/to/xgboost/ - python3 tests/ci_build/tidy.py --cuda=0 --gtest-path=/path/to/google-test + python3 tests/ci_build/tidy.py --cuda=0 Similarly, if you want to exclude C++ source from linting: .. code-block:: bash cd /path/to/xgboost/ - python3 tests/ci_build/tidy.py --cpp=0 --gtest-path=/path/to/google-test + python3 tests/ci_build/tidy.py --cpp=0 diff --git a/tests/ci_build/clang_tidy.sh b/tests/ci_build/clang_tidy.sh deleted file mode 100755 index 5c96a78ba60e..000000000000 --- a/tests/ci_build/clang_tidy.sh +++ /dev/null @@ -1,18 +0,0 @@ -#!/bin/bash - -export GTEST_PKG_NAME=release-1.8.1 -export GTEST_DIR_NAME=googletest-${GTEST_PKG_NAME} # uncompressed directory -export GTEST_ZIP_FILE=${GTEST_PKG_NAME}.zip # downloaded zip ball name - -rm -rf gtest googletest-release* - -wget -nc https://github.com/google/googletest/archive/${GTEST_ZIP_FILE} -unzip -n ${GTEST_ZIP_FILE} -mv ${GTEST_DIR_NAME} gtest && cd gtest -cmake . -DCMAKE_INSTALL_PREFIX=./ins && make -make install - -cd .. -rm ${GTEST_ZIP_FILE} - -python3 tests/ci_build/tidy.py --gtest-path=${PWD}/gtest/ins diff --git a/tests/ci_build/tidy.py b/tests/ci_build/tidy.py index d51d0fd2d3ba..ee2fa2f516b7 100755 --- a/tests/ci_build/tidy.py +++ b/tests/ci_build/tidy.py @@ -29,15 +29,12 @@ 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 + def __init__(self, cpp_lint, cuda_lint): 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: @@ -61,8 +58,7 @@ def _generate_cdb(self): os.mkdir(self.cdb_path) os.chdir(self.cdb_path) cmake_args = ['cmake', '..', '-DCMAKE_EXPORT_COMPILE_COMMANDS=ON', - '-DGOOGLE_TEST=ON', '-DGTEST_ROOT={}'.format( - self.gtest_path)] + '-DGOOGLE_TEST=ON', '-DUSE_DMLC_GTEST=ON'] if self.cuda_lint: cmake_args.extend(['-DUSE_CUDA=ON', '-DUSE_NCCL=ON']) subprocess.run(cmake_args) @@ -108,6 +104,8 @@ def convert_nvcc_command_to_clang(self, command): '--cuda-gpu-arch=sm_' + capability) elif components[i].find('--std=c++11') != -1: converted_components.append('-std=c++11') + elif components[i].startswith('-isystem='): + converted_components.extend(components[i].split('=')) else: converted_components.append(components[i]) @@ -156,6 +154,7 @@ def should_lint(path): return False isxgb = path.find('rabit') == -1 isxgb = isxgb and path.find('dmlc-core') == -1 + isxgb = isxgb and (not path.startswith(self.cdb_path)) if isxgb: return True @@ -235,13 +234,11 @@ def test_tidy(): 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() test_tidy() - with ClangTidy(args.gtest_path, args.cpp, args.cuda) as linter: + with ClangTidy(args.cpp, args.cuda) as linter: passed = linter.run() if not passed: sys.exit(1) diff --git a/tests/cpp/common/test_enum_class_param.cc b/tests/cpp/common/test_enum_class_param.cc index da5ddd67c54f..d224899d61ad 100644 --- a/tests/cpp/common/test_enum_class_param.cc +++ b/tests/cpp/common/test_enum_class_param.cc @@ -31,7 +31,7 @@ TEST(EnumClassParam, Basic) { {"foo", "frog"}, {"bar", "10"} }; // try initializing - param.Init(kwargs); + param.Init(kwargs); // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) ASSERT_EQ(param.foo, Foo::kFrog); ASSERT_EQ(param.bar, 10);