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: fix rpath on shared libraries and binaries targets #12927

Merged
merged 1 commit into from Jan 18, 2017

Conversation

Projects
None yet
3 participants
@rjfd
Contributor

rjfd commented Jan 13, 2017

While building the Ceph RPMs in opensuse build system, we got several errors as mention in the end of the discussion in #12840

The problem was due to the attempt of CMake to rewrite the RPATH of the binaries, or shared libraries, that didn't have any RPATH linked during the building phase.

Signed-off-by: Ricardo Dias rdias@suse.com

@rjfd rjfd requested a review from tchaikov Jan 13, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jan 13, 2017

@rjfd Can you push this wip branch to ceph/ceph-ci.git so Shaman will build it?

@rjfd

This comment has been minimized.

Contributor

rjfd commented Jan 13, 2017

@smithfarm done. The branch in ceph-ci repo with this fix is wip-rdias-testing

@rjfd

This comment has been minimized.

@tchaikov tchaikov self-assigned this Jan 14, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 16, 2017

@rjfd @smithfarm i'd suggest pass -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON to cmake in the ceph.spec recipe a try, as i found that this change is not only changing the INSTALL_RPATH of almost all shared libraries and some executables. if we are not performing tests in the built path, we can just build the libraries and executables with the INSTALL_RPATH, this approach

  • avoids the relink at the install-time.
  • is a more consistent way to fix this issue, and
  • has lower maintenance overhead

what do you think?

@rjfd

This comment has been minimized.

Contributor

rjfd commented Jan 16, 2017

@tchaikov I've tried to build with CMAKE_BUILD_WITH_INSTALL_RPATH=ON and the build failed due to undefined references. It looks like that the build of some libraries, such as libcephfs requires that libceph-common to be found, and since RPATH is set to the INSTALL directory, it cannot find it during compilation/linking.

That is why I used the fix submitted in this PR.

@@ -20,6 +22,8 @@ install(TARGETS ceph_scratchtoolpp DESTINATION bin)
add_executable(ceph_radosacl radosacl.cc)
target_link_libraries(ceph_radosacl librados global)
set_target_properties(ceph_radosacl PROPERTIES

This comment has been minimized.

@tchaikov

tchaikov Jan 16, 2017

Contributor

@rjfd why you don't set the INSTALL_RPATH for all executables? and what makes ceph_radosacl, ceph_scratchtool and ceph_test_libcephfs different from others?

This comment has been minimized.

@rjfd

rjfd Jan 16, 2017

Contributor

you're right ceph_radosacl, ceph_scratchtool, and ceph_test_libcephfs don't need to set the INSTALL_RPATH property. I wrongly identified them.

why you don't set the INSTALL_RPATH for all executables?

I don't understand what you mean, currently INSTALL_RPATH is already set for all binaries, what I'm currently doing is setting an empty 'INSTALL_RPATH` for all binaries that don't depend on any internal (also being built) target

This comment has been minimized.

@tchaikov

tchaikov Jan 16, 2017

Contributor

@rjfd got it. i just wanted to understand the rationale of your change.

@rjfd

This comment has been minimized.

Contributor

rjfd commented Jan 16, 2017

@tchaikov pushed the new changes addressing your comment

@@ -914,6 +918,8 @@ if(WITH_RBD)
if(WITH_KRBD)
add_library(krbd STATIC krbd.cc
$<TARGET_OBJECTS:parse_secret_objs>)
set_target_properties(krbd PROPERTIES

This comment has been minimized.

@tchaikov

tchaikov Jan 18, 2017

Contributor

krbd is a convenient library, and we don't even install it. why would we need to set rpath for it?

This comment has been minimized.

@rjfd

rjfd Jan 18, 2017

Contributor

You're right. Will fix this.

@rjfd

This comment has been minimized.

Contributor

rjfd commented Jan 18, 2017

@tchaikov pushed new change.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 18, 2017

@rjfd, could you update the commit message explaining your change in more details :

for example:

The problem was due to the attempt of CMake to rewrite the RPATH of the binaries, or shared libraries, that didn't have any RPATH linked during the building phase.

currently INSTALL_RPATH is already set for all binaries, what I'm currently doing is setting an empty 'INSTALL_RPATH` for all binaries that don't depend on any internal (also being built) target

modulo this, it's good to merge.

cmake: fix rpath on shared libraries and executables
The problem was due to the attempt of CMake to rewrite the RPATH of
the executables, or shared libraries, that didn't have any RPATH linked
during the building phase.

Currently INSTALL_RPATH is already set for all binaries. This patch
sets an empty INSTALL_RPATH for all binaries that don't depend on
any internal (also being built) target.

Signed-off-by: Ricardo Dias <rdias@suse.com>
@rjfd

This comment has been minimized.

Contributor

rjfd commented Jan 18, 2017

@tchaikov done!

@tchaikov tchaikov merged commit 80b749f into ceph:master Jan 18, 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

@rjfd rjfd deleted the rjfd:wip-fix-rpath-suse branch Jan 19, 2017

@rjfd rjfd removed the wip-rdias-testing label Mar 27, 2018

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