Introduce lldb-framework CMake target and centralize its logic

[xiaobai]

Summary

In this patch I aim to do the following:

1. Create an lldb-framework target that acts as the target that handles generating LLDB.framework. Previously, liblldb acted as the target for generating the framework in addition to generating the actual lldb library. This made the target feel overloaded.
2. Centralize framework generation as much as it makes sense to do so.
3. Create a target lldb-suite, which depends on every tool and library that makes liblldb fully functional. One result of having this target is it makes tracking dependencies much clearer.

[labath]

I like the idea of centralizing the framework code as much as possible. However, I am not sure how the dependency management fits into this. More details in inline comments.

CMakeLists.txt

46

LLDB.framework can only be generated when targeting Apple platforms avoids double negation

cmake/modules/LLDBFramework.cmake

2

Maybe use early exit here?

46

Maybe lldb-argdumper and lldb-server dependencies should be handled as a part of INCLUDE_IN_FRAMEWORK argument to add_lldb_executable? Or do you have other plans for that?

47–48

It seems a bit weird for a target to manage it's own outgoing dependencies. Maybe you could define another dummy target here (liblldb-suite?) which would depend on lldb-framework or liblldb depending on the build type. Then lldb and finish_swig could just always depend on that.

[xiaobai]

cmake/modules/LLDBFramework.cmake

2

Sounds good

46

