This is an archive of the discontinued LLVM Phabricator instance.

Introduce lldb-framework CMake target and centralize its logic
ClosedPublic

Authored by xiaobai on Jun 11 2018, 7:27 PM.

Details

Summary

In this patch I aim to do the following:

  1. Create an lldb-framework target that acts as the target that handles generating LLDB.framework. Previously, liblldb acted as the target for generating the framework in addition to generating the actual lldb library. This made the target feel overloaded.
  2. Centralize framework generation as much as it makes sense to do so.
  3. Create a target lldb-suite, which depends on every tool and library that makes liblldb fully functional. One result of having this target is it makes tracking dependencies much clearer.

Diff Detail

Repository
rL LLVM

Event Timeline

xiaobai created this revision.Jun 11 2018, 7:27 PM

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.

CMakeLists.txt
46 ↗(On Diff #150881)

LLDB.framework can only be generated when targeting Apple platforms avoids double negation

cmake/modules/LLDBFramework.cmake
1 ↗(On Diff #150881)

Maybe use early exit here?

45 ↗(On Diff #150881)

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?

46–47 ↗(On Diff #150881)

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.

xiaobai added inline comments.Jun 12 2018, 11:19 AM
cmake/modules/LLDBFramework.cmake
1 ↗(On Diff #150881)

Sounds good

45 ↗(On Diff #150881)

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?

46–47 ↗(On Diff #150881)

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.

labath added inline comments.Jun 12 2018, 12:08 PM
cmake/modules/LLDBFramework.cmake
45 ↗(On Diff #150881)

Both of the approaches sound reasonable to me. If you want to go this way, then I'm fine with that.

xiaobai added inline comments.Jun 12 2018, 3:30 PM
cmake/modules/LLDBFramework.cmake
45 ↗(On Diff #150881)

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?

xiaobai updated this revision to Diff 151048.Jun 12 2018, 3:31 PM

Updated based on feedback. I would like to get more feedback before I'm comfortable changing it further and/or committing this.

xiaobai updated this revision to Diff 151049.Jun 12 2018, 3:32 PM

Minor change I forgot to make

Harbormaster completed remote builds in B19252: Diff 151049.
compnerd added inline comments.Jun 12 2018, 4:30 PM
CMakeLists.txt
145 ↗(On Diff #151049)

Shouldn't this be else()?

176 ↗(On Diff #151049)

"finish swig"?

cmake/modules/LLDBFramework.cmake
5 ↗(On Diff #151049)

Globs are generally frowned upon for dependency tracking issues.

xiaobai added inline comments.Jun 12 2018, 4:32 PM
CMakeLists.txt
145 ↗(On Diff #151049)

Yup

176 ↗(On Diff #151049)

Should be "finish_swig"

cmake/modules/LLDBFramework.cmake
5 ↗(On Diff #151049)

I see. This code was moved from somewhere else and written by somebody else. My CMake is somewhat weak, how would you do this instead?

xiaobai updated this revision to Diff 151067.Jun 12 2018, 4:33 PM

Minor fixups pointed out by compnerd

clayborg resigned from this revision.Jun 12 2018, 6:00 PM

I'll defer to the cmake experts. I hope we can produce a LLDB.framework that is identical to the Xcode produced version, or at least as close.

labath added inline comments.Jun 13 2018, 12:11 AM
CMakeLists.txt
140 ↗(On Diff #151067)

fully-functioning lldb library.

(this is just to emphasize that it does not include the driver executable with the same name)

154 ↗(On Diff #151067)

I would move this line to the CMakeLilsts.txt which defines the lldb driver target.

180–186 ↗(On Diff #151067)

Could we replace this by a single finish_swig -> lldb-suite dependency ?

cmake/modules/LLDBFramework.cmake
45 ↗(On Diff #150881)

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).

xiaobai added inline comments.Jun 13 2018, 11:26 AM
CMakeLists.txt
140 ↗(On Diff #151067)

Thanks for pointing that out.

154 ↗(On Diff #151067)

That makes sense to me.

180–186 ↗(On Diff #151067)

I didn't think about it at the time but that makes the most sense

cmake/modules/LLDBFramework.cmake
45 ↗(On Diff #150881)

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.

xiaobai updated this revision to Diff 151543.Jun 15 2018, 12:38 PM

Pavel's suggestions.

I think this is in a pretty good state. I built LLDB.framework with xcodebuild and compared it. There are still some discrepancies but this brings us a step closer to a final working solution.

cmake/modules/LLDBFramework.cmake
45 ↗(On Diff #150881)

Decided to keep INCLUDE_IN_FRAMEWORK as removing it would introduce more boilerplate and wrapping that works around the problem instead of fixing it.

This is looking very good, I just have one more quick question. I am wondering whether we couldn't simplify this dependency management a tad bit more. Would the thing I'm proposing below work for you?

CMakeLists.txt
147–153 ↗(On Diff #151543)

This code here.

cmake/modules/AddLLDB.cmake
102–104 ↗(On Diff #151543)

If you reorder this slightly, then you should be able to get rid of the extra dependency code in main CMakeLists.txt.
Something like this:

if (ARG_INCLUDE_IN_FRAMEWORK)
  add_dependencies(lldb-suite ${name})
  if (LLDB_BUILD_FRAMEWORK)
    ...

(of course, then the INCLUDE_IN_FRAMEWORK arg will become misnamed, but that can be solved by renaming it to INCLUDE_IN_SUITE)

xiaobai added a comment.EditedJun 15 2018, 2:00 PM
This comment has been deleted.
cmake/modules/AddLLDB.cmake
102–104 ↗(On Diff #151543)

This is a pretty good idea imo. I think it'll make the dependency tracking much easier. The lldb-framework target then becomes a small target.

xiaobai edited the summary of this revision. (Show Details)Jun 15 2018, 4:46 PM
xiaobai updated this revision to Diff 151598.Jun 15 2018, 6:09 PM

Labath's suggestion

labath accepted this revision.Jun 18 2018, 12:39 AM

Looks good. I'm really happy with how this turned out. Thank you for your patience.

This revision is now accepted and ready to land.Jun 18 2018, 12:39 AM

Indeed!!! So happy to see this work getting done. Great stuff. Will be great to not have to maintain the Xcode project as some point in the near future.

This revision was automatically updated to reflect the committed changes.

Thank you @labath for your help. I'm much happier with this!