Add rpath to liblldb so vendors can ship their own python framework (or others)

[aadsm]

Summary

I want to be able to specify which python framework to use for lldb in macos. With python2.7 we could just rely on the MacOS one but python3.7 is not shipped with the OS.
An alternative is to use the one shipped with Xcode but that could be path dependent or maybe the user doesn't have Xcode installed at all.
A definite solution is to just ship a python framework with lldb. To make this possible I added "@loader_path/../../../" to the rpath so it points to the same directory as the LLDB.framework, this way we can just drop any frameworks there.

[labath]

In principle, this looks pretty similar to D67942, and my opinion on it is the same -- I don't think we should be in the business of trying to package the transitive set of lldb dependencies. I think the lldb install target should install the stuff that it has built itself, and the rest should be up to some higher level packaging system.

[hhb]

In D69931#1736607, @labath wrote:

In principle, this looks pretty similar to D67942, and my opinion on it is the same -- I don't think we should be in the business of trying to package the transitive set of lldb dependencies. I think the lldb install target should install the stuff that it has built itself, and the rest should be up to some higher level packaging system.

+1 it will never end if we go down this way.

[smeenai]

I agree on not getting into the business of installing Python ourselves, but there's also the rpath issue. The Windows case is different because you just put all your DLLs in the same directory (or some other directory in your PATH) and you're done. With macOS, you have to specify an rpath for liblldb to be able to find the Python framework. It'd be really nice to support adding additional rpaths in the build system itself (it could be a more generic mechanism; it doesn't have to be Python-specific). CMake does offer CMAKE_INSTALL_RPATH, but that applies to every single target. You could also use install_name_tool after building to modify the rpath, but that feels like a kludge.

How would people feel about a patch that just adds the ability to set additional rpaths on liblldb?

[labath]

In D69931#1738097, @smeenai wrote:

I agree on not getting into the business of installing Python ourselves, but there's also the rpath issue. The Windows case is different because you just put all your DLLs in the same directory (or some other directory in your PATH) and you're done. With macOS, you have to specify an rpath for liblldb to be able to find the Python framework. It'd be really nice to support adding additional rpaths in the build system itself (it could be a more generic mechanism; it doesn't have to be Python-specific). CMake does offer CMAKE_INSTALL_RPATH, but that applies to every single target. You could also use install_name_tool after building to modify the rpath, but that feels like a kludge.

How would people feel about a patch that just adds the ability to set additional rpaths on liblldb?

+ @beanz for any cmake/osx/framework insights.

Having the ability to add additional rpaths to the (lib)lldb framework seems fine, if there's no other way to arrange that simply dropping dependent libraries into some directory "just works". It's not just windows where this is possible. On linux (and I assume this applies to non-framework osx builds too), you can do that by just dropping things into the "$prefix/lib" directory -- in our default setup both executables and shared libraries get the rpath of "$ORIGIN/../lib" meaning that both executables and shared libraries can find any additional deps there. Is it possible to do something like that with the framework build too? I.e., set the rpath of the LLDB.framework so that it searches for dependencies next to the framework itself? Then you'd just need to drop the python framework next to lldb...

[aadsm]

That should work fine yeah. It should be a matter of adding "@loader_path/../../../" to the rpath.

[beanz]

It is a bit gross that Python does an @rpath install name, that is generally against convention. I can see adding an option to add rpaths to liblldb making sense. I certainly agree LLDB shouldn't be in the business of packaging transitive dependencies.

In a broader sense, Frameworks are dylibs. They can have all the same rpath magic that a dylib has. When you link a macho, the static linker pulls the install_name from each dylib you link against and embeds those as the dylibs to load. Since Python 3.7's install_name is @rpath based, it means that the dynamic linker will look for it in all the @rpath-relative locations specified by liblldb when linking. The proposed path in this patch, -rpath "@loader_path/../../../", uses the @loader_path expansion which is the directory containing the binary that the load command is in (in this case liblldb's directory). Popping 3 directories up from that is likely not sane in most build configurations, but if it works for you meh...

[smeenai]

In D69931#1739063, @beanz wrote:

It is a bit gross that Python does an @rpath install name, that is generally against convention. I can see adding an option to add rpaths to liblldb making sense. I certainly agree LLDB shouldn't be in the business of packaging transitive dependencies.

