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

Remove redundant FindGTest.cmake, Fix gtest include dirs. #3533

Merged
merged 1 commit into from
Aug 6, 2018

Conversation

trivialfis
Copy link
Member

Continue #3469

Gtest is included in CMake before CMake 3.2. We can safely remove it now.

@hcho3
Copy link
Collaborator

hcho3 commented Jul 30, 2018

@trivialfis The CMake test is failing in Travis. You can reproduce this by running

TASK=cmake_test tests/travis/run_test.sh

@trivialfis
Copy link
Member Author

@hcho3 Thanks, I'm trying to install GTest in travis. Now that I know I can reproduce the tests in this way, I will configure guix container for testing.

@trivialfis trivialfis force-pushed the remove-findgtest branch 8 times, most recently from a757860 to bdc230b Compare July 31, 2018 11:55
@trivialfis
Copy link
Member Author

trivialfis commented Jul 31, 2018

Hi, @hcho3 can you give me some insight about why the librt is not explicitly linked when building with gcc-4.8 and glibc-2.2.5 but work before this patch? I am confused which part of this patch triggered the linking failure of dmlc-core on jenkins. Travis built fine though. Thanks.

/opt/rh/devtoolset-2/root/usr/libexec/gcc/x86_64-redhat-linux/4.8.2/ld: CMakeFiles/dmlc_unit_tests.dir/unittest_thread_group.cc.o: undefined reference to symbol 'clock_gettime@@GLIBC_2.2.5'

/opt/rh/devtoolset-2/root/usr/libexec/gcc/x86_64-redhat-linux/4.8.2/ld: note: 'clock_gettime@@GLIBC_2.2.5' is defined in DSO /lib64/librt.so.1 so try adding it to the linker command line

/lib64/librt.so.1: could not read symbols: Invalid operation

collect2: error: ld returned 1 exit status

make[2]: *** [dmlc-core/test/unittest/dmlc_unit_tests] Error 1

make[1]: *** [dmlc-core/test/unittest/CMakeFiles/dmlc_unit_tests.dir/all] Error 2

make[1]: *** Waiting for unfinished jobs....

Scanning dependencies of target gpuxgboost

During removal of FindGTest.cmake, also

* Fix gtest include dirs.
* Remove some blanks and use PWD for gtest dir.
@codecov-io
Copy link

codecov-io commented Aug 5, 2018

Codecov Report

Merging #3533 into master will decrease coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3533      +/-   ##
============================================
- Coverage     45.77%   45.64%   -0.13%     
  Complexity      188      188              
============================================
  Files           166      166              
  Lines         13200    13233      +33     
  Branches        428      445      +17     
============================================
- Hits           6042     6040       -2     
- Misses         6954     6989      +35     
  Partials        204      204
Impacted Files Coverage Δ Complexity Δ
...ala/org/apache/spark/SparkParallelismTracker.scala 89.18% <0%> (-0.29%) 9% <0%> (ø)
.../xgboost4j/scala/example/spark/SparkTraining.scala
...ost4j/scala/example/spark/SparkMLlibPipeline.scala
...ost4j/scala/example/spark/SparkWithDataFrame.scala 0% <0%> (ø) 0% <0%> (?)
...t4j/scala/example/spark/SparkModelTuningTool.scala 0% <0%> (ø) 0% <0%> (?)
src/tree/updater_histmaker.cc 2.82% <0%> (ø) 0% <0%> (ø) ⬇️
src/c_api/c_api.cc 23.8% <0%> (+0.12%) 0% <0%> (ø) ⬇️
src/predictor/cpu_predictor.cc 68.49% <0%> (+3.12%) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 109473d...bf060c2. Read the comment docs.

@hcho3
Copy link
Collaborator

hcho3 commented Aug 5, 2018

@trivialfis How did you fix the error?

@trivialfis
Copy link
Member Author

@hcho3
Nope, I didn't. The linking requirement came from tests of dmlc-core. Those tests are not built before the error showed up, because dmlc couldn't find GTest, which is caused by placing find_package(GTest) at the very last of CMakeLists.txt in xgboost. I rearranged the find_package so dmlc-core found GTest and tried to build unittests, then the failure.

Hence the simple get away is not to let dmlc find GTest just like before. If you want sanity check for dmlc-core, it's preferable to do it in dmlc project.

@hcho3
Copy link
Collaborator

hcho3 commented Aug 6, 2018

LGTM

@RAMitchell RAMitchell merged commit 55caad6 into dmlc:master Aug 6, 2018
@trivialfis
Copy link
Member Author

trivialfis commented Aug 7, 2018

@hcho3 Sorry for bothering you on this. Can you give me some insight about Jenkins? It seems this PR might broke Jenkins. But I don't understand its behaviour.
Currently, both #3548 and #3557 failed to build on Jenkins with CUDA 8.0. The error is fail to find <gtest/gtest.h> header. It's so weird that Travis and Jenkins with CUDA 9.2 built successfully. I tried to diff the first part of output logs from Jenkins with CUDA 8.0 and 9.2:
On 8.0, there is these two lines:
-- Found the GTest includes: /workspace/gtest/include
-- Found the GTest library: /workspace/gtest/lib/libgtest.a

While on 9.2, the GTest line becomes:
-- Found GTest: /workspace/gtest/lib/libgtest.a

It seems on 8.0, the cmake build is still using old FindGTest, which built successfully.

Is there any cache mechanism I should be aware of?

@hcho3
Copy link
Collaborator

hcho3 commented Aug 7, 2018

@trivialfis Just found out that Docker has caching mechanism. Clearing it now.

@hcho3
Copy link
Collaborator

hcho3 commented Aug 7, 2018

Indeed, docker system prune did the trick.

EDIT. I also had to manually delete the old workspace, with had the old FindGTest.cmake.

@trivialfis
Copy link
Member Author

@hcho3 Thanks for helping out.

@hcho3
Copy link
Collaborator

hcho3 commented Aug 7, 2018

@trivialfis No problem. Thank you for your contribution.

@trivialfis trivialfis deleted the remove-findgtest branch August 14, 2018 00:34
@lock lock bot locked as resolved and limited conversation to collaborators Nov 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants