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

Refactor rabit tests #6096

Merged
merged 13 commits into from
Sep 9, 2020
Merged

Conversation

trivialfis
Copy link
Member

@trivialfis trivialfis commented Sep 8, 2020

@trivialfis trivialfis mentioned this pull request Sep 8, 2020
7 tasks
@trivialfis
Copy link
Member Author

Somehow there are duplicated tests defined in cc files and cpp files.

@trivialfis
Copy link
Member Author

@hcho3 I will do a drive by fix for #6092 .

Copy link
Collaborator

@hcho3 hcho3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a first round of review.

You also need to change line 287 of the root CMakeLists.txt: We should install rabit along with objxgboost when BUILD_STATIC_LIB is on. So add it to the variable INSTALL_TARGETS. The reason is that Rabit is a dependency of xgboost, and when you install a static lib, you need to install all of its dependencies.

rabit/CMakeLists.txt Outdated Show resolved Hide resolved
@hcho3
Copy link
Collaborator

hcho3 commented Sep 8, 2020

I'll do another round of review tomorrow, as I am about to go to bed now.

@trivialfis
Copy link
Member Author

@hcho3 Thanks for the explanation. Let me do some tests, I think it's possible to link multiple static libraries into one.

@hcho3
Copy link
Collaborator

hcho3 commented Sep 8, 2020

I think it's possible to link multiple static libraries into one.

You can turn rabit into an object library, so that librabit.a won't need to be installed. Even then, we'd still need to declare rabit in the export set of INSTALL(TARGETS) command, so my previous recommendation still applies.

@trivialfis
Copy link
Member Author

Got it, so I need to restore the installation logic.

@hcho3
Copy link
Collaborator

hcho3 commented Sep 8, 2020

No, you can reuse the installation logic in the root CMakeLists.txt. Just modify INSTALL_TARGETS variable in line 287. No need to maintain installation logic in two places.

@trivialfis
Copy link
Member Author

@hcho3 Thanks again. Modified the script to install it along with objxgboost when building static library.

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2020

Codecov Report

Merging #6096 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6096   +/-   ##
=======================================
  Coverage   78.52%   78.52%           
=======================================
  Files          12       12           
  Lines        3069     3069           
=======================================
  Hits         2410     2410           
  Misses        659      659           

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 b0001a6...a3844ae. Read the comment docs.

@trivialfis trivialfis merged commit 3dcd85f into dmlc:master Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants