cmake: selectively rewrite install rpath by tchaikov · Pull Request #30028 · ceph/ceph

tchaikov · 2019-08-30T08:31:28Z

we use rpath for finding libceph-common, which is an internal library
not supposed to be used by 3rd party applications.

before this change, CMAKE_INSTALL_RPATH is set globally. so cmake is
asked to rewrite the RPATH for all installed targets. but this is not
needed. as some executables do not link against libceph-common. hence,
cmake complains when installing them, like:

CMake Error at src/test/mon/cmake_install.cmake:90 (file):
file RPATH_CHANGE could not write new RPATH:
/usr/lib64/ceph
to the file:
/home/abuild/rpmbuild/BUILDROOT/ceph-15.0.0-4347.g85a07b9.x86_64/usr/bin/ceph_test_log_rss_usage
No valid ELF RPATH or RUNPATH entry exists in the file;

so, in this change, we only apply INSTALL_RPATH to libceph-common.

Fixes: https://tracker.ceph.com/issues/41524
Signed-off-by: Kefu Chai kchai@redhat.com

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

Show available Jenkins commands

jenkins retest this please
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard backend
jenkins test docs
jenkins render docs

tchaikov · 2019-09-02T08:28:13Z

@smithfarm could you help test the update change? after a second thought, i found that the install path of a library cannot be populate to that of RPATH of executables linked against it. so i changed this PR to skip the global RPATH settings for those executables who don't link against the libraries from ceph.

tchaikov · 2019-09-02T08:29:07Z

being built at https://shaman.ceph.com/builds/ceph/wip-rpath-kefu/e8c66f5664c655488fcffb6dcf0f34d7db341465/

some executables like ceph_test_mon_memory_target does not link against gtest or gmock, so no need to link agains them. Signed-off-by: Kefu Chai <kchai@redhat.com>

smithfarm · 2019-09-02T12:26:23Z

@tchaikov openSUSE 15.1 is now built, along with CentOS, Ubuntu, etc., and reported to Shaman whenever you push a branch to ceph-ci.

The last Shaman/Chacra build (obtained by clicking on the link you posted) is green for openSUSE 15.1, I see, but it does not include the latest two commits.

Of course I can trigger a build directly in OBS but the result is almost certain to be exactly the same since Jenkins uses "osc build" to do the build on openSUSE 15.1.

smithfarm · 2019-09-02T12:27:43Z

@tchaikov Could you revert 021eb0a as part of this PR? As it is implicated in [1].

[1] https://bugzilla.novell.com/show_bug.cgi?id=1140346

tchaikov · 2019-09-02T13:08:33Z

but it does not include the latest two commits.

what do you mean by "the latest two commits"? i think https://github.com/ceph/ceph-ci/commits/wip-rpath-kefu includes both commits in this PR. if you think these commits was pushed after i posted the link. that's because i removed #30065 from this PR. as it's not related.

Could you revert 021eb0a as part of this PR? As it is implicated in [1].

again, i am not following you. 021eb0a is not in upstream master. so i am afraid i cannot revert it.

tchaikov · 2019-09-02T13:18:20Z

https://shaman.ceph.com/builds/ceph/wip-kefu-testing-2019-09-02-1658/afdb06fdd413f806d952fb51dd1d3220c16b3dec/

smithfarm · 2019-09-02T13:42:10Z

@tchaikov Never mind - I was confused (some "RPATH mitigation" commits have not been upstreamed yet). I will watch the latest Shaman build and approve if it's green.

smithfarm · 2019-09-02T13:50:06Z

OK, I can confirm that Shaman is green for the current state of this PR. However, I have two questions:

First question: The commit message says:

some executables like ceph_test_mon_memory_target does not link against
libradies built from source tree, like librados or libceph-common. so
cmake does not set RPATH for them. hence cmake complains like:

Can you fix the typo "libradies" -> "libraries" ?

Second question: The commit message says:

before this change, `CMAKE_INSTALL_RPATH` is set globally.

Is it still expected that this PR is removing the code that sets CMAKE_INSTALL_RPATH globally? In master I see:

$ ag CMAKE_INSTALL_RPATH
src/CMakeLists.txt
11:set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE)
12:if(NOT CMAKE_INSTALL_RPATH)
13:  set(CMAKE_INSTALL_RPATH "${CEPH_INSTALL_FULL_PKGLIBDIR}")

smithfarm · 2019-09-02T13:55:18Z

Third question. Should this PR change all instances of INSTALL_RPATH "" to SKIP_RPATH TRUE? There are many.

smithfarm · 2019-09-02T13:58:20Z

Fourth question: All of the INSTALL_RPATH "" lines were added to address various RPATH-related build failures. These build failures keep recurring on different files - they don't happen all at once, and sometimes the builds are green.

I don't understand the root cause of these failures. Before, we were adding INSTALL_RPATH "". Now we are fixing them by adding SKIP_RPATH TRUE. Is there any semantic difference between these two?

Could it be said that, in general, the RPATH build failures are a side effect of libceph-common?

tchaikov · 2019-09-03T02:21:36Z

I will watch the latest Shaman build and approve if it's green.

OK.

Can you fix the typo "libradies" -> "libraries" ?

yes.

Is it still expected that this PR is removing the code that sets CMAKE_INSTALL_RPATH globally?

no.

