Page MenuHomePhabricator

[lldb][CMake] Infer `Clang_DIR` if not passed explicitly
ClosedPublic

Authored by sgraenitz on Aug 6 2019, 3:34 AM.

Details

Summary

If we only get LLVM_DIR and find Clang in the same provided build-tree, automatically infer Clang_DIR like this:

LLVM_DIR = /path/to/build-llvm/lib/cmake/llvm
Clang_DIR = /paht/to/build-llvm/lib/cmake/clang

Diff Detail

Repository
rL LLVM

Event Timeline

sgraenitz created this revision.Aug 6 2019, 3:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 3:34 AM
Herald added a subscriber: mgorny. · View Herald Transcript
compnerd added inline comments.Aug 6 2019, 8:24 AM
lldb/cmake/modules/LLDBStandalone.cmake
6 ↗(On Diff #213566)

What happens in the standalone clang build scenario? Can I ask what is the motivation for this change? I think it is better to require that the user pass the path, as that is an explicit dependency of LLDB.

sgraenitz added inline comments.Aug 6 2019, 8:40 AM
lldb/cmake/modules/LLDBStandalone.cmake
6 ↗(On Diff #213566)

I don't think there's any side-effects on Clang standalone builds. Is that what you mean with "standalone clang build scenario"?

I would like top prevent people from writing custom build scripts on top of CMake. Passing a number of very similar paths to CMake, e.g. each time we want to generate a Xcode project for development, this option seems to become compelling quickly. This patch makes standalone configurations simpler. Basically, it provides a default value. I doesn't cut down functionality.

You can still explicitly pass any path you want. This branch will then not be taken.

compnerd added inline comments.Aug 6 2019, 1:07 PM
lldb/cmake/modules/LLDBStandalone.cmake
6 ↗(On Diff #213566)

I think that the build fragmentation has caused a larger problem, and I would like to avoid that. The standalone build scenario is:

build/llvm
build/clang
build/lldb

In this case, ../clang does not exist (../../clang/lib/cmake/clang does). I think what I would suggest instead is adding a cache file that has the configuration parameters setup already.

sgraenitz added inline comments.Aug 6 2019, 2:06 PM
lldb/cmake/modules/LLDBStandalone.cmake
6 ↗(On Diff #213566)

For the scenario you describe, can you point me to any documentation that describes it or a bot that builds it?

I think what I would suggest instead is adding a cache file that has the configuration parameters setup already.

How would your cache file look like?

labath added inline comments.Aug 6 2019, 11:01 PM
lldb/cmake/modules/LLDBStandalone.cmake
6 ↗(On Diff #213566)

For the scenario you describe, can you point me to any documentation that describes it or a bot that builds it?

I think a lot of linux distributions which want to provide llvm,clang,etc. as separate packages build in this way. And those that don't, they'd probably want to build it that way, but they can't do it because it doesn't work for them for one reason or another (the standalone builds are always a bit behind, because the devs usually just build everything monolithically). I know at least gentoo builds llvm, clang and lldb standalone. Maybe @mgorny has a better overview of what other distros do...

sgraenitz updated this revision to Diff 213834.Aug 7 2019, 3:26 AM

Change comment and condition to only infer Clang_DIR if it exists.

sgraenitz added inline comments.Aug 7 2019, 3:32 AM
lldb/cmake/modules/LLDBStandalone.cmake
6 ↗(On Diff #213566)

Fair enough. So, in this case we just can't make a reasonable guess right?

Maybe we can still simplify the scenario for a single provided build-tree with LLVM and Clang? How could we do it?
I changed the comment and the condition to only infer the directory if it exists. Is that acceptable?

sgraenitz edited the summary of this revision. (Show Details)Aug 7 2019, 3:32 AM

What do you think?

labath added a comment.Aug 7 2019, 4:06 AM

Making it so that the clang is automatically found it if happens to be next to llvm seems like a reasonable thing to me. I have no idea what would be the canonical cmake way to do that...

sgraenitz updated this revision to Diff 213842.Aug 7 2019, 4:20 AM

Update documentation to mention multiple provided build trees and the usage of Clang_DIR

Making it so that the clang is automatically found it if happens to be next to llvm seems like a reasonable thing to me. I have no idea what would be the canonical cmake way to do that...

"the canonical cmake way" good question! It turns out that, providing another hint for find_package(Clang ...) does exactly that: it sets Clang_DIR to the first path where it found Clang. If we do not attach great importance to the status message, we could do:

find_package(LLVM REQUIRED CONFIG HINTS ${LLVM_DIR} NO_CMAKE_FIND_ROOT_PATH)
find_package(Clang REQUIRED CONFIG HINTS ${Clang_DIR} ${LLVM_DIR}/../clang NO_CMAKE_FIND_ROOT_PATH)

# Clang_DIR is defined now even if the ${LLVM_DIR}/../clang fallback was used
sgraenitz updated this revision to Diff 214139.Aug 8 2019, 6:43 AM

Achieve the same via extra HINT to find_package(Clang ...)

labath accepted this revision.Aug 8 2019, 6:52 AM

I'm happy with this, and it looks very nice and idiomatic, but maybe wait for @compnerd a bit, as he had some reservations about the whole thing..

This revision is now accepted and ready to land.Aug 8 2019, 6:52 AM
JDevlieghere accepted this revision.Aug 8 2019, 12:37 PM
Closed by commit rL372210: [lldb][CMake] Infer `Clang_DIR` if not passed explicitly (authored by stefan.graenitz, committed by ). · Explain WhySep 18 2019, 3:18 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2019, 3:18 AM