[lldb] Refactor all POST_BUILD commands into targets

[hhb]

Summary

This makes all dependencies correct.

[labath]

This looks reasonable to me. I'm just wondering, now that these are separate targets, I guess that means they can be run in random order, right? Will that cause any problems, for instance when creating a package and its subpackage (formatters and formatters/cpp maybe)?

lldb/CMakeLists.txt

137

Would it be possible to just make the swig step place this file into the correct place directly?

[hhb]

In D69589#1726812, @labath wrote:

This looks reasonable to me. I'm just wondering, now that these are separate targets, I guess that means they can be run in random order, right? Will that cause any problems, for instance when creating a package and its subpackage (formatters and formatters/cpp maybe)?

I try to avoid that problem by creating directory every time. Other than that I don't think there's any dependencies between two subpackages...

But in general, yes, that's something we should be careful of...

lldb/CMakeLists.txt

137

The difficulty is that, there is a config time path dependency like this:

swig_wrapper -> liblldb -> finish_swig (renamed to lldb_python_packages in this change)

Where liblldb uses the path of LLDBWrapPython.cpp in swig_wrapper. And here we reference the path of liblldb, to calculate the right path for python packages.

That means when swig_wrapper is created, we don't have a good way to figure out the right path for python packages and put the file there directly.

The best way to solve this is to hard code liblldb path for framework build. I.e. replace line 92 here:

get_target_property(liblldb_build_dir liblldb LIBRARY_OUTPUT_DIRECTORY)

With something more constant. This removes the liblldb -> finish_swig dependency. Then all the code here can be moved to scripts/CMakeLists.txt (I think they belong there better anyway). And swig_wrapper can get access to python package path.

The only question is whether we can / should "predict" liblldb path for framework... I'll look more into this. But it probably deserves a separate change.

[hhb]

When creating symlink, make the target depends on relative target path. So that if the target file doesn't exist, the build will fail.

[labath]

This looks great to me. Maybe wait a while to see if anyone else has any thoughts on this?

This not your fault, but the thing that bothers me about this build process (and which this patch makes visible) is how the locations of the python files in source bear absolutely no resemblance to the locations the files will end up installed. For instance, I would never have expected that something from the "examples" folder ends up in an actual shippable artifact. I think it would be great if we could rearrange the sources so that their source layout roughly matches the way in which they are going to be used...

lldb/CMakeLists.txt

137

Ok, that sounds fair. I think it would be great if this stuff could be moved to the scripts folder.

[aadsm]

I'm sorry to reject but this diff breaks install-distribution on Darwin. Here's how to repro:

$ mkdir -p ~/Projects/llvm-build/Release && cd ~/Projects/llvm-build/Release
$ cmake -G Ninja \
 -DLLVM_ENABLE_PROJECTS="clang;lldb" \
 -DCMAKE_BUILD_TYPE=Release \
 -DLLVM_ENABLE_ASSERTIONS=OFF \
 -DLLDB_INCLUDE_TESTS=OFF \
 -DLLVM_DISTRIBUTION_COMPONENTS="lldb;liblldb" \
 -C ../../llvm-project/lldb/cmake/caches/Apple-lldb-macOS.cmake \
 -S../../llvm-project/llvm
$ DESTDIR=$PWD/install/usr ninja install-distribution
ninja: error: 'bin/LLDB.framework/LLDB', needed by 'bin/LLDB.framework/Resources/Python/lldb/_lldb.so', missing and no known rule to make it

The reason I found this is because I was having an issue where _lldb.so was being generated on the build but it was not being copied to the install directory.
After some mucking around I realized that the install-liblldb target installs the python scripts but it's lldb that generates the symlink as a POST_BUILD command (before this diff). I think in some situations the install-liblldb runs before that POST_BUILD command has run. At least I'm able to repro this quite consistently and once ninja install-distribution is finished _lldb.so exists on the build directory but not on the install one (and I can see all files in the python module being copied except that one in the ninja log when running the install step).

I fixed this locally by doing this (this fix doesn't really feel good but not sure what's the best way to do it):

if(LLDB_BUILD_FRAMEWORK)
  # When building as framework liblldb is responsible to install the python
  # scripts. However, we need to make sure that all swig components have been
  # properly finished.
  add_dependencies(install-liblldb finish_swig)
  add_dependencies(install-liblldb-stripped finish_swig)
else()
  # Install the LLDB python module
  ...
endif()

Then I found this diff that hasn't landed yet and tried it (I think there's a race condition caused by the POST_BUILD command but not sure) out and stumbled upon this.

