This is an archive of the discontinued LLVM Phabricator instance.

Remove lldb-private headers when building LLDB.framework with CMake
ClosedPublic

Authored by xiaobai on May 23 2018, 1:07 PM.

Details

Summary

Generating LLDB.framework when building with CMake+Ninja will copy the
lldb-private headers because public_headers contains them, even though we try
to make sure they don't get copied by removing root_private_headers from
root_public_headers.

This patch also removes SystemInitializerFull.h from the LLDB.framework headers when building with CMake.

Diff Detail

Repository
rL LLVM

Event Timeline

xiaobai created this revision.May 23 2018, 1:07 PM

It would be good to verify all headers that are in the LLDB.framework produced by Xcode builds are also in the LLDB.framework from cmake/ninja

I also think that'd be a great idea. The only way I can think to do that would be to build LLDB.framework using both build systems and compare the two. Is there a faster way?

I also think that'd be a great idea. The only way I can think to do that would be to build LLDB.framework using both build systems and compare the two. Is there a faster way?

That is the only guaranteed way as new headers could come along. LLDB.framework doesn't have headers in installed Xcode.app bundles

That is the only guaranteed way as new headers could come along. LLDB.framework doesn't have headers in installed Xcode.app bundles

In that case, adding this test could double build times since we would effectively be building lldb twice. I'm not sure if this is already a pain point, but if it is, then I don't think we should add this as a test right now.

sorry, not as a test, but just as a way to figure out if we are getting all the needed header files when we modify this framework header file copying code.

sorry, not as a test, but just as a way to figure out if we are getting all the needed header files when we modify this framework header file copying code.

Ah, yeah. I'm in the process of trying to build LLDB.framework with cmake+ninja and get the same thing as when you build with xcodebuild.
As far as headers go, we roughly have parity after this change. CMake copies SystemInitializerFull.h into LLDB.Framework/Headers while Xcode doesn't, but I'm not sure that actually matters that much.

sorry, not as a test, but just as a way to figure out if we are getting all the needed header files when we modify this framework header file copying code.

Ah, yeah. I'm in the process of trying to build LLDB.framework with cmake+ninja and get the same thing as when you build with xcodebuild.
As far as headers go, we roughly have parity after this change. CMake copies SystemInitializerFull.h into LLDB.Framework/Headers while Xcode doesn't, but I'm not sure that actually matters that much.

It does as we can't expose any lldb_private functions and this exposes lldb_private::SystemInitializerFull which requires lldb_private::SystemInitializerCommon. The only references to stuff inside lldb_private in headers that make it into the LLDB.framework can be forward references like:

namespace lldb_private {
   class Foo;
}

sorry, not as a test, but just as a way to figure out if we are getting all the needed header files when we modify this framework header file copying code.

Ah, yeah. I'm in the process of trying to build LLDB.framework with cmake+ninja and get the same thing as when you build with xcodebuild.
As far as headers go, we roughly have parity after this change. CMake copies SystemInitializerFull.h into LLDB.Framework/Headers while Xcode doesn't, but I'm not sure that actually matters that much.

It does as we can't expose any lldb_private functions and this exposes lldb_private::SystemInitializerFull which requires lldb_private::SystemInitializerCommon. The only references to stuff inside lldb_private in headers that make it into the LLDB.framework can be forward references like:

namespace lldb_private {
   class Foo;
}

I see, thanks for explaining why it matters. I'll update this revision.

xiaobai updated this revision to Diff 148305.May 23 2018, 3:31 PM

Remove SystemInitializerFull.h from framework headers

xiaobai edited the summary of this revision. (Show Details)May 23 2018, 3:33 PM

The issue is actually that SystemInitializerFull.h and SystemInitializerFull.cpp are in the wrong directories. They belong in the "lldb/Initialization" and "Source//Initialization". We should fix this and then some/all of your changes won't be needed?

From a layering perspective, it makes sense for SystemInitializerFull to live in the outermost layer, as it's the thing which makes sure liblldb pulls in all required components. Since it is only included from files in source/API (which is as it should be), maybe we could just make it a private header and move the file to source/API/SystemInitializerFull.h ?

From a layering perspective, it makes sense for SystemInitializerFull to live in the outermost layer, as it's the thing which makes sure liblldb pulls in all required components. Since it is only included from files in source/API (which is as it should be), maybe we could just make it a private header and move the file to source/API/SystemInitializerFull.h?

This makes the most sense to me. I've uploaded D47342 which I believe does what you've suggested.

The issue is actually that SystemInitializerFull.h and SystemInitializerFull.cpp are in the wrong directories. They belong in the "lldb/Initialization" and "Source//Initialization". We should fix this and then some/all of your changes won't be needed?

Some of them for sure. It's a mistake to add ${LLDB_SOURCE_DIR}/include/lldb/lldb-*.h to public_headers, so that should still be changed.

xiaobai updated this revision to Diff 148688.May 25 2018, 4:39 PM

Updating to reflect changes in r333304

labath accepted this revision.May 26 2018, 6:37 AM
This revision is now accepted and ready to land.May 26 2018, 6:37 AM
This revision was automatically updated to reflect the committed changes.