[Lldb-commits] [PATCH] D48060: Introduce lldb-framework CMake target and centralize its logic

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.



================
Comment at: CMakeLists.txt:46
+  if (NOT APPLE)
+    message(FATAL_ERROR "LLDB.framework cannot be generated unless targeting Apple platforms")
+  endif()
----------------
`LLDB.framework can only be generated when targeting Apple platforms` avoids double negation


================
Comment at: cmake/modules/LLDBFramework.cmake:1
+if (LLDB_BUILD_FRAMEWORK)
+  add_custom_target(lldb-framework)
----------------
Maybe use early exit here?


================
Comment at: cmake/modules/LLDBFramework.cmake:45
+
+  add_dependencies(lldb-framework liblldb lldb-argdumper lldb-server lldb-framework-headers)
+  add_dependencies(finish_swig lldb-framework)
----------------
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?


================
Comment at: cmake/modules/LLDBFramework.cmake:46-47
+  add_dependencies(lldb-framework liblldb lldb-argdumper lldb-server lldb-framework-headers)
+  add_dependencies(finish_swig lldb-framework)
+  add_dependencies(lldb lldb-framework)
+endif()
----------------
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.


https://reviews.llvm.org/D48060