sgraenitz added a comment.
Thanks for the initiative! Would be great to have this part cleaned up as well.
================
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
----------------
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?
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
