labath added inline comments.
================
Comment at: CMakeLists.txt:140
+# lldb-suite is a dummy target that encompasses all the necessary tools and
+# libraries for building a fully-functioning lldb.
+add_custom_target(lldb-suite)
----------------
fully-functioning lldb **library**.
(this is just to emphasize that it does not include the driver executable with
the same name)
================
Comment at: CMakeLists.txt:154
+endif()
+add_dependencies(lldb lldb-suite)
+
----------------
I would move this line to the CMakeLilsts.txt which defines the lldb driver
target.
================
Comment at: CMakeLists.txt:180-186
+ # When building LLDB.framework, finish_swig depends on the framework
+ # directory already being generated. liblldb and lldb-argdumper are built
+ # as a part of the framework.
+ add_dependencies(finish_swig lldb-framework)
+ else()
+ # We depend on liblldb and lldb-argdumper being built before we can do
this step.
+ add_dependencies(finish_swig liblldb lldb-argdumper)
----------------
Could we replace this by a single `finish_swig -> lldb-suite` dependency ?
================
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)
----------------
xiaobai wrote:
> labath wrote:
> > xiaobai wrote:
> > > 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?
> > Both of the approaches sound reasonable to me. If you want to go this way,
> > then I'm fine with that.
> I've begun refactoring to remove `INCLUDE_IN_FRAMEWORK` but I've started
> thinking about some possibly negative repercussions. I'm wondering what you
> think of this:
>
> `add_lldb_tool` (which invokes `add_lldb_executable`) can be used to add lldb
> executables that won't be included in the framework AFAICT (e.g. lldb-mi).
> What I was going to do was remove `INCLUDE_IN_FRAMEWORK` option so that every
> tool will get put into the framework and symlinked to `$BUILD_DIR/bin` when
> you run cmake with `LLDB_BUILD_FRAMEWORK=1`. This will mean that lldb-mi will
> be put in the framework if you do something like `make lldb-mi` after
> configuring with `LLDB_BUILD_FRAMEWORK=1`.
>
> In that case, do you think it makes sense to keep `INCLUDE_IN_FRAMEWORK` as
> an option and use that to build part of the dependency tree for the
> lldb-framework target? Or do you think this is an unlikely enough example
> that it shouldn't matter?
I think this is an important issue. We shouldn't have our install artifacts
depend on what other targets the user happened to build. And it's not just the
user typing "make lldb-mi" thats the problem. He can just decide to type "make"
to build all targets, and then he'll end up with the same issue.
I think it still may be possible to have framework management centralized if
you would make the list of targets that go into the framework explicit here.
Something like:
```
foreach(target in ${TOOLS_THAT_GO_INTO_THE_FRAMEWORK})
do_whatever_add_lldb_tool_would_have_done()
endforeach()
```
But, as I said, I think that keeping this part of the logic inside
add_lldb_tool is fine too. If you decide to keep INCLUDE_IN_FRAMEWORK arg, then
I think it is natural for the dependency management to be done there too.
(Basically, I want to avoid the knowledge of what tools go into the framework
to be defined in more than one place).
https://reviews.llvm.org/D48060
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
