[Lldb-commits] [PATCH] D56531: [CMake] Replace use of llvm-config with LLVM and Clang CMake packages

[Alex Langford via Phabricator via lldb-commits]

xiaobai marked 2 inline comments as done.
xiaobai added inline comments.


================
Comment at: cmake/modules/LLDBStandalone.cmake:7
   option(LLVM_INSTALL_TOOLCHAIN_ONLY "Only include toolchain files in the 'install' target." OFF)
 
+  find_package(LLVM REQUIRED CONFIG
----------------
compnerd wrote:
> Please add the following here:
> 
> ```
> file(TO_CMAKE_PATH LLDB_PATH_TO_LLVM_BUILD "${LLDB_PATH_TO_LLVM_BUILD}")
> file(TO_CMAKE_PATH LLDB_PATH_TO_CLANG_BUILD" "${LLDB_PATH_TO_CLANG_BUILD}")
> ```
> 
> This ensures that you can use platform specific paths when they do not match CMake's view (i.e. Windows)
Will do, thanks!


================
Comment at: cmake/modules/LLDBStandalone.cmake:9
+  find_package(LLVM REQUIRED CONFIG
+    HINTS "${LLDB_PATH_TO_LLVM_BUILD}" NO_DEFAULT_PATH NO_CMAKE_FIND_ROOT_PATH)
+  find_package(Clang REQUIRED CONFIG
----------------
labath wrote:
> Why do you put `NO_DEFAULT_PATH` here? IIUC, the user will now have to specify `LLDB_PATH_TO_LLVM_BUILD` in order to build this, whereas previously llvm-config would be found on the path if it happened to be there (e.g. because it was already installed).
> 
> Would it not make sense to keep this behavior?
In situations where you have multiple LLVM builds where each might be a different version of LLVM, I think it is better to force the user to specify which LLVM build they want to use instead of having them implicitly rely on whichever llvm-config happens to be in their path.

That being said, I would be willing to remove `NO_DEFAULT_PATH` here. I can understand if people find this behavior valuable or if the scenario I described is not very common.


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

https://reviews.llvm.org/D56531