[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 an inline comment as done.
xiaobai 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
----------------
mgorny wrote:
> 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.
@labath: When I wrote this I thought that it is possible to build against an installed LLVM, but I don't think that's currently possible. For example, `LLVM_MAIN_INCLUDE_DIR` should be set to the include directory in the LLVM source tree. TableGen.cmake adds the path in this variable to the include path when it invokes llvm-tablegen. This is exposed in the LLVM CMake package as `LLVM_BUILD_MAIN_INCLUDE_DIR` but only when you're using an LLVM build tree. The reason you need this is `tools/driver/Options.td` includes `"llvm/Option/OptParser.td"`, which does not get put into the include directory of your LLVM build/install.

Maybe I should rewrite part of this to make it clearer that you need a build tree and not an llvm install? Declaring the variables as cache variables is a good idea nonetheless.

@mgorny: It seems like you find the behavior valuable so I will remove `NO_DEFAULT_PATH`. CMake processes the `HINTS` before searching your `PATH` regardles, so if you set `LLDB_PATH_TO_${PROJECT}_BUILD` then it will use that instead of using whatever it finds in your `PATH`.


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

https://reviews.llvm.org/D56531