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

cmake: build boost as an external project #15376

Merged
merged 7 commits into from Jun 14, 2017

Conversation

Projects
None yet
4 participants
@tchaikov
Contributor

tchaikov commented May 31, 2017

No description provided.

@tchaikov tchaikov changed the title from [do not review][DNM] cmake: remove boost submodule to [DNM] cmake: remove boost submodule May 31, 2017

@tchaikov tchaikov requested a review from cbodley May 31, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 31, 2017

@liewegas i think in this way

  • the compilation will speed up a little bit without pulling the whole boost repo and its submodules.
  • and we don't need to rerun src/boost/bootstrap.sh and b2 every time cmake is launched

i will update make-dist to include the boost tarball if this looks fine to you.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jun 1, 2017

retest this please.

@cbodley cbodley self-assigned this Jun 2, 2017

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Jun 10, 2017

Hi, Kefu, are you working on this? It would be great for me.... terrible network in the China...

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jun 10, 2017

@Liuchang0812 i don't follow you. this PR is posted by me, so presumably i am working on this?

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Jun 10, 2017

sorry for confusing you. just hope it could be merged as soon as possible. i try to pull boost code in home. it always failed, because of terrible network.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jun 10, 2017

it's RFC, and pending on some early review.

@@ -300,6 +300,7 @@ endif()
# tcmalloc heap profiler
set(heap_profiler_files ${TCMALLOC_srcs})
add_library(heap_profiler_objs OBJECT ${heap_profiler_files})
add_dependencies(heap_profiler_objs Boost::boost)

This comment has been minimized.

@cbodley

cbodley Jun 12, 2017

Contributor

these deps are how we trigger the external project, and guarantee that the boost headers are available?

if we add a new target that doesn't add this dependency, does it fail somehow? or would it silently pull in the boost system headers?

this seems error-prone, but i'm not sure how to reconcile this with the need for include_directories(BEFORE SYSTEM ${Boost_INCLUDE_DIRS}) to override the system headers

This comment has been minimized.

@tchaikov

tchaikov Jun 12, 2017

Contributor

these deps are how we trigger the external project, and guarantee that the boost headers are available?

yes.

if we add a new target that doesn't add this dependency, does it fail somehow? or would it silently pull in the boost system headers?

yes. no, unless that target depends on some other target which depends on Boost::boost.

this seems error-prone, but i'm not sure how to reconcile this with the need for include_directories(BEFORE SYSTEM ${Boost_INCLUDE_DIRS}) to override the system headers

i feel the same. i was evaluating some alternatives. but none of them are feasible or elegant enough. for example, to preprocess CMakeLists.txt using cmake and add add_dependencies(${THE_TARGET} Boost::boost) for every target.

This comment has been minimized.

@cbodley

cbodley Jun 13, 2017

Contributor

for example, to preprocess CMakeLists.txt using cmake and add add_dependencies(${THE_TARGET} Boost::boost) for every target.

we might consider something hacky like this, which overrides add_library() and add_executable() to inject a global dependency

This comment has been minimized.

@tchaikov

tchaikov Jun 13, 2017

Contributor

that's .... hacky! i will give it a shot. thanks for the link @cbodley !

cmake: use boost target instead of libary path while linking against …
…them

Signed-off-by: Kefu Chai <kchai@redhat.com>

@tchaikov tchaikov changed the title from [DNM] cmake: remove boost submodule to cmake: build boost as an external project Jun 14, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jun 14, 2017

i am removing the commit to remove the boost submodule. will try to get this one merged first, and have that commit in a follow-up PR.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jun 14, 2017

@cbodley fixed and repushed, could you take a look again?

if(ENABLE_SHARED)
set(Boost_USE_STATIC_LIBS OFF)
else()
set(Boost_USE_STATIC_LIBS ON)
endif()
find_package(Boost 1.61 COMPONENTS ${BOOST_COMPONENTS} REQUIRED)

This comment has been minimized.

@cbodley

cbodley Jun 14, 2017

Contributor

is there a reason this is staying at 1.61, when BuildBoost.cmake and the submodule are at 1.63?

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

yes, if user happens to have 1.61 in his/her system, but he/she does not want to rebuild boost. then he/she will benefit from this.

This comment has been minimized.

@cbodley

cbodley Jun 14, 2017

Contributor