One of the goals I had in centralizing the framework generation code would be to centralize and make explicit which tools went into the framework. The idea I had was to get rid of the INCLUDE_IN_FRAMEWORK argument to add_lldb_executable. Since add_lldb_executable builds the binary differently when building for the framework (modifying the rpath, changing the output destination and symlinking it to your build tree's bin dir), it should be sufficient just to check for LLDB_BUILD_FRAMEWORK.

What do you think of this?

47–48

I think that dummy target is a great idea! I was playing around with that idea, but I never thought of as succinctly as you put it. This should make dependency management a bit cleaner.

Oh and liblldb-suite is a great name. I had trouble thinking of a good name for a target like that.

[labath]

cmake/modules/LLDBFramework.cmake

46

Both of the approaches sound reasonable to me. If you want to go this way, then I'm fine with that.

[xiaobai]

cmake/modules/LLDBFramework.cmake

46

I've begun refactoring to remove INCLUDE_IN_FRAMEWORK but I've started thinking about some possibly negative repercussions. I'm wondering what you think of this:

add_lldb_tool (which invokes add_lldb_executable) can be used to add lldb executables that won't be included in the framework AFAICT (e.g. lldb-mi). What I was going to do was remove INCLUDE_IN_FRAMEWORK option so that every tool will get put into the framework and symlinked to $BUILD_DIR/bin when you run cmake with LLDB_BUILD_FRAMEWORK=1. This will mean that lldb-mi will be put in the framework if you do something like make lldb-mi after configuring with LLDB_BUILD_FRAMEWORK=1.

In that case, do you think it makes sense to keep INCLUDE_IN_FRAMEWORK as an option and use that to build part of the dependency tree for the lldb-framework target? Or do you think this is an unlikely enough example that it shouldn't matter?

[xiaobai]

Updated based on feedback. I would like to get more feedback before I'm comfortable changing it further and/or committing this.

[xiaobai]

Minor change I forgot to make

[compnerd]

CMakeLists.txt

146

Shouldn't this be else()?

176

"finish swig"?

cmake/modules/LLDBFramework.cmake

6

Globs are generally frowned upon for dependency tracking issues.

[xiaobai]

CMakeLists.txt

146

Yup

176

Should be "finish_swig"

cmake/modules/LLDBFramework.cmake

6

I see. This code was moved from somewhere else and written by somebody else. My CMake is somewhat weak, how would you do this instead?

[xiaobai]

Minor fixups pointed out by compnerd

[clayborg]

I'll defer to the cmake experts. I hope we can produce a LLDB.framework that is identical to the Xcode produced version, or at least as close.

[labath]

CMakeLists.txt

141

fully-functioning lldb library.

(this is just to emphasize that it does not include the driver executable with the same name)

155

I would move this line to the CMakeLilsts.txt which defines the lldb driver target.

180–186

Could we replace this by a single finish_swig -> lldb-suite dependency ?

cmake/modules/LLDBFramework.cmake

46

I think this is an important issue. We shouldn't have our install artifacts depend on what other targets the user happened to build. And it's not just the user typing "make lldb-mi" thats the problem. He can just decide to type "make" to build all targets, and then he'll end up with the same issue.

I think it still may be possible to have framework management centralized if you would make the list of targets that go into the framework explicit here. Something like:

foreach(target in ${TOOLS_THAT_GO_INTO_THE_FRAMEWORK})
  do_whatever_add_lldb_tool_would_have_done()
endforeach()

But, as I said, I think that keeping this part of the logic inside add_lldb_tool is fine too. If you decide to keep INCLUDE_IN_FRAMEWORK arg, then I think it is natural for the dependency management to be done there too. (Basically, I want to avoid the knowledge of what tools go into the framework to be defined in more than one place).

[xiaobai]

CMakeLists.txt

141

Thanks for pointing that out.

155

That makes sense to me.

180–186

I didn't think about it at the time but that makes the most sense

cmake/modules/LLDBFramework.cmake

46

I'm going to attempt to make centralization work just because I would like to avoid knowledge of what goes into the framework scattered. Thanks for the help.

[xiaobai]

Pavel's suggestions.

[xiaobai]

I think this is in a pretty good state. I built LLDB.framework with xcodebuild and compared it. There are still some discrepancies but this brings us a step closer to a final working solution.

cmake/modules/LLDBFramework.cmake

46

Decided to keep INCLUDE_IN_FRAMEWORK as removing it would introduce more boilerplate and wrapping that works around the problem instead of fixing it.

[labath]

This is looking very good, I just have one more quick question. I am wondering whether we couldn't simplify this dependency management a tad bit more. Would the thing I'm proposing below work for you?

CMakeLists.txt

147–153

This code here.

cmake/modules/AddLLDB.cmake

102–104

If you reorder this slightly, then you should be able to get rid of the extra dependency code in main CMakeLists.txt.
Something like this:

if (ARG_INCLUDE_IN_FRAMEWORK)
  add_dependencies(lldb-suite ${name})
  if (LLDB_BUILD_FRAMEWORK)
    ...

(of course, then the INCLUDE_IN_FRAMEWORK arg will become misnamed, but that can be solved by renaming it to INCLUDE_IN_SUITE)

[xiaobai]

This comment has been deleted.

cmake/modules/AddLLDB.cmake

102–104

This is a pretty good idea imo. I think it'll make the dependency tracking much easier. The lldb-framework target then becomes a small target.

[xiaobai]

Labath's suggestion

[labath]

Looks good. I'm really happy with how this turned out. Thank you for your patience.

[clayborg]

Indeed!!! So happy to see this work getting done. Great stuff. Will be great to not have to maintain the Xcode project as some point in the near future.

[xiaobai]

Thank you @labath for your help. I'm much happier with this!

CMakeLists.txt

 else()
   find_package(LibEdit REQUIRED)
 endif()
 
 if(APPLE)
   add_definitions(-DLLDB_USE_OS_LOG)
 endif()
 
+option(LLDB_BUILD_FRAMEWORK "Build the Darwin LLDB.framework" Off)
 if(LLDB_BUILD_FRAMEWORK)
+  if (CMAKE_VERSION VERSION_LESS 3.7)
+    message(FATAL_ERROR "LLDB_BUILD_FRAMEWORK is not supported on CMake < 3.7")
+  endif()
+  if (NOT APPLE)
+    message(FATAL_ERROR "LLDB.framework can only be generated when targeting Apple platforms")
+  endif()
+  add_custom_target(lldb-framework)
   set(LLDB_FRAMEWORK_DIR
     ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/${LLDB_FRAMEWORK_INSTALL_DIR})
 endif()
 
 add_subdirectory(docs)
 if (NOT LLDB_DISABLE_PYTHON)
   if(LLDB_USE_SYSTEM_SIX)
     set(SIX_EXTRA_ARGS "--useSystemSix")
if(LLDB_INCLUDE_TESTS)
   endif()
 
   add_subdirectory(test)
   add_subdirectory(unittests)
   add_subdirectory(lit)
   add_subdirectory(utils/lldb-dotest)
 endif()
 
+# lldb-suite is a dummy target that encompasses all the necessary tools and
+# libraries for building a fully-functioning liblldb.
+add_custom_target(lldb-suite)
+if (LLDB_BUILD_FRAMEWORK)
+  include(LLDBFramework)
+  add_dependencies(lldb-suite lldb-framework)
+else()
+  if (LLDB_CAN_USE_LLDB_SERVER)
+    add_dependencies(lldb-suite lldb-server)
+  endif()
+  if (LLDB_CAN_USE_DEBUGSERVER)
+    add_dependencies(lldb-suite debugserver)
+  endif()
+  add_dependencies(lldb-suite liblldb lldb-argdumper)
+endif()
+
 if (NOT LLDB_DISABLE_PYTHON)
     # Add a Post-Build Event to copy over Python files and create the symlink
     # to liblldb.so for the Python API(hardlink on Windows)
     add_custom_target(finish_swig ALL
         COMMAND
            ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/scripts/finishSwigWrapperClasses.py
                --srcRoot=${LLDB_SOURCE_DIR}
                --targetDir=${LLDB_PYTHON_TARGET_DIR}
                --cfgBldDir=${LLDB_PYTHON_TARGET_DIR}
                --prefix=${CMAKE_BINARY_DIR}
                --cmakeBuildConfiguration=${CMAKE_CFG_INTDIR}
                --lldbLibDir=lib${LLVM_LIBDIR_SUFFIX}
                ${SIX_EXTRA_ARGS}
                ${FINISH_EXTRA_ARGS}
         VERBATIM
         DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/scripts/finishSwigWrapperClasses.py
         DEPENDS ${LLDB_PYTHON_TARGET_DIR}/lldb.py
         COMMENT "Python script sym-linking LLDB Python API")
-    # We depend on liblldb being built before we can do this step.
-    add_dependencies(finish_swig liblldb lldb-argdumper)
+
+    # We depend on liblldb and lldb-argdumper being built before we can do this step.
+    add_dependencies(finish_swig lldb-suite)
 
     # If we build the readline module, we depend on that happening
     # first.
     if (TARGET readline)
         add_dependencies(finish_swig readline)
     endif()
 
     # Ensure we do the python post-build step when building lldb.
     add_dependencies(lldb finish_swig)
 
-    if(LLDB_BUILD_FRAMEWORK)
-      # The target to install libLLDB needs to depend on finish swig so that the
+    if (LLDB_BUILD_FRAMEWORK)
+      # The target to install libLLDB needs to depend on finish_swig so that the
       # framework build properly copies over the Python files.
       add_dependencies(install-liblldb finish_swig)
     endif()
 
     # Add a Post-Build Event to copy the custom Python DLL to the lldb binaries dir so that Windows can find it when launching
     # lldb.exe or any other executables that were linked with liblldb.
     if (WIN32 AND NOT "${PYTHON_DLL}" STREQUAL "")
         # When using the Visual Studio CMake generator the lldb binaries end up in Release/bin, Debug/bin etc.

cmake/modules/AddLLDB.cmake

function(add_lldb_executable name)
 
   list(APPEND LLVM_LINK_COMPONENTS ${ARG_LINK_COMPONENTS})
   add_llvm_executable(${name} ${ARG_UNPARSED_ARGUMENTS})
 
   target_link_libraries(${name} PRIVATE ${ARG_LINK_LIBS})
   set_target_properties(${name} PROPERTIES
     FOLDER "lldb executables")
 
   if(LLDB_BUILD_FRAMEWORK)
     if(ARG_INCLUDE_IN_FRAMEWORK)
+      add_dependencies(lldb-framework ${name})
       if(NOT IOS)
         set(resource_dir "/Resources")
         set(resource_dots "../")
       endif()
       string(REGEX REPLACE "[^/]+" ".." _dots ${LLDB_FRAMEWORK_INSTALL_DIR})
       set_target_properties(${name} PROPERTIES
             RUNTIME_OUTPUT_DIRECTORY $<TARGET_FILE_DIR:liblldb>${resource_dir}
             BUILD_WITH_INSTALL_RPATH On

cmake/modules/LLDBFramework.cmake

+file(GLOB public_headers ${LLDB_SOURCE_DIR}/include/lldb/API/*.h)
+file(GLOB root_public_headers ${LLDB_SOURCE_DIR}/include/lldb/lldb-*.h)
+file(GLOB root_private_headers ${LLDB_SOURCE_DIR}/include/lldb/lldb-private*.h)
+list(REMOVE_ITEM root_public_headers ${root_private_headers})
+foreach(header
+    ${public_headers}
+    ${root_public_headers}
+    ${LLDB_SOURCE_DIR}/include/lldb/Utility/SharingPtr.h)
+  get_filename_component(basename ${header} NAME)
+  add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders/${basename}
+                     DEPENDS ${header}
+                     COMMAND ${CMAKE_COMMAND} -E copy ${header} ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders/${basename})
+  list(APPEND framework_headers ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders/${basename})
+endforeach()
+add_custom_target(lldb-framework-headers
+  DEPENDS ${framework_headers}
+  COMMAND ${LLDB_SOURCE_DIR}/scripts/framework-header-fix.sh ${CMAKE_CURRENT_BINARY_DIR} ${LLDB_VERSION})
+
+if (NOT IOS)
+  if (NOT LLDB_BUILT_STANDALONE)
+    add_dependencies(lldb-framework clang-headers)
+  endif()
+  add_custom_command(TARGET lldb-framework POST_BUILD
+    COMMAND ${CMAKE_COMMAND} -E copy_directory ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders $<TARGET_FILE_DIR:liblldb>/Headers
+    COMMAND ${CMAKE_COMMAND} -E create_symlink Versions/Current/Headers ${CMAKE_BINARY_DIR}/${LLDB_FRAMEWORK_INSTALL_DIR}/LLDB.framework/Headers
+    COMMAND ${CMAKE_COMMAND} -E create_symlink ${LLDB_FRAMEWORK_VERSION} ${CMAKE_BINARY_DIR}/${LLDB_FRAMEWORK_INSTALL_DIR}/LLDB.framework/Versions/Current
+    COMMAND ${CMAKE_COMMAND} -E copy_directory ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX}/clang/${LLDB_VERSION} $<TARGET_FILE_DIR:liblldb>/Resources/Clang
+  )
+else()
+  add_custom_command(TARGET lldb-framework POST_BUILD
+    COMMAND ${CMAKE_COMMAND} -E copy_directory ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders $<TARGET_FILE_DIR:liblldb>/Headers
+  )
+endif()
+
+set_target_properties(liblldb PROPERTIES
+  OUTPUT_NAME LLDB
+  FRAMEWORK On
+  FRAMEWORK_VERSION ${LLDB_FRAMEWORK_VERSION}
+  LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LLDB_FRAMEWORK_INSTALL_DIR}
+  PUBLIC_HEADER "${framework_headers}")
+
+add_dependencies(lldb-framework
+  liblldb
+  lldb-framework-headers)

source/API/CMakeLists.txt

 if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
   add_definitions( -DEXPORT_LIBLLDB )
 endif()
 
-option(LLDB_BUILD_FRAMEWORK "Build the Darwin LLDB.framework" Off)
-
-if(LLDB_BUILD_FRAMEWORK AND CMAKE_VERSION VERSION_LESS 3.7)
-  message(FATAL_ERROR "LLDB_BUILD_FRAMEWORK is not supported on CMake < 3.7")
-endif()
-
-if (LLDB_BUILD_FRAMEWORK AND NOT APPLE)
-  message(FATAL_ERROR "LLDB.framework cannot be generated unless targeting Apple platforms.")
-endif()
-
 get_property(LLDB_ALL_PLUGINS GLOBAL PROPERTY LLDB_PLUGINS)
 
 add_lldb_library(liblldb SHARED
   SBAddress.cpp
   SBAttachInfo.cpp
   SBBlock.cpp
   SBBreakpoint.cpp
   SBBreakpointLocation.cpp
set_target_properties(liblldb
     PROPERTIES
     OUTPUT_NAME lldb
     )
 endif()
 
 if (LLDB_WRAP_PYTHON)
   add_dependencies(liblldb swig_wrapper)
 endif()
-
-if(LLDB_BUILD_FRAMEWORK)
-  file(GLOB public_headers ${LLDB_SOURCE_DIR}/include/lldb/API/*.h)
-  file(GLOB root_public_headers ${LLDB_SOURCE_DIR}/include/lldb/lldb-*.h)
-  file(GLOB root_private_headers ${LLDB_SOURCE_DIR}/include/lldb/lldb-private*.h)
-  list(REMOVE_ITEM root_public_headers ${root_private_headers})
-
-  foreach(header ${public_headers} ${root_public_headers} ${LLDB_SOURCE_DIR}/include/lldb/Utility/SharingPtr.h)
-    get_filename_component(basename ${header} NAME)
-    add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders/${basename}
-                       DEPENDS ${header}
-                       COMMAND ${CMAKE_COMMAND} -E copy ${header} ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders/${basename})
-    list(APPEND framework_headers ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders/${basename})
-  endforeach()
-
-  add_custom_target(lldb-framework-headers DEPENDS ${framework_headers}
-                    COMMAND ${LLDB_SOURCE_DIR}/scripts/framework-header-fix.sh ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders ${LLDB_VERSION})
-  add_dependencies(liblldb lldb-framework-headers)
-
-  set_target_properties(liblldb PROPERTIES
-    OUTPUT_NAME LLDB
-    FRAMEWORK On
-    FRAMEWORK_VERSION ${LLDB_FRAMEWORK_VERSION}
-    LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LLDB_FRAMEWORK_INSTALL_DIR}
-    PUBLIC_HEADER "${framework_headers}")
-
-  if(NOT IOS)
-    if (NOT LLDB_BUILT_STANDALONE)
-      add_dependencies(liblldb clang-headers)
-    endif()
-    add_custom_command(TARGET liblldb POST_BUILD
-      COMMAND ${CMAKE_COMMAND} -E copy_directory ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders $<TARGET_FILE_DIR:liblldb>/Headers
-      COMMAND ${CMAKE_COMMAND} -E create_symlink Versions/Current/Headers ${CMAKE_BINARY_DIR}/${LLDB_FRAMEWORK_INSTALL_DIR}/LLDB.framework/Headers
-      COMMAND ${CMAKE_COMMAND} -E create_symlink ${LLDB_FRAMEWORK_VERSION} ${CMAKE_BINARY_DIR}/${LLDB_FRAMEWORK_INSTALL_DIR}/LLDB.framework/Versions/Current
-      COMMAND ${CMAKE_COMMAND} -E copy_directory ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX}/clang/${LLDB_VERSION} $<TARGET_FILE_DIR:liblldb>/Resources/Clang
-      )
-  else()
-    add_custom_command(TARGET liblldb POST_BUILD
-      COMMAND ${CMAKE_COMMAND} -E copy_directory ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders $<TARGET_FILE_DIR:liblldb>/Headers
-      )
-  endif()
-
-endif()
-

tools/driver/CMakeLists.txt

add_lldb_tool(lldb
   LINK_COMPONENTS
     Support
   )
 
 if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
   add_definitions( -DIMPORT_LIBLLDB )
 endif()
 
-# Add lldb dependency on lldb-server if we can use it.
-if ( LLDB_CAN_USE_LLDB_SERVER )
-  add_dependencies(lldb lldb-server)
-endif()
-
-# Add lldb dependency on debugserver if we can use it.
-if ( LLDB_CAN_USE_DEBUGSERVER )
-  add_dependencies(lldb debugserver)
-endif()
+add_dependencies(lldb lldb-suite)