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: turn libcommon into a shared library #12840

Merged
merged 2 commits into from Jan 11, 2017

Conversation

Projects
None yet
4 participants
@tchaikov
Contributor

tchaikov commented Jan 9, 2017

prior to this change, libcommon is a convenient library which gets
linked into librados, librbd and libcephfs and all ceph executables.
this incurs some problems:

  • double dose of libcommon in memory space and HDD: waste of memory
    and disk space.
  • if an application links to two libraries including libcommon at the
    same time. take librados and libcephfs as an example, they could
    interfere with each other by changing the other guy's status.
    after this change, libcommon is tuned into a shared library and
    renamed to libceph-common. it will be installed into $prefix/lib/ceph,
    and packaged in librados2.

ceph.spec.in,debian/librados2.install: package libceph-common in
librados2.
CMakeLists.txt:

  • do not link against libboost-* if not necessary.
  • s/common/ceph-common/g
  • install libceph-common into $prefix/lib/ceph
  • set rpath to $prefix/lib/ceph
  • link against ceph-common if an executable needs access to non public
    symbols in ceph.

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

@@ -535,16 +538,38 @@ else()
endif()
if(ENABLE_SHARED)
list(APPEND libcommon_files
$<TARGET_OBJECTS:global_common_objs>)
endif(ENABLE_SHARED)

This comment has been minimized.

@rjfd

rjfd Jan 10, 2017

Contributor

nit: why keep this empty conditional block?

@@ -518,15 +518,6 @@ find_package(Boost 1.61 COMPONENTS ${BOOST_COMPONENTS} REQUIRED)
include_directories(SYSTEM ${Boost_INCLUDE_DIRS})
include_directories(SYSTEM ${PROJECT_BINARY_DIR}/include)
if (WITH_SYSTEM_BOOST)

This comment has been minimized.

@rjfd

rjfd Jan 10, 2017

Contributor

Was this necessary for turning libcommon into a shared library, or is just a cleanup? If it's just to clean up, maybe we should separate it from this PR.

This comment has been minimized.

@tchaikov

tchaikov Jan 10, 2017

Contributor

@rjfd at a first glance, yeah, it is indeed a cleanup. but it was a side effect of turning libcommon into a share lib. i forgot exactly why.

This comment has been minimized.

@tchaikov

tchaikov Jan 10, 2017

Contributor

oh, i moved the linkage to ceph-common. so it could a part of this commit.

+if(NOT WITH_SYSTEM_BOOST)
+  target_link_libraries(ceph-common ${ZLIB_LIBRARIES})
+endif()

tchaikov added some commits Jan 9, 2017

cmake: turn libcommon into a shared library
prior to this change, libcommon is a convenient library which gets
linked into librados, librbd and libcephfs and all ceph executables.
this incurs some problems:
 - double dose of libcommon in memory space and HDD: waste of memory
   and disk space.
 - if an application links to two libraries including libcommon at the
   same time. take librados and libcephfs as an example, they could
   interfere with each other by changing the other guy's status.
after this change, libcommon is tuned into a shared library and
renamed to libceph-common. it will be installed into $prefix/lib/ceph,
and packaged in librados2.

ceph.spec.in,debian/librados2.install: package libceph-common in
  librados2.
CMakeLists.txt:
  - do not link against libboost-* if not necessary.
  - s/common/ceph-common/g
  - install libceph-common into $prefix/lib/ceph
  - set rpath to $prefix/lib/ceph
  - link against ceph-common if an executable needs access to non public
    symbols in ceph.

Signed-off-by: Kefu Chai <kchai@redhat.com>
cmake: s/snappy/${SNAPPY_LIBRARIES}/
also s/z/${ZLIB_LIBARIES}/

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

This comment has been minimized.

Contributor

tchaikov commented Jan 10, 2017

changelog

  • remove empty if block in CMakeLists.txt.
@liewegas

This comment has been minimized.

Member

liewegas commented Jan 10, 2017

👍 on this change! This should avoid some longstanding headaches.

@rjfd

rjfd approved these changes Jan 10, 2017

LGTM

@rjfd

This comment has been minimized.

Contributor

rjfd commented Jan 10, 2017

@tchaikov will you run a set of suites in teuthology to check if nothing stops working?

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 10, 2017

@rjfd sure, i am on it.

@tchaikov

This comment has been minimized.

@tchaikov tchaikov merged commit 03db36f into ceph:master Jan 11, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@tchaikov tchaikov deleted the tchaikov:wip-libceph-common branch Jan 11, 2017

yuyuyu101 added a commit to yuyuyu101/ceph that referenced this pull request Jan 11, 2017

Merge pull request ceph#12840 from tchaikov/wip-libceph-common
cmake: turn libcommon into a shared library

Reviewed-by: Sage Weil <sage@redhat.com>
Reviewed-by: Ricardo Dias <rdias@suse.com>
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jan 12, 2017

When I try to build this in the OBS, I get:

[ 3178s] Install the project...
[ 3178s] -- Install configuration: "RelWithDebInfo"
[ 3178s] -- Installing: /home/abuild/rpmbuild/BUILDROOT/ceph-11.1.0+git.1484223486.fa61e0b-1.1.x86_64/usr/lib64/ceph/libceph-common.so
[ 3179s] CMake Error at src/cmake_install.cmake:53 (file):
[ 3179s]   file RPATH_CHANGE could not write new RPATH:
[ 3179s] 
[ 3179s]     /usr/lib64/ceph
[ 3179s] 
[ 3179s]   to the file:
[ 3179s] 
[ 3179s]     /home/abuild/rpmbuild/BUILDROOT/ceph-11.1.0+git.1484223486.fa61e0b-1.1.x86_64/usr/lib64/ceph/libceph-common.so
[ 3179s] 
[ 3179s]   No valid ELF RPATH or RUNPATH entry exists in the file;
[ 3179s] Call Stack (most recent call first):
[ 3179s]   cmake_install.cmake:37 (include)
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 12, 2017

@smithfarm could you try to build it with -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON? seems the rpath of libceph-common.so was removed at build-time, so cmake failed to update its rpath at install-time. but the downside is, the built executable might not be able to run in the build directory.

@tchaikov

This comment has been minimized.

smithfarm added a commit to SUSE/ceph that referenced this pull request Jan 12, 2017

rpm: run cmake with -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON
This fixes OBS builds broken by ceph#12840

Signed-off-by: Nathan Cutler <ncutler@suse.com>

orendu added a commit to orendu/ceph that referenced this pull request Jan 26, 2017

Fix broken async/rdma compilation since move to libceph-common
Was broken since merge of pull request ceph#12840

Signed-off-by: Oren Duer <oren@mellanox.com>

orendu added a commit to orendu/ceph that referenced this pull request Jan 26, 2017

cmake: Fix broken async/rdma compilation since move to libceph-common
Was broken since merge of pull request ceph#12840

Signed-off-by: Oren Duer <oren@mellanox.com>

chardan added a commit to chardan/ceph that referenced this pull request Feb 3, 2017

cmake: Fix broken async/rdma compilation since move to libceph-common
Was broken since merge of pull request ceph#12840

Signed-off-by: Oren Duer <oren@mellanox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment