don't set rpath #203
don't set rpath #203
Conversation
|
I don't know much about RPATH. It would be great if someone else reviews this PR. |
|
Using RPATH on installation binaries can be extremely convenient in some cases (when installing the library in non-standard location or using a cmake superbuild) and undesirable in other cases (for example during debian packaging, see https://wiki.debian.org/RpathIssue). As the desirable RPATH behavior changes between different use cases, I suggest adding the In our projects we use the |
|
Yep, makes sense to keep the original behavior by default to avoid unexpected breakages:
|
CMakeModules/CompilerSettings.cmake
Outdated
| set(CMAKE_BUILD_WITH_INSTALL_RPATH FALSE) | ||
| set(CMAKE_INSTALL_RPATH "${CMAKE_INSTALL_LIBDIR_FULL}") | ||
| set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE) | ||
| endif(NO_DEFAULT_RPATH) |
There was a problem hiding this comment.
The style used in fcl's CMakeLists (at least in this one) is to avoid keeping the condition in the endif(), I would suggest removing this.
CMakeModules/CompilerSettings.cmake
Outdated
| # By default CMake sets RPATH for binaries in the build tree, but clears | ||
| # it when installing. Switch this option off if the default behaviour is | ||
| # desired. | ||
| option(NO_DEFAULT_RPATH "Set RPATH for installed binaries" ON) |
There was a problem hiding this comment.
All FCL CMake options that are not global CMake options (such as BUILD_SHARED_LIBS or BUILD_TESTING) are prefixed with FCL_, I suggest to use the same convention here. Furthermore given that this is a rather advanced option, I suggest marking it as advanced using mark_as_advanced.
|
@jslee02 I have some minor style remarks, but other then that I think that the logic is ok now. |
The currently used compiler settings explicitly set RPATH to installed binaries which is not desired behaviour in some circumstances, e.g. OpenEmbedded's QA checks report an issue about redundant RPATH set to the standard '/usr/lib' directory. This patch adds the configure option FCL_NO_DEFAULT_RPATH which is ON by default in order to preserve the current behaviour, but when set to OFF make CMake use its default RPATH settings, that is set RPATH for the binaries in the build tree, but clear them out for installed binaries. Signed-off-by: Dmitry Rozhkov <[email protected]>
|
Addressed the review comments and force pushed. |
|
It looks good to me. Thanks @rojkov and @traversaro ! Merging this RP. |
When building with OpenEmbedded the lib's rpath is set to
/usr/lib which is redundant and reported as a QA issue.
Signed-off-by: Dmitry Rozhkov [email protected]