In a broader sense, Frameworks are dylibs. They can have all the same rpath magic that a dylib has. When you link a macho, the static linker pulls the install_name from each dylib you link against and embeds those as the dylibs to load. Since Python 3.7's install_name is @rpath based, it means that the dynamic linker will look for it in all the @rpath-relative locations specified by liblldb when linking. The proposed path in this patch, -rpath "@loader_path/../../../", uses the @loader_path expansion which is the directory containing the binary that the load command is in (in this case liblldb's directory). Popping 3 directories up from that is likely not sane in most build configurations, but if it works for you meh...

The assumption here is that you'd have a directory layout like the following:

|- LLDB.framework
  |- Versions
    |- A
      |- LLDB
|- Python3.framework
  |- Versions
    |- 3.7
      |- Python3

Python3's install name is @rpath/Python3.framework/Versions/3.7/Python3, so by adding -rpath "@loader_path/../../.." to LLDB, you make @labath's request work:

In D69931#1738285, @labath wrote:

Is it possible to do something like that with the framework build too? I.e., set the rpath of the LLDB.framework so that it searches for dependencies next to the framework itself? Then you'd just need to drop the python framework next to lldb...

The assumption, of course, is that LLDB is being built as a framework as well, but I believe that's the standard setup on Darwin.

[aadsm]

Updating to only add an rpath that points to the directory where LLDB.frameworks gets installed.

[aadsm]

The proposed path in this patch, -rpath "@loader_path/../../../", uses the @loader_path expansion which is the directory containing the binary that the load command is in (in this case liblldb's directory). Popping 3 directories up from that is likely not sane in most build configurations, but if it works for you meh...

This is exactly what Apple ships with Xcode today:

$ otool -l /Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/liblldbPluginScriptInterpreterPython3.dylib | grep RPATH -A 2
          cmd LC_RPATH
      cmdsize 72
         path @loader_path/../../../../../../../../Library/Frameworks/ (offset 12)
--
          cmd LC_RPATH
      cmdsize 72
         path @loader_path/../../../../../Developer/Library/Frameworks/ (offset 12)
--
          cmd LC_RPATH
      cmdsize 72
         path @loader_path/../../../../Developer/Library/Frameworks/ (offset 12)
--
          cmd LC_RPATH
      cmdsize 48
         path @loader_path/../../../../Frameworks (offset 12)
--
          cmd LC_RPATH
      cmdsize 40
         path @loader_path/../../../ (offset 12)

$ otool -L /Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.7/Python3
/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.7/Python3:
	@rpath/Python3.framework/Versions/3.7/Python3 (compatibility version 3.7.0, current version 3.7.0)

But they build liblldbPluginScriptInterpreterPython3 as a dylib instead of statically inside liblldb (so that they can have both 2.7 and 3.7).

[labath]

This seems reasonable to me.

path @loader_path/../../../../../../../../Library/Frameworks/

LOL :)

[smeenai]

Commit message needs to be updated.

lldb/cmake/modules/LLDBFramework.cmake

124

You may wanna add the reason for this to the comment as well.

[smeenai]

lldb/cmake/modules/LLDBFramework.cmake

124

Doesn't need to be super detailed ... even just adding something like "so that we can find any frameworks installed beside it" should be enough.

[aadsm]

Clarify the point of the install_path

[smeenai]

LGTM

lldb/cmake/modules/LLDBFramework.cmake

125

Nit: 80 columns

lldb/cmake/modules/LLDBFramework.cmake

if(NOT IOS)
 
   add_custom_command(TARGET liblldb POST_BUILD
     COMMAND ${CMAKE_COMMAND} -E copy_directory
             ${clang_resource_headers_dir}
             $<TARGET_FILE_DIR:liblldb>/Resources/Clang/include
     COMMENT "LLDB.framework: copy clang vendor-specific headers"
   )
 endif()
+
+# Import Python framework if specified
+if(PYTHON_FRAMEWORK_DIR)
+  install(DIRECTORY ${PYTHON_FRAMEWORK_DIR}
+    DESTINATION ${LLDB_FRAMEWORK_INSTALL_DIR}
+    COMPONENT liblldb)
+  if(PYTHON_FRAMEWORK_INSTALL_RPATH)
+    set_property(TARGET liblldb APPEND PROPERTY INSTALL_RPATH
+      ${PYTHON_FRAMEWORK_INSTALL_RPATH})
+  endif()
+endif()