some executables like ceph_test_mon_memory_target do not link against libraries built from source tree, like librados and libceph-common. so cmake does not set RPATH for them. hence cmake complains like: before this change, `CMAKE_INSTALL_RPATH` is set globally. so cmake is asked to rewrite the RPATH for all installed targets. but this is not needed. as some executables do not link against libceph-common. hence, cmake complains when installing them, like: CMake Error at src/test/mon/cmake_install.cmake:90 (file): file RPATH_CHANGE could not write new RPATH: /usr/lib64/ceph to the file: /home/abuild/rpmbuild/BUILDROOT/ceph-15.0.0-4347.g85a07b9.x86_64/usr/bin/ceph_test_log_rss_usage No valid ELF RPATH or RUNPATH entry exists in the file; after this change, `SKIP_RPATH` is set for those executables which do not link against any libraries created from ceph source tree. so we can avoid setting the RPATH for these executables when `make install`. the same applies to libceph-common. Fixes: https://tracker.ceph.com/issues/41524 Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov · 2019-09-03T02:30:37Z

well, there are more than 2 questions =)

Should this PR change all instances of INSTALL_RPATH "" to SKIP_RPATH TRUE? There are many.

i haven't audited all of the INSTALL_RPATH changes. and i am not sure it's in the scope of this PR. but i would be happy to review such a change or do it by myself if i have more bandwidth.

Is there any semantic difference between these two?

sure. i am revising the commit message, hopefully it's more readable.

Could it be said that, in general, the RPATH build failures are a side effect of libceph-common?

could you be more specific?

sseshasa · 2019-09-03T05:40:38Z

LGTM

tchaikov · 2019-09-06T17:40:18Z

smithfarm · 2019-09-06T19:48:31Z

the second commit message still says:

before this change, `CMAKE_INSTALL_RPATH` is set globally. so cmake is
asked to rewrite the RPATH for all installed targets. but this is not
needed.

but I guess that no longer applies in the final version of this PR. In other words, CMAKE_INSTALL_RPATH is still set globally, correct?

Also, although there may be a semantic difference between INSTALL_RPATH "" and SKIP_RPATH TRUE, I'm hoping the end result (avoidance of FTBFS in openSUSE 15.1) is the same in both cases ;-)

smithfarm · 2019-10-01T10:57:16Z

Still getting FTBFS in OBS even with this patch applied:

[ 5036s] CMake Error at src/test/mon/cmake_install.cmake:90 (file):
[ 5036s]   file RPATH_CHANGE could not write new RPATH:
[ 5036s] 
[ 5036s]     /usr/lib64/ceph
[ 5036s] 
[ 5036s]   to the file:
[ 5036s] 
[ 5036s]     /home/abuild/rpmbuild/BUILDROOT/ceph-15.0.0-5052.4.x86_64/usr/bin/ceph_test_log_rss_usage
[ 5036s] 
[ 5036s]   No valid ELF RPATH or RUNPATH entry exists in the file;
[ 5036s] Call Stack (most recent call first):
[ 5036s]   src/test/cmake_install.cmake:268 (include)
[ 5036s]   src/cmake_install.cmake:378 (include)
[ 5036s]   cmake_install.cmake:42 (include)
[ 5036s] 
[ 5036s] 
[ 5036s] make: *** [Makefile:84: install] Error 1
[ 5036s] error: Bad exit status from /var/tmp/rpm-tmp.ju5s2T (%install)
[ 5036s] 
[ 5036s] 
[ 5036s] RPM build errors:
[ 5036s]     Bad exit status from /var/tmp/rpm-tmp.ju5s2T (%install)
[ 5036s]

Reopened #29922

tchaikov added bug-fix build/ops wip-kefu-testing labels Aug 30, 2019

tchaikov mentioned this pull request Aug 30, 2019

cmake: set empty RPATH for some test executables #29922

Merged

tchaikov force-pushed the wip-rpath branch from 50dd034 to e8c66f5 Compare September 2, 2019 08:23

cmake: only link against necessary libs

45bab2c

some executables like ceph_test_mon_memory_target does not link against gtest or gmock, so no need to link agains them. Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov force-pushed the wip-rpath branch from e8c66f5 to f995b9d Compare September 2, 2019 08:33

tchaikov requested review from smithfarm and sseshasa September 2, 2019 13:18

smithfarm requested a review from rjfd September 2, 2019 14:27

tchaikov force-pushed the wip-rpath branch from f995b9d to ed1dcda Compare September 3, 2019 02:22

tchaikov force-pushed the wip-rpath branch from ed1dcda to 6170815 Compare September 3, 2019 02:29

sseshasa approved these changes Sep 3, 2019

View reviewed changes

tchaikov merged commit 7e11f42 into ceph:master Sep 6, 2019

tchaikov deleted the wip-rpath branch September 6, 2019 17:40

smithfarm mentioned this pull request Apr 6, 2021

nautilus: cmake: set empty RPATH for some test executables #40619

Merged

Conversation

tchaikov commented Aug 30, 2019

Checklist

Uh oh!

tchaikov commented Sep 2, 2019

Uh oh!

tchaikov commented Sep 2, 2019

Uh oh!

smithfarm commented Sep 2, 2019

Uh oh!

smithfarm commented Sep 2, 2019

Uh oh!

tchaikov commented Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tchaikov commented Sep 2, 2019

Uh oh!

smithfarm commented Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smithfarm commented Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smithfarm commented Sep 2, 2019

Uh oh!

smithfarm commented Sep 2, 2019

Uh oh!

tchaikov commented Sep 3, 2019

Uh oh!

tchaikov commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sseshasa commented Sep 3, 2019

Uh oh!

tchaikov commented Sep 6, 2019

Uh oh!

smithfarm commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smithfarm commented Oct 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tchaikov commented Sep 2, 2019 •

edited

Loading

smithfarm commented Sep 2, 2019 •

edited

Loading

smithfarm commented Sep 2, 2019 •

edited

Loading

tchaikov commented Sep 3, 2019 •

edited

Loading

smithfarm commented Sep 6, 2019 •

edited

Loading