[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
----------------
sgraenitz wrote:
> xiaobai wrote:
> > 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`.
> Is it that instead of `-DLLVM_CONFIG=/path/to/llvm-build/bin/llvm-config` we would now pass `-DLLDB_PATH_TO_LLVM_BUILD=/path/to/llvm-build`?
> 
> > LLDB_PATH_TO_(LLVM|CLANG)_BUILD variables [...] they will likely have the same value?
> 
> In my understanding this was always required. Did you try pointing `LLDB_PATH_TO_CLANG_BUILD` to a standalone build of Clang? Is there a good use-case for it? How do we detect that this Clang builds against the same LLVM build-tree?
> 
> When using LLVM/Clang/etc. as a dependency in external projects I usually followed the advice in https://www.llvm.org/docs/CMake.html#embedding-llvm-in-your-project:
> > If LLVM is not installed or you wish to build directly against the LLVM build tree you can use LLVM_DIR as previously mentioned.
> 
> That would be quite simple and `find_package(LLVM)` accepts `LLVM_DIR` as an input. Did you check how the other subproject do that?
>Is it that instead of -DLLVM_CONFIG=/path/to/llvm-build/bin/llvm-config we would now pass -DLLDB_PATH_TO_LLVM_BUILD=/path/to/llvm-build?

Correct.

>In my understanding this was always required. Did you try pointing LLDB_PATH_TO_CLANG_BUILD to a standalone build of Clang? Is there a good use-case for it? How do we detect that this Clang builds against the same LLVM build-tree?

I did not try doing it with a standalone build of Clang. I'm not sure if there is a good use-case for it, but if somebody knows then I'd like to hear about it. I am aware that it is possible to do a standalone build of Clang but I usually do integrated builds.

As for detecting whether or not the Clang builds against the same LLVM tree, I'm not sure if that's possible currently with the way LLVM and Clang's CMake packages are set up.

> That would be quite simple and find_package(LLVM) accepts LLVM_DIR as an input. Did you check how the other subproject do that?

The other subprojects that I have looked at (clang, lld) have the current behavior of using llvm-config, which I think repeats a lot of work. Ideally the other projects would use the LLVM CMake package as well imo.

As for assigning LLVM_DIR directly, that is something that you could do. Even if I submitted this patch today as-is, setting LLVM_DIR directly would ignore `LLDB_PATH_TO_LLVM_BUILD` entirely. I would prefer still having the `HINTS` mechanism because it can take a list of directories. This is especially useful on build machines where the LLVM build can be found in one of a few possible locations. I also believe that the variable `LLDB_PATH_TO_LLVM_BUILD` is a little easier to understand than `LLVM_DIR` for people trying to figure out how to build LLDB. I see these two solutions as the same in every other regard though.


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

https://reviews.llvm.org/D56531