Page MenuHomePhabricator

[CMake] Support exporting runtimes using the CMake export
Needs ReviewPublic

Authored by phosek on Jul 19 2018, 8:21 PM.

Details

Reviewers
beanz
smeenai
Summary

We collect target names in the global LLVM_RUNTIMES property and
then export them to a per-target export file.

Diff Detail

Event Timeline

phosek created this revision.Jul 19 2018, 8:21 PM
phosek updated this revision to Diff 185694.Feb 6 2019, 7:17 PM
phosek added a reviewer: smeenai.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 7:17 PM
smeenai added inline comments.Feb 12 2019, 10:47 PM
libcxx/lib/CMakeLists.txt
101 ↗(On Diff #185694)

Seems like an unrelated change? I can see how it was exposed by adding the exported target support, but it still seems like it could go in separately.

llvm/runtimes/CMakeLists.txt
215

Why do we need to collect targets manually instead of just using the export(EXPORT) signature?

218

Is this assuming the per-triple resource directory layout?

Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2019, 10:47 PM
phosek marked 3 inline comments as done.Feb 13 2019, 2:56 PM
phosek added inline comments.
libcxx/lib/CMakeLists.txt
101 ↗(On Diff #185694)

D57872 is the right way which is the dependency of this change, once it lands I'll rebase this.

llvm/runtimes/CMakeLists.txt
215

I just followed what LLVM already does for LLVMExports but I'm fine using export(EXPORT) if that's preferred.

218
smeenai added inline comments.Feb 19 2019, 1:09 PM
llvm/runtimes/CMakeLists.txt
215

Yeah, the export(EXPORT) form should be equivalent here I think, and it should eliminate redundancies/prevent future divergences, so I'd prefer that.

phosek updated this revision to Diff 195636.Apr 17 2019, 3:15 PM
phosek marked 3 inline comments as done.

Sorry, had missed this.

I realized later that LLVM's export setup is to support selectively exporting components using LLVM_DISTRIBUTION_COMPONENTS. Is there an equivalent to that in the runtimes world? If there is, the export installation should respect that; if not, this LGTM.

phosek updated this revision to Diff 197813.May 2 2019, 9:46 AM

Sorry, I did not see previous revisions of this, but this looks unrelated to CMake at all now -- am I mistaken? The only CMake change is re-indenting unless I missed something.

Seems like you got the wrong diff.

phosek updated this revision to Diff 198347.May 6 2019, 3:07 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 6 2019, 3:07 PM
Herald added subscribers: Restricted Project, cfe-commits. · View Herald Transcript
phosek added a comment.May 6 2019, 3:09 PM

Seems like you got the wrong diff.

It was the right diff but uploaded to a wrong change ;)

Sorry, had missed this.

I realized later that LLVM's export setup is to support selectively exporting components using LLVM_DISTRIBUTION_COMPONENTS. Is there an equivalent to that in the runtimes world? If there is, the export installation should respect that; if not, this LGTM.

There's LLVM_RUNTIME_DISTRIBUTION_COMPONENTS which @beanz introduced a while back which is supposed to serve the same purpose. I wired it up but there were some other issues that were revealed while testing this change, hopefully those should be all addressed now.

smeenai added inline comments.May 6 2019, 3:27 PM
libunwind/src/CMakeLists.txt
160

Should be libunwind and not libcxxabi

llvm/runtimes/CMakeLists.txt
216

I think the way LLVM's build is set up, targets are always exported to the export file in the build tree (which requires maintaining the separate list of targets), but only exported to the install tree if they're in LLVM_DISTRIBUTION_COMPONENTS. I don't know how much you care about maintaining that behavior here.

I think the right way to do this was to use install(export)?

I think the right way to do this was to use install(export)?

Oh, and you are doing this, but in llvm/runtimes. Why don't we handle this per project? I don't think we should assume that the rest of LLVM is available in order to export those targets.