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.

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

Remove setting CMAKE_MACOSX_RPATH to 0 by default #1632

Merged
merged 1 commit into from
Sep 23, 2023

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Sep 9, 2023

In conda-forge/gtsam-feedstock#15 (comment), setting CMAKE_MACOSX_RPATH to 0 (combined with the fact that the build system uses the gtsam.cpython-311-darwin.so in the build directory to copy in lib/python3.11/site-packages/gtsam/gtsam.cpython-311-darwin.so instead of installing it) lead to a quite difficult to debug strange behaviour.

As CMAKE_MACOSX_RPATH is set to 1 since CMake 3.0 , I wanted to see it the upstream CI fails. If setting CMAKE_MACOSX_RPATH to 0 make the CI fails and it is necessary to disable RPATH for some reason, the following CMake variable are the one currently suggested by CMake, and so I would try to set them instead of tweaking CMAKE_MACOSX_RPATH :

@varunagrawal
Copy link
Collaborator

Hi @traversaro. I tried going through the original issue but my lack of familiarity with that project is hindering my progress.

Can you please explain the issue in more detail? It seems like this affects Apple Silicon (e.g. M1) specifically, so I'd like to understand this in significant detail.

@traversaro
Copy link
Contributor Author

Hi @traversaro. I tried going through the original issue but my lack of familiarity with that project is hindering my progress.

Sure! Sorry, I opened the issue quickly to avoid forgetting about this.

Can you please explain the issue in more detail? It seems like this affects Apple Silicon (e.g. M1) specifically, so I'd like to understand this in significant detail.

The behavior of setting CMAKE_MACOSX_RPATH to 0 is:

While this does not mean that setting CMAKE_MACOSX_RPATH to 0 is wrong or a bug, this may lead to hard to debug behaviours like the one that we encountered in conda-forge/gtsam-feedstock#15 (comment), that is just an example and of which there is no need to understand all the details.

With this PR I wanted to:

  • bring to the attention of gtsam devs the possible problem of setting CMAKE_MACOSX_RPATH instead of using its default value
  • check if CI tests pass fine if we just use the default value of CMAKE_MACOSX_RPATH

In case CMAKE_MACOSX_RPATH is actually needed for anything, I think we can probably achieve the same result setting appropriate default values for the more widely used CMake variables CMAKE_SKIP_RPATH, CMAKE_SKIP_BUILD_RPATH andCMAKE_SKIP_INSTALL_RPATH.

@traversaro
Copy link
Contributor Author

In the conda-forge specific case, avoiding to set CMAKE_MACOSX_RPATH to 0 permitted to avoid had to manually modify the install_path of the library as done in https://github.com/borglab/gtsam-manylinux-build/blob/389fa7e6a8d2dacaf6cb2e3a3590436c476c630b/build-macos-new.sh#L144 . However, the case of building pypi packages may be different, so I would need to double check this.

@varunagrawal
Copy link
Collaborator

@traversaro thank you for the explanation. Given your point that the default value is 1 and has been since 2014 gives me enough reason to approve this PR. Especially with the CI passing.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @traversaro. People like you really help us with these subtle issues. :)

@varunagrawal varunagrawal merged commit 4bece8e into borglab:develop Sep 23, 2023
26 checks passed
@traversaro
Copy link
Contributor Author

Thanks for merging! Feel free to open issue and tag me if you find problems with these, for example in pypi wheel generation.

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.

2 participants