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.

Sign up for GitHub

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

Jump to bottom

[one dmlc-core] by default build with dmlc-core from xgboost #91

Merged
merged 3 commits into from
Apr 26, 2019
Merged

[one dmlc-core] by default build with dmlc-core from xgboost #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7fb701b
[one dmlc-core] by default build with dmlc-core from xgboost, use
Apr 24, 2019
cb17156
remove dmlc-core
Apr 26, 2019
b6174af
compatible with windows
Apr 26, 2019
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
3 changes: 0 additions & 3 deletions .gitmodules
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ addons:
update: true

before_install:
- git clone https://github.com/dmlc/dmlc-core
- export TRAVIS=dmlc-core/scripts/travis/
- source ${TRAVIS}/travis_setup_env.sh
- ${TRAVIS}/travis_osx_install.sh
Expand Down
25 changes: 18 additions & 7 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ project(rabit VERSION 0.2.0)

option(RABIT_BUILD_TESTS "Build rabit tests" OFF)
option(RABIT_BUILD_MPI "Build MPI" OFF)
option(RABIT_BUILD_DMLC "Include DMLC_CORE in build" ON)

add_library(rabit src/allreduce_base.cc src/allreduce_robust.cc src/engine.cc src/c_api.cc)
add_library(rabit_base src/allreduce_base.cc src/engine_base.cc src/c_api.cc)
Expand All @@ -26,13 +27,23 @@ if(RABIT_BUILD_TESTS)
list(APPEND rabit_libs rabit_mock) # add to list to apply build settings, then remove
endif()

foreach(lib ${rabit_libs})
#include "./internal/utils.h"
target_include_directories(${lib} PUBLIC
"$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include>"
"$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/dmlc-core/include>"
)
endforeach()
if(RABIT_BUILD_DMLC)
foreach(lib ${rabit_libs})
#include "./internal/utils.h"
target_include_directories(${lib} PUBLIC
"$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include>"
"$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/dmlc-core/include>"
)
endforeach()
else()
foreach(lib ${rabit_libs})
#include "./internal/utils.h"
target_include_directories(${lib} PUBLIC
"$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include>"
"$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/../dmlc_core/include>"
Copy link
Member

Choose a reason for hiding this comment

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

may leave comments here about we assume rabit and dmlc-core are both put in the root path of the project which depends on it?

Copy link
Member

Choose a reason for hiding this comment

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

or we have some way to detect it

Copy link
Member

Choose a reason for hiding this comment

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

nvm, I think they only need to be in the same level of dir which is very likely to be the case

Copy link

Choose a reason for hiding this comment

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

This is a really ugly hack. Don’t assume something outside of your project. This is what cmake is for. Set the correct include dir on the target using cmake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/dmlc/rabit/pull/106/files

)
endforeach()
endif()

if(RABIT_BUILD_TESTS)
list(REMOVE_ITEM rabit_libs "rabit_mock") # remove here to avoid installing it
Expand Down
9 changes: 8 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
OS := $(shell uname)

RABIT_BUILD_DMLC = 0

ifeq ($(RABIT_BUILD_DMLC),1)
DMLC=dmlc-core
else
DMLC=../dmlc-core
endif

export WARNFLAGS= -Wall -Wextra -Wno-unused-parameter -Wno-unknown-pragmas -std=c++11
export CFLAGS = -O3 $(WARNFLAGS) -I $(DMLC)/include -I include/
export LDFLAGS =-Llib
Expand Down Expand Up @@ -74,7 +82,6 @@ ALIB= lib/librabit.a lib/librabit_empty.a lib/librabit_mock.a lib/librabit_base.
MPISLIB= lib/librabit_mpi.so
MPIALIB= lib/librabit_mpi.a
HEADERS=src/*.h include/rabit/*.h include/rabit/internal/*.h
DMLC=dmlc-core

.PHONY: clean all install mpi python lint doc doxygen

Expand Down
1 change: 0 additions & 1 deletion dmlc-core
Open in desktop
Submodule dmlc-core deleted from 13d5ac
3 changes: 3 additions & 0 deletions include/rabit/serializable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@
#include <vector>
#include <string>
#include "./internal/utils.h"

#ifndef DMLC_IO_H_
#include "../../dmlc-core/include/dmlc/io.h"
Copy link

Choose a reason for hiding this comment

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

This is a no go. If you do this you can just put the source of dmlc and rabit together in a single project.
Correct way to do this would be to just include dmlc/io.h and to let cmake correctly determine include dirs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/dmlc/rabit/pull/106/files

#endif // DMLC_IO_H_

namespace rabit {
/*!
Expand Down
10 changes: 5 additions & 5 deletions scripts/travis_script.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

# main script of travis
if [ ${TASK} == "lint" ]; then
make lint || exit -1
make lint RABIT_BUILD_DMLC=1 || exit -1
fi

if [ ${TASK} == "doc" ]; then
Expand All @@ -11,24 +11,24 @@ if [ ${TASK} == "doc" ]; then
fi

if [ ${TASK} == "build" ]; then
make all || exit -1
make all RABIT_BUILD_DMLC=1 || exit -1
fi

if [ ${TASK} == "mpi-build" ]; then
./scripts/mpi_build.sh
cd test
make mpi && make speed_test.mpi || exit -1
make mpi RABIT_BUILD_DMLC=1 && make speed_test.mpi RABIT_BUILD_DMLC=1 || exit -1
fi

if [ ${TASK} == "test" ]; then
cd test
make all || exit -1
make all RABIT_BUILD_DMLC=1 || exit -1
../scripts/travis_runtest.sh || exit -1
fi

if [ ${TASK} == "cmake-build" ]; then
mkdir build
cd build
cmake ..
cmake .. -DRABIT_BUILD_DMLC=ON
make all || exit -1
fi
2 changes: 1 addition & 1 deletion src/socket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ class TCPSocket : public Socket{
struct linger sl;
sl.l_onoff = 1; /* non-zero value enables linger option in kernel */
sl.l_linger = timeout; /* timeout interval in seconds */
if (setsockopt(sockfd, SOL_SOCKET, SO_LINGER, &sl, sizeof(sl)) == -1) {
if (setsockopt(sockfd, SOL_SOCKET, SO_LINGER, reinterpret_cast<char*>(&sl), sizeof(sl)) == -1) {
Socket::Error("SO_LINGER");
}
}
Expand Down