sgraenitz added a comment.
In D56531#1352298 <https://reviews.llvm.org/D56531#1352298>, @mgorny wrote:
> Please don't risk merging this before the branching.
+1
The official branch goal is Wednesday this week.
================
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:
> 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.
Sounds good to me. It would be nice if `LLDB_PATH_TO_LLVM_BUILD` is sufficient
for the regular case of having LLVM & Clang in one build tree.
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
