Re-enable @rpath on OS/X when using relative install prefix #1524
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## develop #1524 +/- ##
===========================================
+ Coverage 83.88% 83.90% +0.01%
===========================================
Files 132 132
Lines 10841 10841
Branches 2792 2792
===========================================
+ Hits 9094 9096 +2
+ Misses 1048 1046 -2
Partials 699 699
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mtl1979
force-pushed
the
mac-rpath
branch
26 times, most recently
from
e53b637
to
15414eb
Compare
| @@ -963,8 +963,15 @@ else() | |||
| set(PC_INC_INSTALL_DIR "${CMAKE_INSTALL_INCLUDEDIR}") | |||
| endif() | |||
|
|
|||
| if(APPLE) | |||
| option(WITH_RPATH "Enable RPATH for shared library" OFF) | |||
There was a problem hiding this comment.
Why have an option? Just allow it always.
There was a problem hiding this comment.
Because it would break installation to system directory. You can't just change CMAKE_INSTALL_LIBDIR to alter RPATH, because the default might match the new value as it's not guaranteed to be absolute path.
There was a problem hiding this comment.
Having an option allows backwards compatibility (if someone depended on this). But I think that default should be ON.
There was a problem hiding this comment.
The issue with having it ON by default is that there is no way to recover the full RPATH from the information in the cache because the cache isn't built at this stage yet... so the RPATH would be incorrect. With configure there is no issue as we don't use cache at all.
It's a lot more common for the CMAKE_INSTALL_LIBDIR to contain just lib than to contain full path like /usr/local/lib. As there is no cache yet, we can't use CMAKE_INSTALL_FULL_LIBDIR to determine what the full path would be, as it isn't updated before first install() call.
There was a problem hiding this comment.
In ITK, we will want to use RPATH by default. I assume the problem with @kislinsk's suggestion (remove custom code) is that configure has slightly different behavior?
There was a problem hiding this comment.
The problem is that cmake doesn't like relative paths, so ./lib must be specified as /lib even though the two refer to two distinct paths.
There was a problem hiding this comment.
It does seem pretty cludgy, but I guess if anyone knows of a cleaner way to do this, they can open a PR.
There was a problem hiding this comment.
cmake has a lot more limitations than configure script and I'm pretty sure cmake developers are not going to fix every limitation that is by design. I tried whole night different options and either it failed CI tests, or it didn't work as expected.
Only limitation in configure script and its Makefiles affecting this PR is that DESTDIR must end with `/´, otherwise relative paths don't work at all...
There was a problem hiding this comment.
@bradking Do you have some suggestion for CMake build system?
It wasn't easy to get working the right way as cmake doesn't support paths relative to |
See #1523.