Skip to content
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

Use @loader_path instead of $ORIGIN on MacOS #403

Merged
merged 7 commits into from
May 30, 2023
Merged

Use @loader_path instead of $ORIGIN on MacOS #403

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
94d2700
Use @loader_path instead of $ORIGIN on MacOS
manopapad Apr 20, 2023
05c3d37
Only add a special case for MacOS, anywhere else will use $ORIGIN
manopapad May 2, 2023
89fb8a3
Replace all uses of \$ORIGIN with $ORIGIN
manopapad May 2, 2023
163bb58
Merge branch 'branch-23.06' into rpath-origin-mac
manopapad May 2, 2023
2cf5240
Merge branch 'branch-23.06' into rpath-origin-mac
manopapad May 16, 2023
8dfa21c
Merge branch 'branch-23.06' into rpath-origin-mac
bdice May 17, 2023
96bc14f
Merge branch 'branch-23.08' into rpath-origin-mac
bdice May 30, 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
7 changes: 6 additions & 1 deletion rapids-cmake/cython/add_rpath_entries.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,19 @@ function(rapids_cython_add_rpath_entries)
list(APPEND cleaned_paths "${path}")
endforeach()

if (CMAKE_SYSTEM_NAME STREQUAL "Linux")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this @loader_path behavior specific to Darwin and use $ORIGIN in an "else"? I'd like to avoid the string check for "Linux" as a defensive behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we should explicit check for Darwin and otherwise use $ORIGIN

set(platform_rpath_origin "\$ORIGIN")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be escaped? ($ORIGIN vs. \$ORIGIN) It isn't escaped here in the original code, but it is escaped in the install_rpath in the other file. I'm not 100% sure how variable interpolation works in CMake so escaping may be needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to escape $ORIGIN with CMake 3.16+

elseif (CMAKE_SYSTEM_NAME STREQUAL "Darwin")
set(platform_rpath_origin "@loader_path")
endif ()
get_property(targets GLOBAL PROPERTY "rapids_cython_associations_${_RAPIDS_CYTHON_TARGET}")
foreach(target IN LISTS targets)
# Compute the path relative to the current target.
set(target_paths)
get_target_property(target_source_dir ${target} SOURCE_DIR)
foreach(target_path IN LISTS cleaned_paths)
cmake_path(RELATIVE_PATH target_path BASE_DIRECTORY "${target_source_dir}")
list(APPEND target_paths "$ORIGIN/${target_path}")
list(APPEND target_paths "${platform_rpath_origin}/${target_path}")
endforeach()
list(JOIN target_paths ";" target_paths)

Expand Down
7 changes: 6 additions & 1 deletion rapids-cmake/cython/create_modules.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,12 @@ function(rapids_cython_create_modules)
install(TARGETS ${cython_module} DESTINATION ${_RAPIDS_CYTHON_INSTALL_DIR})

# Default the INSTALL_RPATH for all modules to $ORIGIN.
set_target_properties(${cython_module} PROPERTIES INSTALL_RPATH "\$ORIGIN")
if (CMAKE_SYSTEM_NAME STREQUAL "Linux")
set(platform_rpath_origin "\$ORIGIN")
elseif (CMAKE_SYSTEM_NAME STREQUAL "Darwin")
set(platform_rpath_origin "@loader_path")
endif ()
set_target_properties(${cython_module} PROPERTIES INSTALL_RPATH "${platform_rpath_origin}")

# Store any provided associated targets in a global list
foreach(associated_target IN LISTS _RAPIDS_CYTHON_ASSOCIATED_TARGETS)
Expand Down