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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
