Skip to content

Fix wrong RPATH being injected into v2.3.0 Python bindings DSO #1849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
doug-walker merged 2 commits into from
Dec 7, 2023
Merged

Fix wrong RPATH being injected into v2.3.0 Python bindings DSO #1849

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb42672
Fix wrong RPATH being injected into Python bindings DSO
KevinJW Sep 6, 2023
206edf0
Merge branch 'main' into 1846-v230-python-bindings-source-builds-fail…
doug-walker Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/bindings/python/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Copy Markdown
Collaborator

@remia remia Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to rollback to the condition we had in RB-2.2 and do the same for CompilerFlags.
if (UNIX AND NOT CMAKE_SKIP_RPATH)

I originally was trying to have the Python build only ship with the required RPATH and nothing more but it turned out it created this bug and I also later realised that the wheel have a "repair" step at the end, which for macOS simply replace the RPATH entirely (as can be seen in GHA logs; I assume Linux is similar but the log is quieter) so these were probably not really needed.

Copy link
Copy Markdown
Collaborator

@remia remia Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Having a dummy change in the .github/workflows/wheel_workflow.yml might be nice to trigger the wheel build and make sure this doesn't have any negative impact).

Copy link
Copy Markdown
Collaborator

@remia remia Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KevinJW What do you think about simplifying the above as suggested in the earlier comment? Would be great to have this in the upcoming release @doug-walker is planning for this week.

# 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)
Expand Down