AVSLab / basilisk

Astrodynamics simulation framework
https://hanspeterschaub.info/basilisk
ISC License
147 stars 61 forks source link

MacOS: Cannot `pip` install from source due to hard-coded RPATH #826

Closed dpad closed 1 month ago

dpad commented 1 month ago

Describe the bug The current settings for the RPATH (path where the linker tries to find linked libraries) seem to be hard-coded to the absolute build directory for MacOS builds. This affects pip-based installation for MacOS, in particular for cases where pip copies the source code into the temporary build directory. The following cases are affected (on MacOS only):

I'm not 100% sure what the correct fix is for setting the RPATH appropriately, but the following patch at least seems to work when I asked someone with a MacOS to test it.

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
   set(CMAKE_SKIP_BUILD_RPATH FALSE) # use, i.e. don't skip the full RPATH for the build tree
   set(CMAKE_BUILD_WITH_INSTALL_RPATH TRUE) # when building, don't use the install RPATH already (but later on when
                                            # installing)
-  set(CMAKE_INSTALL_RPATH "${CMAKE_BINARY_DIR}/Basilisk") # the RPATH to be used when installing
+  set(CMAKE_INSTALL_RPATH "${CMAKE_BINARY_DIR}/Basilisk"  # the RPATH to be used when installing
+      # Also add relative paths so we can install from wheel/sdist.
+      # TODO: Is there a proper way to set relative library paths without just
+      # guessing how many parent ".." directories to use?
+      "@loader_path"
+      "@loader_path/.."
+      "@loader_path/../.."
+  )

   # don't add the automatically determined parts of the RPATH which point to directories outside the build tree to the
   # install RPATH

To reproduce Steps to reproduce the behavior:

  1. On a MacOS, Try to install the source directly from git: pip install git+https://github.com/AVSLab/basilisk.git
  2. Run something with Basilisk, see errors about missing libraries

Desktop (please complete the following information):

sassy-asjp commented 1 month ago

Note that the patch for macOS mirrors what is already happening in Linux:

https://github.com/AVSLab/basilisk/blob/bf17f69eca74cf3d2619bc41f0e3d385aae2e415/src/CMakeLists.txt#L502

Since I think that the files get put into the install directory by pip, instead of running some cmake generated installation command, it might be the best possible without really fiddling with how pip is installing the files.

schaubh commented 1 month ago

Howdy @dpad and @sassy-asjp , thanks for posting this issue and discussion. We have been building wheels on our Linux distribution only so far for use with BSK-RL tools. If you need me to test a branch on my macOS systems, let me know.

sassy-asjp commented 1 month ago

@schaubh

We've tested our fix on our side and think it works generally well. We think our fix seems kinda hacky, but a similar trick has been used on the Linux side already, so probably does make sense. If the open source project is happy with it, we can open up a pull request for it.

schaubh commented 1 month ago

Howdy @sassy-asjp , I'm open to including this approach. We are calling the wheel generation a beta feature for now, so we can explore some of these solutions.

To summarize, you are looking to add

set(CMAKE_INSTALL_RPATH "\$ORIGIN/../../") 

to the macOS cmake settings, not use the "@loader_path/../.." solution @dpad had?

@dpad , any thoughts on @sassy-asjp approach?

dpad commented 1 month ago

@schaubh I believe what @sassy-asjp is saying is that "@loader_path/../.." is the MacOS equivalent of the Linux "\$ORIGIN/../.." option (the latter of which is already present in the current Basilisk build system, but the former is not yet). So we're suggesting to add the "@loader_path/../.." to the MacOS RPATH option, so that it matches the Linux option and works for the cases I mentioned.

schaubh commented 1 month ago

Ok, thanks for the clarification. I'm open to try this on a PR branch. As long as this doesn't break our current build systems we should be fine.