[hhb]

In D69589#1731782, @aadsm wrote:

I fixed this locally by doing this (this fix doesn't really feel good but not sure what's the best way to do it):

I found it confusing too. So finish_swig should depends on liblldb, because at least for windows, liblldb has to be exist before copying to python package path. And lldb correctly depends on both of them.

Then install-liblldb depends on liblldb. Which doesn't guarantee the build of finish_swig... Indeed your change may be the only way out...

This issue should be there before all recent changes. I'm not sure why it works before. Maybe by luck?

Do you want to upload a patch?

[hhb]

Alternatively we should not depends on install-liblldb to install python packages, but have a install command similar to what we did below for other platforms.

[hhb]

See D68370. We probably need to do the same thing for darwin...

[hhb]

Sent D69834. Haven't get a chance to test but you can see the idea...

Also please add lldb-python-scripts to your LLVM_DISTRIBUTION_COMPONENTS when testing.

[aadsm]

This is great! it's so much better. Thank so much for looking into this and fixing it.

[aadsm]

@hhb fwiw, I still get the same error with this diff (after applying it on top of D69834). But D69834 by itself works great!

[hhb]

Rebase

[hhb]

Fix some naming

[hhb]

In D69589#1733567, @aadsm wrote:

@hhb fwiw, I still get the same error with this diff (after applying it on top of D69834). But D69834 by itself works great!

Hmmm this change should not break it again... I rebased and cleaned up a little. Can you help try again?

[aadsm]

