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

[Alex Langford via Phabricator via lldb-commits]

xiaobai added inline comments.


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


================
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)
----------------
labath wrote:
> 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?
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?


================
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()
----------------
labath wrote:
> 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.
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.


https://reviews.llvm.org/D48060