[lldb] Remove tools copied into LLDB.framework before install

[JDevlieghere]

Summary

CMake supports building Framework bundles for Apple platforms. We rely
on this functionality to create LLDB.framework. From CMake's
perspective, a framework is associated with a single target. In reality,
this is often not the case. In addition to a main library (liblldb) the
frameworks often contains associated resources that are their own CMake
targets, such as debugserver and lldb-argdumper.

When building and using the framework to run the test suite, those
binaries are expected to be in LLDB.framework. We have a function
(lldb_add_to_buildtree_lldb_framework) that copies those targets into
the framework.

When CMake installs a framework, it copies over the content of the
framework directory and "installs" the associated target. In addition to
copying the target, installing also means updating the RPATH. However,
the RPATH is only updated for the target associated with the framework.
Everything else is naively copied over, including executables or
libraries that have a different build and install RPATH. This means that
those tools need to have their own install rules.

If the framework is installed
first, the aforementioned tools are copied over with build RPATHs from
the build tree. And when the tools themselves are installed, the
binaries get overwritten and the RPATHs updated to the install RPATHs.

The problem is that CMake provides no guarantees when it comes to the
order in which components get installed. If those tools get installed
first, the inverse happens and the binaries get overwritten with the
ones that have build RPATHs.

lldb_add_to_buildtree_lldb_framework has a comment correctly stating
that those copied binaries should be removed before install.
Unfortunately, there's nothing in place today to do that. This patch
adds a custom target (lldb-framework-cleanup) that must will be run
before the install phase of LLDB.framework.

[bulbazord]

I think this idea will work but I have a few comments and questions:

Based on my understanding of this change, we're supposed to manually run the build system with the target lldb-framework-cleanup before we perform an install. Is this the case? This seems most useful when you have a script that controls the build process (e.g. swift's build-script).
I suppose this doesn't really change anything if you don't know about this new target, but it would be useful to document this somewhere (perhaps in docs/resources/build.rst?)

Making this a dependency of the install target is not sufficient as the target needs to be build before the install phase.

I can see why making the install target depend on lldb-framework-cleanup is not good, but what about making it a dependency of the target that installs the framework? I think it's something like install-liblldb? You could then guarantee that the cleanup happens before we install the framework. Maybe this is still too naïve or brittle though, what do you think?

[mib]

lldb/cmake/modules/AddLLDB.cmake

247–249

The comment here is confusing to me. May be you can try to re-phrase it.

[JDevlieghere]

In D141021#4037840, @bulbazord wrote:

I think this idea will work but I have a few comments and questions:

Based on my understanding of this change, we're supposed to manually run the build system with the target lldb-framework-cleanup before we perform an install. Is this the case? This seems most useful when you have a script that controls the build process (e.g. swift's build-script).
I suppose this doesn't really change anything if you don't know about this new target, but it would be useful to document this somewhere (perhaps in docs/resources/build.rst?)

Making this a dependency of the install target is not sufficient as the target needs to be build before the install phase.

I can see why making the install target depend on lldb-framework-cleanup is not good, but what about making it a dependency of the target that installs the framework? I think it's something like install-liblldb? You could then guarantee that the cleanup happens before we install the framework. Maybe this is still too naïve or brittle though, what do you think?

Interesting, I tried the exact thing that you described and my custom command wasn't run, but I cannot reproduce that anymore. The only reason I tried again was so I could say: "see, this doesn't work, it won't get executed", but then it did...

I don't know what I did differently then, but as this seems to work and is clearly more desirable, I'll update the patch.

[JDevlieghere]

Make lldb-framework-cleanup a dependency of install-liblldb.

[bulbazord]

lldb/source/API/CMakeLists.txt

215–216

I think you need to add it as a dependency of install-liblldb-stripped as well?

[JDevlieghere]

Add dependency to install-liblldb-stripped.

[labath]

Does this mean that the in-tree lldb will cease to be functional once someone "installs" it? (at least until the next rebuild)

[bulbazord]

In D141021#4043411, @labath wrote:

Does this mean that the in-tree lldb will cease to be functional once someone "installs" it? (at least until the next rebuild)

I believe so, but only in the case where you are building the LLDB framework on a macOS machine. I don't think this is ideal behavior but I personally think the tradeoff is worth it since you have deterministic installations behavior w.r.t. RPATHs.

[labath]

