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

[CI] Simplify CMake build with modern CMake techniques #5871

Merged
merged 3 commits into from
Jul 8, 2020
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
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ addons:
- graphviz
- openssl
- libgit2
- lz4
- wget
- r
update: true
Expand Down
25 changes: 12 additions & 13 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,16 @@ if (USE_OPENMP)
find_package(OpenMP REQUIRED)
endif (USE_OPENMP)

# core xgboost
add_subdirectory(${xgboost_SOURCE_DIR}/src)

# dmlc-core
msvc_use_static_runtime()
add_subdirectory(${xgboost_SOURCE_DIR}/dmlc-core)
set_target_properties(dmlc PROPERTIES
CXX_STANDARD 14
CXX_STANDARD_REQUIRED ON
POSITION_INDEPENDENT_CODE ON)
list(APPEND LINKED_LIBRARIES_PRIVATE dmlc)
if (MSVC)
target_compile_options(dmlc PRIVATE
-D_CRT_SECURE_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE)
Expand All @@ -128,6 +130,7 @@ if (MSVC)
-D_CRT_SECURE_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE)
endif (TARGET dmlc_unit_tests)
endif (MSVC)
target_link_libraries(objxgboost PUBLIC dmlc)

# rabit
set(RABIT_BUILD_DMLC OFF)
Expand All @@ -136,13 +139,13 @@ set(RABIT_WITH_R_LIB ${R_LIB})
add_subdirectory(rabit)

if (RABIT_MOCK)
list(APPEND LINKED_LIBRARIES_PRIVATE rabit_mock_static)
target_link_libraries(objxgboost PUBLIC rabit_mock_static)
if (MSVC)
target_compile_options(rabit_mock_static PRIVATE
-D_CRT_SECURE_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE)
endif (MSVC)
else()
list(APPEND LINKED_LIBRARIES_PRIVATE rabit)
target_link_libraries(objxgboost PUBLIC rabit)
if (MSVC)
target_compile_options(rabit PRIVATE
-D_CRT_SECURE_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE)
Expand All @@ -164,19 +167,16 @@ if (R_LIB)
add_subdirectory(${xgboost_SOURCE_DIR}/R-package)
endif (R_LIB)

# core xgboost
list(APPEND LINKED_LIBRARIES_PRIVATE Threads::Threads ${CMAKE_THREAD_LIBS_INIT})
# Plugin
add_subdirectory(${xgboost_SOURCE_DIR}/plugin)
add_subdirectory(${xgboost_SOURCE_DIR}/src)
target_link_libraries(objxgboost PUBLIC dmlc)
set(XGBOOST_OBJ_SOURCES "${XGBOOST_OBJ_SOURCES};$<TARGET_OBJECTS:objxgboost>")

#-- library
if (BUILD_STATIC_LIB)
add_library(xgboost STATIC ${XGBOOST_OBJ_SOURCES})
add_library(xgboost STATIC)
else (BUILD_STATIC_LIB)
add_library(xgboost SHARED ${XGBOOST_OBJ_SOURCES})
add_library(xgboost SHARED)
endif (BUILD_STATIC_LIB)
target_link_libraries(xgboost PRIVATE objxgboost)

if (USE_NVTX)
enable_nvtx(xgboost)
Expand All @@ -192,7 +192,6 @@ target_include_directories(xgboost
INTERFACE
$<INSTALL_INTERFACE:${CMAKE_INSTALL_PREFIX}/include>
$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include>)
target_link_libraries(xgboost PRIVATE ${LINKED_LIBRARIES_PRIVATE})

# This creates its own shared library `xgboost4j'.
if (JVM_BINDINGS)
Expand All @@ -201,7 +200,8 @@ endif (JVM_BINDINGS)
#-- End shared library

