AcademySoftwareFoundation / OpenColorIO

A color management framework for visual effects and animation.
https://opencolorio.org
BSD 3-Clause "New" or "Revised" License
1.76k stars 434 forks source link

v2.3.0 Python bindings source builds failing to load libOpenColorIO #1846

Closed remia closed 8 months ago

remia commented 12 months ago

Filing this following report by @mjtitchener-fn during today's TSC meeting for tracking purpose.

On the latest release 2.3 and main branch, when building the Python package from source, there is an issue with RPATH not correctly installed preventing the import of PyOpenColorIO when using shared build.

The issue is most likely here, CMAKE_INSTALL_RPATH is indeed always defined at this point because it is set globally here.

This does not affect the wheels because RPATH are defined independently here.

KevinJW commented 12 months ago

My build on Linux, sets an rpath based on the top level setting e.g. cmake command

cmake -DCMAKE_INSTALL_PREFIX="$DEST" -DCMAKE_INSTALL_RPATH_USE_LINK_PATH=TRUE -DOCIO_INSTALL_EXT_PACKAGES=ALL -DPython_EXECUTABLE=$PYTHON_EXECUTABLE -DPYTHON=$PYTHON_EXECUTABLE -DOCIO_USE_OPENEXR_HALF=ON -Dyaml-cpp_STATIC_LIBRARY=ON -DHalf_STATIC_LIBRARY=ON ${BASE}/${SOURCE}

and then near the end of the build we see:

-- Set runtime path of "/<my local install path>/opencolorio.v2.3.0-python2.7/bin/ociodisplay" to "$ORIGIN/../lib64:$ORIGIN" -- Set runtime path of "/<my local install path>/opencolorio.v2.3.0-python2.7/lib64/python2.7/site-packages/PyOpenColorIO/PyOpenColorIO.so" to "$ORIGIN/../lib64:$ORIGIN"

So the python bindings DSO is using the same rpath as the executables rather than the override, as @remia is suggesting.

This occurs for both Python 2.7 and 3.

KevinJW commented 12 months ago

So is it as simple as:

diff --git a/src/bindings/python/CMakeLists.txt b/src/bindings/python/CMakeLists.txt
index d7da50fd..94af5630 100644
--- a/src/bindings/python/CMakeLists.txt
+++ b/src/bindings/python/CMakeLists.txt
@@ -170,7 +170,7 @@ if(UNIX)
        set_property(TARGET PyOpenColorIO PROPERTY POSITION_INDEPENDENT_CODE ON)
 endif()

-if (UNIX AND NOT CMAKE_SKIP_RPATH AND NOT CMAKE_INSTALL_RPATH)
+if (UNIX AND NOT CMAKE_SKIP_RPATH AND CMAKE_INSTALL_RPATH)
     # Update the default RPATH so the Python binding dynamic library can find the OpenColorIO
     # dynamic library based on the default installation directory structure.
     if (APPLE)
mjtitchener-fn commented 12 months ago

I was chatting to @remia about this on Slack after the TSC call yesterday. It does appear to be that simple, the following fixed it for me.

- if (UNIX AND NOT CMAKE_SKIP_RPATH AND NOT CMAKE_INSTALL_RPATH)
+ if (UNIX AND NOT CMAKE_SKIP_RPATH)
KevinJW commented 12 months ago

Indeed, I left the AND CMAKE_INSTALL_RPATH as the variable is used inside the if 'block', but I'm in no way a cmake expert, around the effects of the variable being not set/false, etc