Use bundled gtest

[trams]

1. Suggest to use bundled gtest in tests
2. Use dmlc bundled gtest in all CI scripts
3. Make clang-tidy to use dmlc embedded gtest

[trams]

I have some issue with ctidy (it seems I broke it) which I am struggling to reproduce

[trams]

I managed to reproduce the issue but still no clue what and why it is happening.
So I removed the move of ctidy on embedded gtest for now

[trivialfis]

@trams Could you provide a link to the failure?

[trams]

@trivialfis sure. Here is a link https://xgboost-ci.net/blue/organizations/jenkins/xgboost/detail/PR-4900/3/pipeline/

I see this
[2019-09-26T22:23:09.697Z] /workspace/tests/cpp/data/test_simple_csr_source.cu:2:10: error: 'gtest/gtest.h' file not found [clang-diagnostic-error]

[2019-09-26T22:23:09.697Z] #include <gtest/gtest.h>

I looked at commands json Cmake generated and it looked fine. It included the folder with googletests headers

[trivialfis]

Wait I'm revisiting my tidy script, sorry for the noise.

[trivialfis]

@trams The tidy is running on the source code of googletest itself... Maybe some flags got mixed in for XGBoost. Could you clear out the gtest source code from build directory?

[trams]

Could you clear out the gtest source code from build directory?

What do you exactly mean by that?
I have the issue with Travis CI checks as well as local checks. It check gtest source tree indeed but it does not fail on it (it produces only warnings). For some reason it complains on some *.cu files and only on some

I do not exactly know how bundled gtests work (only in theory). It has been implemented by @hcho3 . Could you help, @hcho3 ?

Also I suggest for now to focus on this pull request and deal with updating tidy scripts later

[hcho3]

I'll take a look

[hcho3]

@trams I think I have a fix. Can you mark the checkbox that says "Allow edits from maintainers"?

@trivialfis Here is the root cause:

• Google Test under cdb/ directory should be excluded from clang-tidy checks.
• The testxgboost target refers to the include directories for Google Test with -isystem flag. For some reason, device code uses -isystem=<foobar> whereas host code uses -isystem <foobar>. I modified tidy.py to re-format -isystem=<foobar> into -isystem <foobar>.
• Clang-tidy fails on the following piece of code with enum class parameter:

enum class Foo : int {
  kBar = 0, kFrog = 1, kCat = 2, kDog = 3
};

DECLARE_FIELD_ENUM_CLASS(Foo);

struct MyParam : dmlc::Parameter<MyParam> {
  Foo foo;
};

int main(void) {
  MyParam param;
  std::map<std::string, std::string> kwargs{ {"foo", "frog"}, {"bar", "10"} };
  param.Init(kwargs);
}

Clang-tidy raises UndefinedBinaryOperatorResult error because it thinks that param.foo is never set. However, param.foo is indeed set. So this is a false alarm, and I added // NOLINT(clang-analyzer-core.UndefinedBinaryOperatorResult) to suppress the check.

[hcho3]

Here is the patch to fix clang-tidy:

diff --git a/Jenkinsfile b/Jenkinsfile
index 6eab50fe..91d6fc35 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/tests/ci_build/clang_tidy.sh b/tests/ci_build/clang_tidy.sh
deleted file mode 100755
index 5c96a78b..00000000
--- 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 d51d0fd2..ee2fa2f5 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 @@ class ClangTidy(object):
         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 @@ class ClangTidy(object):
                         '--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 @@ class ClangTidy(object):
                 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 @@ 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()
 
     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 da5ddd67..dd3fc256 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);
 

[trams]

Sorry for a slow response.

This pull requests allows to be edited by maintainers so feel free to go ahead and modify anything
Meanwhile I'll try to resubmit move ctidy to use embedded google test

@hcho3 thank you for a detailed analysis !

[trams]

Thank you! I was a bit confused by tests not passing

[hcho3]

@trams There was outage in the test farm (#4921). It is fixed now.