fix(build): Fix recently broken rpath setting #4618
Conversation
As part of the big python wheel merge, compiler.cmake got changed in a subtle way in how it sets CMAKE_INSTALL_RPATH. This broken iv for me on a Mac with dynamic libraries. Strangely (and I'm too lazy to chase down exactly why), oiiotool seems fine. So this was missed entirely by the CI, which never runs 'iv', since it's an interactive app. (Note to self: we should find some way to at least verify that it can execute as part of the testsuite.) Anyway, I believe that this change adds the install lib area back to the path, and it does seem to repair my ability to run iv on my end. Signed-off-by: Larry Gritz <[email protected]>
|
@zachlewis @JeanChristopheMorinPerso Opinions/review desired. |
src/cmake/compiler.cmake
Outdated
| set (CMAKE_INSTALL_RPATH ${CMAKE_INSTALL_FULL_LIBDIR} | ||
| ${BASEPOINT} | ||
| ${BASEPOINT}/${CMAKE_INSTALL_LIBDIR}) |
There was a problem hiding this comment.
I'm tempted to suggest something like this.
| set (CMAKE_INSTALL_RPATH ${CMAKE_INSTALL_FULL_LIBDIR} | |
| ${BASEPOINT} | |
| ${BASEPOINT}/${CMAKE_INSTALL_LIBDIR}) | |
| set (CMAKE_INSTALL_RPATH ${BASEPOINT} ${BASEPOINT}/${CMAKE_INSTALL_LIBDIR}) | |
| if (NOT SKBUILD) | |
| set (CMAKE_INSTALL_RPATH ${CMAKE_INSTALL_FULL_LIBDIR} ${CMAKE_INSTALL_RPATH}) | |
| endif() |
This would avoid having the full path when building the wheels. The wheels should be as hermetic as possible.
There was a problem hiding this comment.
Sure, that makes sense.
There was a problem hiding this comment.
Does it make sense to add ${BASEPOINT}/../lib to the SKBUILD clause?
BASEPOINT is going to be something/bin, where the executable lives. ${BASEPOINT}/${CMAKE_INSTALL_LIBDIR} is therefore something/bin/lib which is useless, that's never where the libraries will be, right?
There was a problem hiding this comment.
Umm, won't BASEPOINT be lib for the library? We technically need two different RPATH don't we? One for the executables and one for the libs no?
There was a problem hiding this comment.
RPATH is only used to find libraries, whether the thing needing the library is a binary or another library. So RPATH always needs to include the lib directory.
There was a problem hiding this comment.
Sure, what I meant was that the executable will have a different RPATH. The executable needs $ORIGIN/../lib while the library(ies) need $ORIGIN.
There was a problem hiding this comment.
Right. But ${BASEPOINT}/../${CMAKE_INSTALL_LIBDIR} points to the same correct place in both cases, and seems to me less error-prone than trying to set rpath attributes separately for every library and binary target -- seems easy for somebody adding libraries or binaries later or modifying the cmake scripts to mess it up by not understanding this subtlety.
There was a problem hiding this comment.
I pushed an amendment to the PR that swaps the absolute path for what I was discussing above.
Signed-off-by: Larry Gritz <[email protected]>
As part of the recent big python wheel merge, compiler.cmake got changed in a subtle way in how it sets CMAKE_INSTALL_RPATH.
This broken iv for me on a Mac with dynamic libraries. Strangely (and I'm too lazy to chase down exactly why), oiiotool seems fine. So this was missed entirely by the CI, which never runs 'iv', since it's an interactive app. (Note to self: we should find some way to at least verify that it can execute as part of the testsuite.)
Anyway, I believe that this change adds the install lib area back to the path, and it does seem to repair my ability to run iv on my end.