Use @loader_path instead of $ORIGIN on MacOS #403
Conversation
|
That solution is consistent with what I've seen used on Mac before, but I'll defer to @robertmaynard since he has far more experience than I do. |
There was a problem hiding this comment.
Like @vyasr, I think this looks generally right but I would defer to @robertmaynard.
| @@ -72,14 +72,19 @@ function(rapids_cython_add_rpath_entries) | |||
| list(APPEND cleaned_paths "${path}") | |||
| endforeach() | |||
|
|
|||
| if (CMAKE_SYSTEM_NAME STREQUAL "Linux") | |||
| set(platform_rpath_origin "\$ORIGIN") | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No need to escape $ORIGIN with CMake 3.16+
| @@ -72,14 +72,19 @@ function(rapids_cython_add_rpath_entries) | |||
| list(APPEND cleaned_paths "${path}") | |||
| endforeach() | |||
|
|
|||
| if (CMAKE_SYSTEM_NAME STREQUAL "Linux") | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree, we should explicit check for Darwin and otherwise use $ORIGIN
|
Comments addressed, PTAL |
|
@bdice is there anything else to do on this one? I updated against latest 23.06, but I am unable to set labels, as required by the "Label Checker" action. |
Added labels |
There was a problem hiding this comment.
All looks fine to me. Are we good to merge @robertmaynard @vyasr ?
|
I bumped this to 23.08 since 23.06 is in code freeze. |
|
Happy to merge this into 23.8 |
|
/merge |
Description
The dynamic libraries currently produced by rapids-cmake on MacOS for cython modules don't work, because the wrong "ORIGIN" marker is used. The typical suggestion appears to be to use
@loader_pathin place of$ORIGINon MacOS, although there is one more option that I can't currently recall.Please excuse my ignorance around your processes. Please feel free to make/request changes.
Checklist
cmake-format.jsonis up to date with these changes.include_guard(GLOBAL))