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

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.


================
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:
> xiaobai wrote:
> > 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.
> I don't actually use the standalone build, so I don't care about this too much. I just mentioned this because this is the default behavior when searching for packages (as well as the previous behavior when we searched for llvm-config). However, it is true that we are version-locked much more tightly with llvm than with any of the other packages we search for with find_package.
> 
> The other thing that bugs me about the LLDB_PATH_TO_(LLVM|CLANG)_BUILD variables is that they seem to imply that you should point them to the build tree of llvm/clang. However, it should be possible to build lldb against an already-installed llvm, right (in which case they will likely have the same value)? In either case, I think it would be nice to explicitly declare these as a cache variable, if only so we can provide a docstring for them.
In situations when you have multiple versions of LLVM in PATH, you usually set PATH in the order you want it to be used. And you really don't like when projects try to second-guess you.


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

https://reviews.llvm.org/D56531