[libcxx-commits] [PATCH] D62640: [CMake] Use find_package(LLVM) instead of LLVMConfig

[Shoaib Meenai via Phabricator via libcxx-commits]

smeenai added a comment.

LGTM with the `NO_CMAKE_FIND_ROOT_PATH `added.



================
Comment at: llvm/runtimes/CMakeLists.txt:62
 
+  find_package(LLVM)
+
----------------
phosek wrote:
> smeenai wrote:
> > `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?
> I've used `find_package(LLVM PATHS ...)`, is that what you had in mind?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62640/new/

https://reviews.llvm.org/D62640