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