Page MenuHomePhabricator

[CMake] Add error to clarify that lldb requires libcxx
ClosedPublic

Authored by jryans on May 13 2019, 4:21 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jryans created this revision.May 13 2019, 4:21 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: mgorny. · View Herald Transcript
JDevlieghere added inline comments.May 13 2019, 5:05 PM
lldb/CMakeLists.txt
127 ↗(On Diff #199346)

Should we check that LLVM_ENABLE_PROJECTS is not empty, for people that are still using the old layout?

Thanks for adding this. Would it make sense to use LLVM_ENABLE_PROJECTS_USED? https://github.com/llvm/llvm-project/blob/a568222d/llvm/CMakeLists.txt#L128

BTW LLVM_ENABLE_PROJECTS can be "all" but LLVM should already have handled it at this point.

lldb/CMakeLists.txt
127 ↗(On Diff #199346)

+1

jryans updated this revision to Diff 199394.May 14 2019, 3:48 AM

Updated to support old project layout as well

jryans marked 3 inline comments as done.May 14 2019, 3:59 AM

Thanks all for the review! 😄

Thanks for adding this. Would it make sense to use LLVM_ENABLE_PROJECTS_USED? https://github.com/llvm/llvm-project/blob/a568222d/llvm/CMakeLists.txt#L128

BTW LLVM_ENABLE_PROJECTS can be "all" but LLVM should already have handled it at this point.

LLVM_ENABLE_PROJECTS_USED seems to be about "what did we use last time", which felt confusing to test here, so I have avoided that so far.

In the updated version, I have changed to testing if(NOT TARGET cxx), which should cover all cases, including:

  • LLVM_ENABLE_PROJECTS='clang;lldb;libcxx'
  • LLVM_ENABLE_PROJECTS='all'
  • Old layout
lldb/CMakeLists.txt
127 ↗(On Diff #199346)

Ah yes, thanks for catching that! I have updated this to support the old layout as well.

Thanks for the review! 😄 I don't have commit access yet, so could you please commit this for me?

sgraenitz accepted this revision.May 14 2019, 9:52 AM

Sure, will commit this on your behalf tomorrow. (If nothing else comes up.)

This revision is now accepted and ready to land.May 14 2019, 9:52 AM
Closed by commit rL360756: [CMake] Add error to clarify that lldb requires libcxx (authored by stefan.graenitz, committed by ). · Explain WhyMay 15 2019, 1:57 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2019, 1:57 AM

@sgraenitz Thanks for committing! 😄 I guess the original author info is lost for this one, but consider adding a "Patch by" line for future commits from those without access.

Yes, sorry for that. I realized it after the commit was in. The commit I got from arc patch did have the original author information. It must have changed in git llvm push, probably because it's still going to SVN and then gets mirrored back to git. I missed this last detail.

consider adding a "Patch by" line for future commits from those without access.

Given that I do this about once a year, I hope we will have switched to git at this point ;-)