Page MenuHomePhabricator

[CMake] Use find_package(LLVM) instead of LLVMConfig
ClosedPublic

Authored by phosek on May 29 2019, 7:23 PM.

Diff Detail

Event Timeline

phosek created this revision.May 29 2019, 7:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2019, 7:23 PM
phosek updated this revision to Diff 202104.May 29 2019, 7:29 PM
phosek updated this revision to Diff 202108.May 29 2019, 8:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2019, 8:06 PM
phosek retitled this revision from [CMake] Pass LLVM_BINARY_DIR to runtimes build to [CMake] Use find_package(LLVM) instead of LLVMConfig.May 29 2019, 8:07 PM
phosek edited the summary of this revision. (Show Details)

I've tested this locally and it seems to be working fine, I'd like to land this to unbreak our bots.

smeenai added inline comments.May 29 2019, 11:40 PM
llvm/runtimes/CMakeLists.txt
62

CMAKE_PREFIX_PATH won't work correctly when CMAKE_FIND_ROOT_PATH is used and CMAKE_FIND_ROOT_PATH_MODE_PACKAGE is set to ONLY, which is a fairly common configuration for cross-compilation. Setting LLVM_DIR explicitly to ${LLVM_BINARY_DIR}/lib/cmake/llvm should always work. You could combine that with NO_DEFAULT_PATH to ensure you only search in the specified directory, unless you want to be able to find some other LLVM package that's installed on the system?

phosek updated this revision to Diff 202122.May 30 2019, 12:10 AM
phosek marked an inline comment as done.
phosek added inline comments.
llvm/runtimes/CMakeLists.txt
62

I've used find_package(LLVM PATHS ...), is that what you had in mind?

LGTM with the NO_CMAKE_FIND_ROOT_PATH added.

llvm/runtimes/CMakeLists.txt
62

I believe PATHS is also affected by CMAKE_FIND_ROOT_PATH, though you could explicitly add NO_CMAKE_FIND_ROOT_PATH to avoid that. What I was thinking of was setting LLVM_DIR instead of CMAKE_PREFIX_PATH in your previous patch which was doing the explicit -DCMAKE_PREFIX_PATH=... (the <package>_DIR variable is used by CMake automatically to search for <package>), but I think PATHS + NO_CMAKE_FIND_ROOT_PATH should be equivalent and keeps the path in one place.

Does just using ${LLVM_BINARY_DIR} as the PATHS argument do the right thing, i.e. search inside lib/cmake/llvm itself? If not, you could use ${LLVM_LIBRARY_DIR} as part of the path specification here.

smeenai accepted this revision.May 30 2019, 12:20 AM
This revision is now accepted and ready to land.May 30 2019, 12:20 AM
phosek updated this revision to Diff 202127.May 30 2019, 12:30 AM
phosek marked an inline comment as done.
phosek added inline comments.
llvm/runtimes/CMakeLists.txt
62

I tested it and just ${LLVM_BINARY_DIR} seems to be sufficient so that's what I went with.

This revision was automatically updated to reflect the committed changes.