I don't dispute the value of determinism, but it seems like there ought to be a way to achieve this with corrupting the build tree (which in itself is not very 'deterministic').
What if we had two copies of the framework in the build-tree? One pristine copy, which would only contain liblldb (and any stuff that cmake puts there by default), and then another one which would be used for running, and which would contain all of the manually added stuff (with the right rpaths and all). When installing, one would use the pristine copy as the source instead of the adulterated one.

lldb/cmake/modules/AddLLDB.cmake

function(lldb_add_to_buildtree_lldb_framework name subdir)
   # not exist yet, it will exist when the generator expression gets expanded.
   set(copy_dest "${LLDB_FRAMEWORK_ABSOLUTE_BUILD_DIR}/${subdir}/$<TARGET_FILE_NAME:${name}>")
 
   # Copy into the given subdirectory for testing.
   add_custom_command(TARGET ${name} POST_BUILD
     COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:${name}> ${copy_dest}
     COMMENT "Copy ${name} to ${copy_dest}"
   )
+
+  # Create a custom target to remove the copy again from LLDB.framework in the
+  # build tree.
+  # Intentionally use remove_directory because the target can be a either a
+  # file or directory and using remove_directory is harmless for files.
+  add_custom_target(${name}-cleanup
+    COMMAND ${CMAKE_COMMAND} -E remove_directory ${copy_dest}
+    COMMENT "Removing ${name} from LLDB.framework")
+  add_dependencies(lldb-framework-cleanup
+    ${name}-cleanup)
 endfunction()
 
 # Add extra install steps for dSYM creation and stripping for the given target.
 function(lldb_add_post_install_steps_darwin name install_prefix)
   if(NOT APPLE)
     message(WARNING "Darwin-specific functionality; not currently available on non-Apple platforms.")
     return()
   endif()

lldb/cmake/modules/LLDBConfig.cmake

if(LLDB_BUILD_FRAMEWORK)
   set(LLDB_FRAMEWORK_INSTALL_DIR Library/Frameworks CACHE STRING "Install directory for LLDB.framework")
 
   get_filename_component(LLDB_FRAMEWORK_ABSOLUTE_BUILD_DIR ${LLDB_FRAMEWORK_BUILD_DIR} ABSOLUTE
     BASE_DIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR})
 
   # Essentially, emit the framework's dSYM outside of the framework directory.
   set(LLDB_DEBUGINFO_INSTALL_PREFIX ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin CACHE STRING
       "Directory to emit dSYM files stripped from executables and libraries (Darwin Only)")
+
+  # Custom target to remove the targets (binaries, directories) that were
+  # copied into LLDB.framework in the build tree.
+  #
+  # These targets need to be removed before the install phase because otherwise
+  # because otherwise they may overwrite already installed binaries with the
+  # wrong RPATH (i.e. build RPATH instead of install RPATH).
+  #
+  # This target needs to be created here (rather than in API/CMakeLists.txt)
+  # because add_lldb_tool creates the custom rules to copy the binaries before
+  # the framework target exists and that's the only place where this is
+  # tracked.
+  add_custom_target(lldb-framework-cleanup
+    COMMENT "Cleaning up build-tree frameworks in preparation for install")
 endif()
 
 if(APPLE AND CMAKE_GENERATOR STREQUAL Xcode)
   if(NOT LLDB_EXPLICIT_XCODE_CACHE_USED)
     message(WARNING
       "When building with Xcode, we recommend using the corresponding cache script. "
       "If this was a mistake, clean your build directory and re-run CMake with:\n"
       "  -C ${CMAKE_SOURCE_DIR}/cmake/caches/Apple-lldb-Xcode.cmake\n"

lldb/source/API/CMakeLists.txt

add_custom_command(TARGET liblldb POST_BUILD
       COMMAND ${CMAKE_COMMAND} -E create_symlink "${LLDB_EXTERNAL_CLANG_RESOURCE_DIR}"
               "${LLDB_CLANG_RESOURCE_DIR_PARENT}/${LLDB_CLANG_RESOURCE_DIR_NAME}"
     )
   endif()
 endif()
 
 if(LLDB_BUILD_FRAMEWORK)
   include(LLDBFramework)
+
+  add_dependencies(install-liblldb
+    lldb-framework-cleanup)
+  add_dependencies(install-liblldb-stripped
+    lldb-framework-cleanup)
 endif()