Make framework-header-fix process copied headers

[keith]

Summary

Previously the framework-header-fix script would change the sources
before they were copied, leading to unnecessary rebuilds on repeat
ninja lldb invocations. This runs the script on the headers after
they're copied into the produced LLDB.framework, meaning it doesn't
affect any files being built.

[keith]

It seems like if this was a common occurrence, it would've been fixed earlier, so I'm wondering if there's a difference in the way I'm building lldb that causes this. Using cmake:

cmake ../llvm -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=YES -DLLDB_EXPORT_ALL_SYMBOLS=1 -DLLDB_BUILD_FRAMEWORK=ON

And then ninja lldb, repeat ninja lldb builds causes ~500 files to be rebuilt with no source changes. I've diffed the LLDB.framework before and after and there don't seem to be any differences in the produced headers.

[labath]

Adding Alex, as he was doing a lot of framework work lately.

[xiaobai]

If I understand correctly, this is putting the headers directly into the framework? That's a pretty good idea :D

In D49779#1174659, @keith wrote:

It seems like if this was a common occurrence, it would've been fixed earlier, so I'm wondering if there's a difference in the way I'm building lldb that causes this. Using cmake:

cmake ../llvm -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=YES -DLLDB_EXPORT_ALL_SYMBOLS=1 -DLLDB_BUILD_FRAMEWORK=ON

And then ninja lldb, repeat ninja lldb builds causes ~500 files to be rebuilt with no source changes. I've diffed the LLDB.framework before and after and there don't seem to be any differences in the produced headers.

That's roughly how I build LLDB as well, give or take a few options.

cmake/modules/LLDBFramework.cmake

21 ↗

(On Diff #157201)

This line shouldn't be necessary anymore then, right?

[keith]

cmake/modules/LLDBFramework.cmake

21 ↗

(On Diff #157201)

It still is, the change below doesn't actually do the copying, it only runs the script on the location the headers have already been copied too.

[xiaobai]

I think this might actually not do what we want it to do. I've commented inline with my concerns.

cmake/modules/LLDBFramework.cmake

21 ↗

(On Diff #157201)

Are you saying that it will fix the framework headers that have already been copied? If so, then I don't think this will work, because the copy occurs after lldb-framework has been built, and lldb-framework depends on lldb-framework-headers.

41 ↗

(On Diff #157201)

This should not just depend on framework_headers, because those are in the location ${CMAKE_CURRENT_BINARY_DIR}/FrameworkHeaders. You want to run the script on $<TARGET_FILE_DIR:liblldb>/Headers, which is not guaranteed to exist or have been populated with headers when this actually gets run. See my comment above.

[keith]

• Make headers a post build command

[keith]

cmake/modules/LLDBFramework.cmake

41 ↗

(On Diff #157201)

Updated this by making it a POST_BUILD command instead

[xiaobai]

Looks good

cmake/modules/LLDBFramework.cmake

41 ↗

(On Diff #157201)

Can you combine this with the lldb-framework POST_BUILD above? After that this should be good to go.

[keith]

cmake/modules/LLDBFramework.cmake

41 ↗

(On Diff #157201)

If I combine it with the one above I'd have to do it twice I think?

[xiaobai]

cmake/modules/LLDBFramework.cmake

41 ↗

(On Diff #157201)

I think if you insert the command after moving the headers then you shouldn't have to do it twice

[keith]

cmake/modules/LLDBFramework.cmake

41 ↗

(On Diff #157201)

I might be misunderstanding the suggestion. I think I would have to after line 21 and line 28

[xiaobai]

cmake/modules/LLDBFramework.cmake

41 ↗

(On Diff #157201)

Oh, completely forgot about the line 28 case. In that case, why not pull out the copying of headers to right above this one then? Should allow us to prevent duplicating that logic.

[keith]

• Hoist LLDB.framework headers copy out of condition

[keith]

cmake/modules/LLDBFramework.cmake

41 ↗

(On Diff #157201)

Done!

[xiaobai]

Awesome! Thank you so much for your patience and contribution. :)

Do you need somebody to commit this for you?

[keith]

No problem! Yes please! :)

lldb/trunk/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})
+
+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 ${LLDB_SOURCE_DIR}/scripts/framework-header-fix.sh $<TARGET_FILE_DIR:liblldb>/Headers ${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 ${LLDB_FRAMEWORK_DIR}/LLDB.framework/Headers
     COMMAND ${CMAKE_COMMAND} -E create_symlink ${LLDB_FRAMEWORK_VERSION} ${LLDB_FRAMEWORK_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}
   MACOSX_FRAMEWORK_INFO_PLIST ${LLDB_SOURCE_DIR}/resources/LLDB-Info.plist
   LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LLDB_FRAMEWORK_INSTALL_DIR}
   PUBLIC_HEADER "${framework_headers}")
 
 add_dependencies(lldb-framework
-  lldb-framework-headers
   lldb-suite)