@hhb I just did, and get the same error as well :(
ninja: error: 'bin/LLDB.framework/LLDB', needed by 'bin/LLDB.framework/Resources/Python/lldb/_lldb.so', missing and no known rule to make it

[hhb]

In D69589#1734818, @aadsm wrote:

@hhb I just did, and get the same error as well :(
ninja: error: 'bin/LLDB.framework/LLDB', needed by 'bin/LLDB.framework/Resources/Python/lldb/_lldb.so', missing and no known rule to make it

Thanks for the testing. It is interesting and I can reproduce now by build, remove LLDB.framework and build again.

LLDB.framework/LLDB symlink is not created by any target I can find. I tried liblldb or lldb. Seems only a full build will create it? I'm not familiar with framework build. Will spend some time to look into it.

[aadsm]

Yeah, weird. I wonder if this is something cmake does implicitly when the target is defined as FRAMEWORK: https://cmake.org/cmake/help/latest/prop_tgt/FRAMEWORK.html

[sgraenitz]

I'd recommend to be careful when making a change of this size. There are a lot of possible configurations.
If you split it up into a few smaller and more dedicated commits, you will make your life easier. Otherwise someone might just revert everything if this happens to break a bot.

In D69589#1736299, @aadsm wrote:

Yeah, weird. I wonder if this is something cmake does implicitly when the target is defined as FRAMEWORK: https://cmake.org/cmake/help/latest/prop_tgt/FRAMEWORK.html

This is correct. There is a comment mentioning it here:
https://github.com/llvm/llvm-project/blob/276a5b2d5f1f/lldb/cmake/modules/LLDBFramework.cmake#L44

lldb/CMakeLists.txt

204

This change e.g. is reasonable, but unrelated. It could go into a separate NFC commit.

[tatyana-krasnukha]

lldb/CMakeLists.txt

114

This is quite off-topic, but it would be great if this function could handle whole directories as well as separate files. So adding a script will not require to change this file.

[labath]

lldb/CMakeLists.txt

114

It isn't really off topic, but I don't think it's really feasible, or a good idea even :P.

Think of this as the list of cpp files in the cmake add_library calls. Though there are ways to avoid listing the files there (via some sort of globbing, generally), they usually come with their own problems. Either the glob is evaluated at configuration time, and then you have to remember to re-run cmake to pick up the newly added files; or the glob is done at build time, which means that even a noop build needs to reglob everything...

[JDevlieghere]

In D69589#1769663, @sgraenitz wrote:

I'd recommend to be careful when making a change of this size. There are a lot of possible configurations.
If you split it up into a few smaller and more dedicated commits, you will make your life easier. Otherwise someone might just revert everything if this happens to break a bot.

+1

Please include me as a reviewer when making changes like this to the build system, especially if it affects the framework build. It's only sparsely tested upstream (in part because we have a bunch of downstream changes) but it is the way we ship LLDB to our customers so it is really import to us. If I happen to miss the review I likely won't discover issues until the next time we rebranch at which point it might be a lot harder to figure them out.

[JDevlieghere]

Ha, I looked at the date but didn't realize this was a review from fall last year :-)

[hhb]

I gave up on this. 😊

lldb/CMakeLists.txt

if (NOT LLDB_DISABLE_PYTHON)
   if(LLDB_BUILD_FRAMEWORK)
     set(lldb_python_build_path "${liblldb_build_dir}/LLDB.framework/Resources/Python/lldb")
   else()
     set(lldb_python_build_path "${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/${LLDB_PYTHON_RELATIVE_PATH}/lldb")
   endif()
 
   # 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 VERBATIM
-    COMMAND ${CMAKE_COMMAND} -E make_directory ${lldb_python_build_path}
-    DEPENDS ${lldb_scripts_dir}/lldb.py
-    COMMENT "Python script sym-linking LLDB Python API")
+  add_custom_target(finish_swig ALL
+    COMMENT "Copy over Python files and create symlinks for LLDB Python API.")
+  set_target_properties(finish_swig PROPERTIES FOLDER "lldb misc")
+
+  function(add_copy_file_target name)
+    cmake_parse_arguments(ARG "" "DEST_DIR;DEST_FILE_NAME" "FILES" ${ARGN})
+    add_custom_command(OUTPUT ${ARG_DEST_DIR} VERBATIM
+      COMMAND ${CMAKE_COMMAND} -E make_directory ${ARG_DEST_DIR})
+    foreach(src_file ${ARG_FILES})
+      if(ARG_DEST_FILE_NAME)
+        set(file_name ${ARG_DEST_FILE_NAME})
+      else()
+        get_filename_component(file_name ${src_file} NAME)
+      endif()
+      set(dest_file ${ARG_DEST_DIR}/${file_name})
+      list(APPEND DEST_FILES ${dest_file})
+      add_custom_command(OUTPUT ${dest_file} VERBATIM
+        COMMAND ${CMAKE_COMMAND} -E copy ${src_file} ${dest_file}
+        DEPENDS ${ARG_DEST_DIR} ${src_file})
+    endforeach()
+    add_custom_target(${name} DEPENDS ${DEST_FILES} ${ARG_DEST_DIR})
+  endfunction()
 
   if(NOT LLDB_USE_SYSTEM_SIX)
-    add_custom_command(TARGET finish_swig POST_BUILD VERBATIM
-      COMMAND ${CMAKE_COMMAND} -E copy
-        "${LLDB_SOURCE_DIR}/third_party/Python/module/six/six.py"
-        "${lldb_python_build_path}/../six.py")
+    add_copy_file_target(lldb_python_six
+      FILES    "${LLDB_SOURCE_DIR}/third_party/Python/module/six/six.py"
+      DEST_DIR "${lldb_python_build_path}/..")
+    add_dependencies(finish_swig lldb_python_six)
   endif()
 
-  add_custom_command(TARGET finish_swig POST_BUILD VERBATIM
-    COMMAND ${CMAKE_COMMAND} -E copy
-      "${lldb_scripts_dir}/lldb.py"
-      "${lldb_python_build_path}/__init__.py")
+  add_copy_file_target(lldb_python_init
+    FILES          "${lldb_scripts_dir}/lldb.py"
+    DEST_DIR       "${lldb_python_build_path}"
+    DEST_FILE_NAME "__init__.py")
+  add_dependencies(finish_swig lldb_python_init)
 
   if(APPLE)
-    SET(lldb_python_heap_dir "${lldb_python_build_path}/macosx/heap")
-    add_custom_command(TARGET finish_swig POST_BUILD VERBATIM
-      COMMAND ${CMAKE_COMMAND} -E make_directory ${lldb_python_heap_dir}
-      COMMAND ${CMAKE_COMMAND} -E copy
-        "${LLDB_SOURCE_DIR}/examples/darwin/heap_find/heap/heap_find.cpp"
+    add_copy_file_target(lldb_python_heap
+      FILES    "${LLDB_SOURCE_DIR}/examples/darwin/heap_find/heap/heap_find.cpp"
                "${LLDB_SOURCE_DIR}/examples/darwin/heap_find/heap/Makefile"
-        ${lldb_python_heap_dir})
+      DEST_DIR "${lldb_python_build_path}/macosx/heap")
+    add_dependencies(finish_swig lldb_python_heap)
   endif()
 
-  function(create_python_package target pkg_dir)
+  add_copy_file_target(lldb_python_embeded_interpreter
+    FILES    "${LLDB_SOURCE_DIR}/source/Interpreter/embedded_interpreter.py"
+    DEST_DIR "${lldb_python_build_path}")
+  add_dependencies(finish_swig lldb_python_embeded_interpreter)
+
+  function(add_lldb_python_package_target name pkg_dir)
     cmake_parse_arguments(ARG "" "" "FILES" ${ARGN})
-    if(ARG_FILES)
-      set(copy_cmd COMMAND ${CMAKE_COMMAND} -E copy ${ARG_FILES} ${pkg_dir})
-    endif()
-    add_custom_command(TARGET ${target} POST_BUILD VERBATIM
-      COMMAND ${CMAKE_COMMAND} -E make_directory ${pkg_dir}
-      ${copy_cmd}
+    set(ABS_PKG_DIR "${lldb_python_build_path}/${pkg_dir}")
+    add_copy_file_target("${name}_srcs" FILES ${ARG_FILES} DEST_DIR ${ABS_PKG_DIR})
+    add_custom_command(OUTPUT "${ABS_PKG_DIR}/__init__.py" VERBATIM
       COMMAND ${PYTHON_EXECUTABLE} "${LLDB_SOURCE_DIR}/scripts/Python/createPythonInit.py"
         ${pkg_dir} ${ARG_FILES}
-      WORKING_DIRECTORY ${lldb_python_build_path})
+      WORKING_DIRECTORY ${lldb_python_build_path}
+      DEPENDS "${name}_srcs")
+    add_custom_target(${name} DEPENDS "${ABS_PKG_DIR}/__init__.py" "${name}_srcs")
   endfunction()
 
-  add_custom_command(TARGET finish_swig POST_BUILD VERBATIM
-    COMMAND ${CMAKE_COMMAND} -E copy
-      "${LLDB_SOURCE_DIR}/source/Interpreter/embedded_interpreter.py" ${lldb_python_build_path})
-
-  create_python_package(finish_swig "formatters/cpp"
+  add_lldb_python_package_target(lldb_python_formatters_cpp "formatters/cpp"
     FILES "${LLDB_SOURCE_DIR}/examples/synthetic/gnu_libstdcpp.py"
           "${LLDB_SOURCE_DIR}/examples/synthetic/libcxx.py")
+  add_dependencies(finish_swig lldb_python_formatters_cpp)
+
   # Make an empty __init__.py in lldb/runtime as this is required for
   # Python to recognize lldb.runtime as a valid package (and hence,
   # lldb.runtime.objc as a valid contained package)
-  create_python_package(finish_swig "runtime")
+  add_lldb_python_package_target(lldb_python_runtime "runtime")
+  add_dependencies(finish_swig lldb_python_runtime)
+
   # Having these files copied here ensure that lldb/formatters is a
   # valid package itself
-  create_python_package(finish_swig "formatters"
+  add_lldb_python_package_target(lldb_python_formatters "formatters"
     FILES "${LLDB_SOURCE_DIR}/examples/summaries/cocoa/cache.py"
           "${LLDB_SOURCE_DIR}/examples/summaries/synth.py"
           "${LLDB_SOURCE_DIR}/examples/summaries/cocoa/metrics.py"
           "${LLDB_SOURCE_DIR}/examples/summaries/cocoa/attrib_fromdict.py"
           "${LLDB_SOURCE_DIR}/examples/summaries/cocoa/Logger.py")
-  create_python_package(finish_swig "utils"
+  add_dependencies(finish_swig lldb_python_formatters)
+
+  add_lldb_python_package_target(lldb_python_utils "utils"
     FILES "${LLDB_SOURCE_DIR}/examples/python/symbolication.py")
+  add_dependencies(finish_swig lldb_python_utils)
+
   if(APPLE)
-    create_python_package(finish_swig "macosx"
+    add_lldb_python_package_target(lldb_python_macosx "macosx"
       FILES "${LLDB_SOURCE_DIR}/examples/python/crashlog.py"
             "${LLDB_SOURCE_DIR}/examples/darwin/heap_find/heap.py")
-    create_python_package(finish_swig "diagnose"
+    add_dependencies(finish_swig lldb_python_macosx)
+
+    add_lldb_python_package_target(lldb_python_diagnose "diagnose"
       FILES "${LLDB_SOURCE_DIR}/examples/python/diagnose_unwind.py"
             "${LLDB_SOURCE_DIR}/examples/python/diagnose_nsstring.py")
+    add_dependencies(finish_swig lldb_python_diagnose)
   endif()
 
-  function(create_relative_symlink target dest_file output_dir output_name)
+  function(add_relative_symlink_target name dest_file output_dir output_name)
     get_filename_component(dest_file ${dest_file} ABSOLUTE)
     get_filename_component(output_dir ${output_dir} ABSOLUTE)
     file(RELATIVE_PATH rel_dest_file ${output_dir} ${dest_file})
     if(CMAKE_HOST_UNIX)
       set(LLVM_LINK_OR_COPY create_symlink)
     else()
       set(LLVM_LINK_OR_COPY copy)
     endif()
-    add_custom_command(TARGET ${target} POST_BUILD VERBATIM
+    add_custom_command(OUTPUT ${output_dir}/${output_name} DEPENDS ${output_dir}/${rel_dest_file}
       COMMAND ${CMAKE_COMMAND} -E ${LLVM_LINK_OR_COPY} ${rel_dest_file} ${output_name}
-      WORKING_DIRECTORY ${output_dir})
+      WORKING_DIRECTORY ${output_dir} VERBATIM)
+    add_custom_target(${name} DEPENDS ${output_dir}/${output_name})
   endfunction()
 
   if(LLDB_BUILD_FRAMEWORK)
     set(LIBLLDB_SYMLINK_DEST "${liblldb_build_dir}/LLDB.framework/LLDB")
   else()
     set(LIBLLDB_SYMLINK_DEST "${LLVM_SHLIB_OUTPUT_INTDIR}/liblldb${CMAKE_SHARED_LIBRARY_SUFFIX}")
   endif()
   if(WIN32)
     if(CMAKE_BUILD_TYPE STREQUAL Debug)
       set(LIBLLDB_SYMLINK_OUTPUT_FILE "_lldb_d.pyd")
     else()
       set(LIBLLDB_SYMLINK_OUTPUT_FILE "_lldb.pyd")
     endif()
   else()
     set(LIBLLDB_SYMLINK_OUTPUT_FILE "_lldb.so")
   endif()
-  create_relative_symlink(finish_swig ${LIBLLDB_SYMLINK_DEST}
+  add_relative_symlink_target(lldb_python_liblldb ${LIBLLDB_SYMLINK_DEST}
                               ${lldb_python_build_path} ${LIBLLDB_SYMLINK_OUTPUT_FILE})
+  add_dependencies(finish_swig lldb_python_liblldb)
 
   if(NOT LLDB_BUILD_FRAMEWORK)
     set(LLDB_ARGDUMPER_FILENAME "lldb-argdumper${CMAKE_EXECUTABLE_SUFFIX}")
-    create_relative_symlink(finish_swig "${LLVM_RUNTIME_OUTPUT_INTDIR}/${LLDB_ARGDUMPER_FILENAME}"
+    add_relative_symlink_target(lldb_python_argdumper
+                                "${LLVM_RUNTIME_OUTPUT_INTDIR}/${LLDB_ARGDUMPER_FILENAME}"
                                 ${lldb_python_build_path} ${LLDB_ARGDUMPER_FILENAME})
+    add_dependencies(finish_swig lldb_python_argdumper)
   endif()
 
-  add_dependencies(finish_swig swig_wrapper liblldb lldb-argdumper)
-  set_target_properties(finish_swig swig_wrapper PROPERTIES FOLDER "lldb misc")
-
   # Ensure we do the python post-build step when building lldb.
   add_dependencies(lldb finish_swig)
 
   # Install the LLDB python module
   if(LLDB_BUILD_FRAMEWORK)
     set(LLDB_PYTHON_INSTALL_PATH ${LLDB_FRAMEWORK_INSTALL_DIR}/LLDB.framework/Resources/Python)
   else()
     set(LLDB_PYTHON_INSTALL_PATH ${LLDB_PYTHON_RELATIVE_PATH})

lldb/scripts/CMakeLists.txt

COMMAND ${SWIG_EXECUTABLE}
       ${LLDB_SOURCE_DIR}/scripts/lldb.swig
   VERBATIM
   COMMENT "Builds LLDB Python wrapper")
 
 add_custom_target(swig_wrapper ALL DEPENDS
   ${CMAKE_CURRENT_BINARY_DIR}/LLDBWrapPython.cpp
   ${CMAKE_CURRENT_BINARY_DIR}/lldb.py
 )
+set_target_properties(swig_wrapper PROPERTIES FOLDER "lldb misc")