xiaobai 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)
----------------
labath wrote:
> fully-functioning lldb **library**.
>
> (this is just to emphasize that it does not include the driver executable
> with the same name)
Thanks for pointing that out.
================
Comment at: CMakeLists.txt:154
+endif()
+add_dependencies(lldb lldb-suite)
+
----------------
labath wrote:
> I would move this line to the CMakeLilsts.txt which defines the lldb driver
> target.
That makes sense to me.
================
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)
----------------
labath wrote:
> Could we replace this by a single `finish_swig -> lldb-suite` dependency ?
I didn't think about it at the time but that makes the most sense
================
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:
> 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).
I'm going to attempt to make centralization work just because I would like to
avoid knowledge of what goes into the framework scattered. Thanks for the help.
https://reviews.llvm.org/D48060
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