#-- CLI for xgboost
add_executable(runxgboost ${xgboost_SOURCE_DIR}/src/cli_main.cc ${XGBOOST_OBJ_SOURCES})
add_executable(runxgboost ${xgboost_SOURCE_DIR}/src/cli_main.cc)
target_link_libraries(runxgboost PRIVATE objxgboost)
if (USE_NVTX)
enable_nvtx(runxgboost)
endif (USE_NVTX)
Expand All @@ -211,7 +211,6 @@ target_include_directories(runxgboost
${xgboost_SOURCE_DIR}/include
${xgboost_SOURCE_DIR}/dmlc-core/include
${xgboost_SOURCE_DIR}/rabit/include)
target_link_libraries(runxgboost PRIVATE ${LINKED_LIBRARIES_PRIVATE})
set_target_properties(
runxgboost PROPERTIES
OUTPUT_NAME xgboost
Expand Down
2 changes: 1 addition & 1 deletion Jenkinsfile
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ def BuildCPU() {
# This step is not necessary, but here we include it, to ensure that DMLC_CORE_USE_CMAKE flag is correctly propagated
# We want to make sure that we use the configured header build/dmlc/build_config.h instead of include/dmlc/build_config_default.h.
# See discussion at https://github.com/dmlc/xgboost/issues/5510
${dockerRun} ${container_type} ${docker_binary} tests/ci_build/build_via_cmake.sh
${dockerRun} ${container_type} ${docker_binary} tests/ci_build/build_via_cmake.sh -DPLUGIN_LZ4=ON -DPLUGIN_DENSE_PARSER=ON
${dockerRun} ${container_type} ${docker_binary} build/testxgboost
"""
// Sanitizer test
Expand Down
20 changes: 8 additions & 12 deletions R-package/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,36 +6,32 @@ file(GLOB_RECURSE R_SOURCES
${CMAKE_CURRENT_LIST_DIR}/src/*.c)
# Use object library to expose symbols
add_library(xgboost-r OBJECT ${R_SOURCES})

set(R_DEFINITIONS
target_compile_definitions(xgboost-r
PUBLIC
-DXGBOOST_STRICT_R_MODE=1
-DXGBOOST_CUSTOMIZE_GLOBAL_PRNG=1
-DDMLC_LOG_BEFORE_THROW=0
-DDMLC_DISABLE_STDIN=1
-DDMLC_LOG_CUSTOMIZE=1
-DRABIT_CUSTOMIZE_MSG_
-DRABIT_STRICT_CXX98_)
Comment on lines +9 to 17
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These flags are unique to R and they make changes to objxgboost. The resulting libxgboost.so is substantially different from what the Python and JVM bindings need.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To share a common library, we'd need to carefully extract R-specific code into a separate CMake target, so that the R flags only modify the separate target and not the common library. This may be nontrivial effort.

Copy link
Member

Choose a reason for hiding this comment

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

For cran yes, for conda maybe no. Let me take a look, see how much effort does it take.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you review this PR as it is? Let's follow up with another PR for the common library discussion.

target_compile_definitions(xgboost-r
PRIVATE ${R_DEFINITIONS})
target_include_directories(xgboost-r
PRIVATE
${LIBR_INCLUDE_DIRS}
${PROJECT_SOURCE_DIR}/include
${PROJECT_SOURCE_DIR}/dmlc-core/include
${PROJECT_SOURCE_DIR}/rabit/include)
target_link_libraries(xgboost-r PRIVATE ${LIBR_CORE_LIBRARY})
set_target_properties(
xgboost-r PROPERTIES
CXX_STANDARD 14
CXX_STANDARD_REQUIRED ON
POSITION_INDEPENDENT_CODE ON)

set(XGBOOST_DEFINITIONS "${XGBOOST_DEFINITIONS};${R_DEFINITIONS}" PARENT_SCOPE)
set(XGBOOST_OBJ_SOURCES $<TARGET_OBJECTS:xgboost-r> PARENT_SCOPE)
set(LINKED_LIBRARIES_PRIVATE ${LINKED_LIBRARIES_PRIVATE} ${LIBR_CORE_LIBRARY} PARENT_SCOPE)

if (USE_OPENMP)
target_link_libraries(xgboost-r PRIVATE OpenMP::OpenMP_CXX)
endif ()
# Get compilation and link flags of xgboost-r and propagate to objxgboost
target_link_libraries(objxgboost PUBLIC xgboost-r)
# Add all objects of xgboost-r to objxgboost
target_sources(objxgboost INTERFACE $<TARGET_OBJECTS:xgboost-r>)

set(LIBR_HOME "${LIBR_HOME}" PARENT_SCOPE)
set(LIBR_EXECUTABLE "${LIBR_EXECUTABLE}" PARENT_SCOPE)
set(LIBR_EXECUTABLE "${LIBR_EXECUTABLE}" PARENT_SCOPE)
9 changes: 3 additions & 6 deletions jvm-packages/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
find_package(JNI REQUIRED)

add_library(xgboost4j SHARED
${PROJECT_SOURCE_DIR}/jvm-packages/xgboost4j/src/native/xgboost4j.cpp
${XGBOOST_OBJ_SOURCES})
${PROJECT_SOURCE_DIR}/jvm-packages/xgboost4j/src/native/xgboost4j.cpp)
target_link_libraries(xgboost4j PRIVATE objxgboost)
target_include_directories(xgboost4j
PRIVATE
${JNI_INCLUDE_DIRS}
Expand All @@ -16,7 +16,4 @@ set_target_properties(
xgboost4j PROPERTIES
CXX_STANDARD 14
CXX_STANDARD_REQUIRED ON)
target_link_libraries(xgboost4j
PRIVATE
${LINKED_LIBRARIES_PRIVATE}
${JAVA_JVM_LIBRARY})
target_link_libraries(xgboost4j PRIVATE ${JAVA_JVM_LIBRARY})
7 changes: 3 additions & 4 deletions plugin/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
if (PLUGIN_LZ4)
set(PLUGIN_SOURCES ${PLUGIN_SOURCES}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Notice the typo: PLUGIN_SOURCES should have been PLUGINS_SOURCES.

${xgboost_SOURCE_DIR}/plugin/lz4/sparse_page_lz4_format.cc PARENT_SCOPE)
target_sources(objxgboost PRIVATE ${xgboost_SOURCE_DIR}/plugin/lz4/sparse_page_lz4_format.cc)
target_link_libraries(objxgboost PUBLIC lz4)
endif (PLUGIN_LZ4)

if (PLUGIN_DENSE_PARSER)
set(PLUGINS_SOURCES ${PLUGINS_SOURCES}
${xgboost_SOURCE_DIR}/plugin/dense_parser/dense_libsvm.cc PARENT_SCOPE)
target_sources(objxgboost PRIVATE ${xgboost_SOURCE_DIR}/plugin/dense_parser/dense_libsvm.cc)
endif (PLUGIN_DENSE_PARSER)
2 changes: 1 addition & 1 deletion plugin/lz4/sparse_page_lz4_format.cc
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ inline void CompressArray<DType>::Write(dmlc::Stream* fo) {
}

template<typename StorageIndex>
class SparsePageLZ4Format : public SparsePageFormat {
class SparsePageLZ4Format : public SparsePageFormat<SparsePage> {
public:
explicit SparsePageLZ4Format(bool use_lz4_hc)
: use_lz4_hc_(use_lz4_hc) {
Expand Down
23 changes: 9 additions & 14 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ file(GLOB_RECURSE CPU_SOURCES *.cc *.h)
list(REMOVE_ITEM CPU_SOURCES ${xgboost_SOURCE_DIR}/src/cli_main.cc)

#-- Object library
# Object library is necessary for jvm-package, which creates its own shared
# library.
# Object library is necessary for jvm-package, which creates its own shared library.
add_library(objxgboost OBJECT)
target_sources(objxgboost PRIVATE ${CPU_SOURCES})
if (USE_CUDA)
file(GLOB_RECURSE CUDA_SOURCES *.cu *.cuh)
add_library(objxgboost OBJECT ${CPU_SOURCES} ${CUDA_SOURCES} ${PLUGINS_SOURCES})
target_compile_definitions(objxgboost
PRIVATE -DXGBOOST_USE_CUDA=1)
target_sources(objxgboost PRIVATE ${CUDA_SOURCES})
target_compile_definitions(objxgboost PRIVATE -DXGBOOST_USE_CUDA=1)
target_include_directories(objxgboost PRIVATE ${xgboost_SOURCE_DIR}/cub/)
target_compile_options(objxgboost PRIVATE
$<$<COMPILE_LANGUAGE:CUDA>:--expt-extended-lambda>
Expand All @@ -20,7 +20,7 @@ if (USE_CUDA)
find_package(Nccl REQUIRED)
target_include_directories(objxgboost PRIVATE ${NCCL_INCLUDE_DIR})
target_compile_definitions(objxgboost PRIVATE -DXGBOOST_USE_NCCL=1)
list(APPEND SRC_LIBS ${NCCL_LIBRARY})
target_link_libraries(objxgboost PUBLIC ${NCCL_LIBRARY})
endif (USE_NCCL)

if (USE_NVTX)
Expand All @@ -47,8 +47,6 @@ if (USE_CUDA)
CUDA_STANDARD 14
CUDA_STANDARD_REQUIRED ON
CUDA_SEPARABLE_COMPILATION OFF)
else (USE_CUDA)
add_library(objxgboost OBJECT ${CPU_SOURCES} ${PLUGINS_SOURCES})
endif (USE_CUDA)

target_include_directories(objxgboost
Expand Down Expand Up @@ -79,8 +77,7 @@ set_target_properties(objxgboost PROPERTIES
target_compile_definitions(objxgboost
PRIVATE
-DDMLC_LOG_CUSTOMIZE=1 # enable custom logging
$<$<NOT:$<CXX_COMPILER_ID:MSVC>>:_MWAITXINTRIN_H_INCLUDED>
${XGBOOST_DEFINITIONS})
$<$<NOT:$<CXX_COMPILER_ID:MSVC>>:_MWAITXINTRIN_H_INCLUDED>)
if (USE_DEBUG_OUTPUT)
target_compile_definitions(objxgboost PRIVATE -DXGBOOST_USE_DEBUG_OUTPUT=1)
endif (USE_DEBUG_OUTPUT)
Expand All @@ -97,14 +94,12 @@ if (XGBOOST_BUILTIN_PREFETCH_PRESENT)
endif (XGBOOST_BUILTIN_PREFETCH_PRESENT)

find_package(Threads REQUIRED)
list(APPEND SRC_LIBS Threads::Threads ${CMAKE_THREAD_LIBS_INIT})
target_link_libraries(objxgboost PUBLIC Threads::Threads ${CMAKE_THREAD_LIBS_INIT})

if (USE_OPENMP OR USE_CUDA) # CUDA requires OpenMP
find_package(OpenMP REQUIRED)
list(APPEND SRC_LIBS OpenMP::OpenMP_CXX)
target_link_libraries(objxgboost PRIVATE OpenMP::OpenMP_CXX)
target_link_libraries(objxgboost PUBLIC OpenMP::OpenMP_CXX)
endif (USE_OPENMP OR USE_CUDA)
set(LINKED_LIBRARIES_PRIVATE "${LINKED_LIBRARIES_PRIVATE};${SRC_LIBS}" PARENT_SCOPE)

# For MSVC: Call msvc_use_static_runtime() once again to completely
# replace /MD with /MT. See https://github.com/dmlc/xgboost/issues/4462
Expand Down
2 changes: 1 addition & 1 deletion tests/ci_build/Dockerfile.cpu
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ SHELL ["/bin/bash", "-c"] # Use Bash as shell
# Install all basic requirements
RUN \
apt-get update && \
apt-get install -y tar unzip wget git build-essential doxygen graphviz llvm libasan2 libidn11 && \
apt-get install -y tar unzip wget git build-essential doxygen graphviz llvm libasan2 libidn11 liblz4-dev && \
# CMake
wget -nv -nc https://cmake.org/files/v3.13/cmake-3.13.0-Linux-x86_64.sh --no-check-certificate && \
bash cmake-3.13.0-Linux-x86_64.sh --skip-license --prefix=/usr && \
Expand Down
7 changes: 3 additions & 4 deletions tests/cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ if (USE_CUDA)
file(GLOB_RECURSE CUDA_TEST_SOURCES "*.cu")
list(APPEND TEST_SOURCES ${CUDA_TEST_SOURCES})
endif (USE_CUDA)
add_executable(testxgboost ${TEST_SOURCES} ${XGBOOST_OBJ_SOURCES}
add_executable(testxgboost ${TEST_SOURCES}
${xgboost_SOURCE_DIR}/plugin/example/custom_obj.cc)
target_link_libraries(testxgboost PRIVATE objxgboost)

if (USE_CUDA)
# OpenMP is mandatory for CUDA
Expand Down Expand Up @@ -73,10 +74,8 @@ set_target_properties(
CXX_STANDARD_REQUIRED ON)
target_link_libraries(testxgboost
PRIVATE
${GTEST_LIBRARIES}
${LINKED_LIBRARIES_PRIVATE})
${GTEST_LIBRARIES})

target_compile_definitions(testxgboost PRIVATE ${XGBOOST_DEFINITIONS})
set_output_directory(testxgboost ${xgboost_BINARY_DIR})

# This grouping organises source files nicely in visual studio
Expand Down