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 <lg@larrygritz.com>
|
@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.
OH, I didn't realize we were tweaking it in those spots as well. So it seems to me that the real problem is that the one in fancy_add_executable is ONLY happening for SKBUILD. When I build on my Mac from source, it's not getting this logic that includes the ../lib suffix, and that's why it's not finding the library.
There was a problem hiding this comment.
So you have some special logic in libutil (for libOpenImageIO_Util), but not in libOpenImageIO (for the main libOpenImageIO)?
Can all these scattered special cases be removed and just use the one in compiler.cmake for everything?
There was a problem hiding this comment.
So you have some special logic in libutil (for libOpenImageIO_Util), but not in libOpenImageIO (for the main libOpenImageIO)?
Yep, to make sure libOpenImageIO_Util can find libOpenImageIO.
(libOpenImageIO doesn't link against libOpenImageIO_Util)
(Without doing this, the default cibuildwheel repair wheel step would flag libOpenImageIO_Util as missing dependencies (even though libOpenImageIO was right there in the same directory. So frustrating)
Can all these scattered special cases be removed and just use the one in compiler.cmake for everything?
I do believe so!!
There was a problem hiding this comment.
OK, updated once again. Got rid of the special cases in fancy_add_executable and in libutil/CMakeLists.txt. Just the one place in compiler.cmake, and it has both the basepoint and basepoint/../lib, which I think covers all of our bases.
There was a problem hiding this comment.
Thanks Larry, this seems correct to me now.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
|
Any further comments on this? |
Signed-off-by: Larry Gritz <lg@larrygritz.com>
…ion#4618) 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. --------- Signed-off-by: Larry Gritz <lg@larrygritz.com>
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.