Page MenuHomePhabricator

[CMake] Initial support for LLDB.framework
ClosedPublic

Authored by beanz on Sep 19 2016, 5:11 PM.

Details

Summary

This patch adds a CMake option LLDB_BUILD_FRAMEWORK, which builds libLLDB as a macOS framework instead of as a *nix shared library.

With this patch any LLDB executable that has the INCLUDE_IN_FRAMEWORK option set will be built into the Framework's resources directory, and a symlink to the exeuctable will be placed under the build directory's bin folder. Creating the symlinks allows users to run commands from the build directory without altering the workflow.

The framework generated by this patch passes the LLDB test suite, but has not been tested beyond that. It is not expected to be fully ready to ship, but it is a first step.

With this patch binaries that are placed inside the framework aren't being properly installed. Fixing that would increase the patch size significantly, so I'd like to do that in a follow-up.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz updated this revision to Diff 71892.Sep 19 2016, 5:11 PM
beanz retitled this revision from to [CMake] Initial support for LLDB.framework.
beanz updated this object.
beanz added reviewers: zturner, tfiala.
beanz added a subscriber: lldb-commits.
zturner added inline comments.Sep 19 2016, 5:24 PM
scripts/Python/finishSwigPythonLLDB.py
397 ↗(On Diff #71892)

Was this line intentional?

source/API/CMakeLists.txt
9 ↗(On Diff #71892)

Is there any reason to have this off when building on OSX? Is there any value in having it not be an option at all, but just if you're on Darwin, you just always get a Framework?

tfiala accepted this revision.Sep 19 2016, 6:36 PM
tfiala edited edge metadata.
tfiala added a subscriber: labath.

LGTM.

Chris - you might want to add @labath to this as well.

source/API/CMakeLists.txt
9 ↗(On Diff #71892)

I mentioned to Chris that I wasn't sure if the Google folks (or maybe Dawn) that build with the CMake build might want to keep it building the old way.

I'd be all for only building only the framework way with CMake if nobody else particularly cared. That would simplify things. But I don't want to force it if anybody else cares.

Our goal is to get the CMake-based build producing the same exact thing as our Xcode-based build.

This revision is now accepted and ready to land.Sep 19 2016, 6:36 PM

I've tested this on OS X with and without LLDB_BUILD_FRAMEWORK set, and check-lldb works in both cases. I would expect it to also work on Linux and Windows. The change should be NFC if LLDB_BUILD_FRAMEWORK=Off.

scripts/Python/finishSwigPythonLLDB.py
397 ↗(On Diff #71892)

Yes, previously when -m was not passed it returned on line 386, I've removed that return, and need to construct strSrc regardless of the branch above.

source/API/CMakeLists.txt
9 ↗(On Diff #71892)

Since this is new functionality that changes the build system behavior it is always best to leave it optional and off by default so that it can be properly tested and won't impact consumers. For users who don't set LLDB_BUILD_FRAMEWORK=On this patch should be NFC.

Additionally since OS X is Unix it would be undesirable to force OS X to always build a framework because some users may prefer unix-style builds (me being one of them). Whether or not the default build should be a framework I think is a question we can ask ourselves later once this patch is fully tested and after subsequent patches to fix the install targets.

labath added inline comments.Sep 20 2016, 1:20 PM
cmake/modules/AddLLDB.cmake
75 ↗(On Diff #71892)

I am wondering whether this wouldn't be cleaner if instead of matching on the library name, we made Framework a top-level concept, like PARAM_SHARED, and PARAM_OBJECT is.

115 ↗(On Diff #71892)

It should be possible to achieve this with the INSTALL_RPATH target property. I think that is much cleaner. https://cmake.org/cmake/help/v3.0/prop_tgt/INSTALL_RPATH.html

source/API/CMakeLists.txt
9 ↗(On Diff #71892)

Thank you for thinking about us Todd. For us, it makes our downstream scripts a bit simpler when the darwin and linux builds produce the same layout, but we're not particularly married to them. If it makes things significantly simpler, we would be fine with dropping it (but please do let us know if you intend to do that).

zturner accepted this revision.Sep 21 2016, 8:25 AM
zturner edited edge metadata.
tfiala added a comment.EditedSep 21 2016, 9:00 AM

I'd say the install rpath change is probably worth doing on the first round. I think the top-level concept for frameworks is interesting but would be fine to come in as another change.

I'd like to start being able to use this.

Thank you for thinking about us Todd.

You're welcome :-)

cmake/modules/AddLLDB.cmake
75 ↗(On Diff #71892)

@labath that seems to make sense.

115 ↗(On Diff #71892)

Yep, that seems like the more appropriate solution.

source/API/CMakeLists.txt
9 ↗(On Diff #71892)

Thank you for thinking about us Todd.

If it makes things significantly simpler, we would be fine with dropping it (but please do let us know if you intend to do that).

I think we'll see how it goes with it being optional up front, both so we can get it polished up and so we can see what kind of complexity we introduce for supporting both ways. If it all goes well, we'll talk about simplifying with a single delivery style on macOS.

labath edited edge metadata.Sep 21 2016, 12:23 PM

I'd say the install rpath change is probably worth doing on the first round. I think the top-level concept for frameworks is interesting but would be fine to come in as another change.

I'd like to start being able to use this.

sgtm

beanz updated this revision to Diff 72097.Sep 21 2016, 12:41 PM
beanz edited edge metadata.
  • Use INSTALL_RPATH and BUILD_WITH_INSTALL_RPATH instead of manually setting the rpath linker flags

Okay, LGTM.

This revision was automatically updated to reflect the committed changes.