ah right, this is the WITH_SYSTEM_BOOST part. what about the build_boost(1.61 part below?

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

yeah, i was trying to map the logic of FindBoost to BuildBoost, and wanted to make BuildBoost more generic in hope that other projects are able to leverage this cmake module with less work. if it seems useless and over-engineering, i am happy to remove this cruft.

This comment has been minimized.

@cbodley

cbodley Jun 14, 2017

Contributor

i don't object to BuildBoost, just wondering if we should call it with build_boost(1.63

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

sure, updated!

@cbodley

This comment has been minimized.

Contributor

cbodley commented Jun 14, 2017

i am removing the commit to remove the boost submodule. will try to get this one merged first, and have that commit in a follow-up PR.

if this doesn't remove the submodule, but adds an external project that fetches the boost tarball separately, is there a net benefit to merging this as it is?

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jun 14, 2017

if this doesn't remove the submodule, but adds an external project that fetches the boost tarball separately, is there a net benefit to merging this as it is?

@cbodley,

  • no more mysterious cmake warnings from FindBoost,
  • no more ./b2 calls when cmake is re-launched.
  • and some developers with slow internet access would be happier if the "src/boost" is optional. believe it or not, it took me more than 1 hour to pull the whole boost repo from github for the first time.
@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Jun 14, 2017

it took me more than 1 hour to pull the whole boost repo from github for the first time.

can't agree more....

# but i don't want to bother with stripping off the TARGET_OBJECTS elements
# from it. so let's stick with the old behavior now.
cmake_policy(SET CMP0051 OLD)
endif()

This comment has been minimized.

@cbodley

cbodley Jun 14, 2017

Contributor

okay, it took me a while to realize this was related to get_target_property(sources ${target} SOURCES) in maybe_add_boost_dep(). could you mention maybe_add_boost_dep() in this comment?

This comment has been minimized.

@adamemerson

adamemerson Jun 14, 2017

Contributor

Even if we keep the submodule it's worth having boost build when we're building and not configuing.

@cbodley

@tchaikov thanks for helping me understand everything here 👍

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jun 14, 2017

changelog

  • amend the comment for the reason why we have cmake_policy(SET CMP0051 OLD)
  • build_boost(1.63,...)

tchaikov added some commits Apr 16, 2017

cmake: build boost as an external project
Signed-off-by: Kefu Chai <kchai@redhat.com>
cmake: include boost before default system directory
so ceph will prefer the local boost installation to the one in system if
any.

Signed-off-by: Kefu Chai <kchai@redhat.com>
common: #include <atomic> in headers where atomic<> is used
so these headers are self-contained.

Signed-off-by: Kefu Chai <kchai@redhat.com>
cmake: do not include Monitor.cc in osd
Signed-off-by: Kefu Chai <kchai@redhat.com>
cmake: move common/util.cc into ceph-common
Signed-off-by: Kefu Chai <kchai@redhat.com>
cmake: do not link libmon with mon_common_objs
ceph-common includes them already.

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jun 14, 2017

@cbodley thanks for your pointers and the review, will merge it once shaman is happy: https://shaman.ceph.com/builds/ceph/wip-15376-kefu/

@tchaikov tchaikov merged commit 1e1b108 into ceph:master Jun 14, 2017

2 of 4 checks passed

arm64 make check arm64 make check started
Details
make check running make check
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details

@tchaikov tchaikov deleted the tchaikov:wip-remove-boost-submodule branch Jun 14, 2017

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Jun 15, 2017

Hi, Kefu, I tested this patch today. It seems that we don't remove boost submodule. we will pull whole boost repos when we execute ./do_cmake.sh. Need we remove boost submodule or update do_cmake.sh script?

➜  test-cmake git clone https://github.com/ceph/ceph.git
Cloning into 'ceph'...
remote: Counting objects: 515228, done.
remote: Compressing objects: 100% (26/26), done.
remote: Total 515228 (delta 12), reused 9 (delta 1), pack-reused 515201
Receiving objects: 100% (515228/515228), 215.12 MiB | 352.00 KiB/s, done.
Resolving deltas: 100% (406117/406117), done.
Checking connectivity... done.
➜  test-cmake cd ceph 
➜  ceph git:(master) ./do_cmake.sh 
+ git submodule update --init --recursive
Submodule 'ceph-erasure-code-corpus' (https://github.com/ceph/ceph-erasure-code-corpus.git) registered for path 'ceph-erasure-code-corpus'
Submodule 'ceph-object-corpus' (https://github.com/ceph/ceph-object-corpus.git) registered for path 'ceph-object-corpus'
Submodule 'src/Beast' (https://github.com/ceph/Beast.git) registered for path 'src/Beast'
Submodule 'src/blkin' (https://github.com/ceph/blkin) registered for path 'src/blkin'
Submodule 'src/boost' (https://github.com/boostorg/boost.git) registered for path 'src/boost'
Submodule 'src/civetweb' (https://github.com/ceph/civetweb) registered for path 'src/civetweb'
Submodule 'src/crypto/isa-l/isa-l_crypto' (https://github.com/01org/isa-l_crypto) registered for path 'src/crypto/isa-l/isa-l_crypto'
Submodule 'src/dpdk' (https://github.com/ceph/dpdk) registered for path 'src/dpdk'
Submodule 'src/erasure-code/jerasure/gf-complete' (https://github.com/ceph/gf-complete.git) registered for path 'src/erasure-code/jerasure/gf-complete'
Submodule 'src/erasure-code/jerasure/jerasure' (https://github.com/ceph/jerasure.git) registered for path 'src/erasure-code/jerasure/jerasure'
Submodule 'src/googletest' (https://github.com/ceph/googletest) registered for path 'src/googletest'
Submodule 'src/isa-l' (https://github.com/ceph/isa-l) registered for path 'src/isa-l'
Submodule 'src/lua' (https://github.com/ceph/lua.git) registered for path 'src/lua'
Submodule 'src/rapidjson' (https://github.com/ceph/rapidjson) registered for path 'src/rapidjson'
Submodule 'src/rocksdb' (https://github.com/ceph/rocksdb) registered for path 'src/rocksdb'
Submodule 'src/spdk' (https://github.com/ceph/spdk.git) registered for path 'src/spdk'
Submodule 'src/xxHash' (https://github.com/ceph/xxHash.git) registered for path 'src/xxHash'
Submodule 'src/zstd' (https://github.com/facebook/zstd) registered for path 'src/zstd'
Cloning into 'ceph-erasure-code-corpus'...
remote: Counting objects: 3362, done.
remote: Compressing objects: 100% (60/60), done.
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jun 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment