[Lldb-commits] [PATCH] D49779: Make framework-header-fix process copied headers

[Alex Langford via Phabricator via lldb-commits]

xiaobai requested changes to this revision.
xiaobai added a comment.
This revision now requires changes to proceed.

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



================
Comment at: cmake/modules/LLDBFramework.cmake:21
   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
----------------
keith wrote:
> xiaobai wrote:
> > This line shouldn't be necessary anymore then, right?
> 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.
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.


================
Comment at: cmake/modules/LLDBFramework.cmake:41
+add_custom_target(lldb-framework-headers
+  DEPENDS ${framework_headers}
+  COMMAND ${LLDB_SOURCE_DIR}/scripts/framework-header-fix.sh
----------------
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.


https://reviews.llvm.org/D49779