Skip to content

Conversation

@kennykos
Copy link

See kokkos/pykokkos#313 for issue.

@IvanGrigorik
Copy link
Collaborator

IvanGrigorik commented Nov 24, 2025

@kennykos
What about previous macOS versions?
Should you still modify MACOSX_RPATH like it was done before, or not?

@gliga
Copy link

gliga commented Nov 24, 2025

Could you explain the reason this works.
If it works, I am fine to merge.

@kennykos
Copy link
Author

@gliga @IvanGrigorik

To address your concerns, my latest commit adds back in MACOSX_RPATH for backwards compatibility and directly translates the UNIX INSTALL_RPATH to macos lingo (as per https://docs.conda.io/projects/conda-build/en/stable/resources/use-shared-libraries.html, $ORIGIN is evq with @loader_path)

@JBludau
Copy link
Contributor

JBludau commented Nov 25, 2025

What does the CMake documentation say on this?

@IvanGrigorik
Copy link
Collaborator

@kennykos can you try to override just INSTALL_RPATH? It looks like that should work as well

@kennykos
Copy link
Author

@kennykos can you try to override just INSTALL_RPATH? It looks like that should work as well

Removing MACOSX_RPATH worked on my local machine, I just pushed the code!

@kennykos
Copy link
Author

What does the CMake documentation say on this?

Here is the CMake documentation for INSTALL_RPATH, but it's a little unclear. The conda documentation stateing the equivalence of $ORIGIN and @loader_path is what I used to motivate this change.

@JBludau
Copy link
Contributor

JBludau commented Nov 26, 2025

why are we not using what is recommended for apple platforms?
https://cmake.org/cmake/help/v4.2/prop_tgt/INSTALL_NAME_DIR.html#prop_tgt:INSTALL_NAME_DIR

@kennykos
Copy link
Author

why are we not using what is recommended for apple platforms? https://cmake.org/cmake/help/v4.2/prop_tgt/INSTALL_NAME_DIR.html#prop_tgt:INSTALL_NAME_DIR

This worked on my local install, my last commit pushed the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants