labath added a comment.
It's true that external dependencies are a lot more critical for lldb than
llvm. That might justify some deviation from it, but I would certainly try to
minimize that. I am a big anti-fan of overwriting user-provided values, so I'd
really try to avoid that method.
Besides lldb developers, another category we should consider are llvm (or clang
or lld, ...) developers who know nothing about lldb, but they ended up building
it because they specified LLVM_ENABLE_PROJECTS=all. These people probably don't
want to figure out how to install libedit, or how to disable it, but if the
default config "just works" for them, they'd be happy to build lldb and make
sure their patch doesn't break it. Optimizing for this experience may be even
more important than core lldb developers, since there's fewer of the latter and
they usually just configure lldb once.
Ignoring inconsistency with llvm, the approach I would like the most is to have
the dependency options be ternary. "ON" means the dependency must be there, and
if it isn't you get an error. "OFF" means no dependency, we don't even try to
look for it. The third option (let's call it "AUTO") would try to look for the
dependency, and use it if it's there -- if not, you get the usual status
message.
This isn't entirely consistent with llvm, but it's not that far from it either
-- llvm essentially just has two of these options: "OFF" and "AUTO", only auto
is called "ON". If we wanted to be more consistent, we could even rename these
options to "OFF", "ON" and "FORCE_ON"...
People who want to be sure they are using python could just set this variable
to "FORCE_ON", for instance in their cache file. Would that be enough to
address your desire for explicitness?
As for the number of variables, I don't think we really need that many of them.
I think the default value should always be "auto". The only reason we spend so
much effort in computing the default value is because some platforms don't ever
have a certain dependency, and we wanted the build to "just work" there. If
"auto" just works, this can all be thrown out the window.
And I don't think we need a separate /names/ for the "value selected by user"
and "the effective value". We can reuse the "cache" vs "regular variable"
distinction here (as you have accidentally done in this patch). While shadowing
cache variables is a bit "naughty", I think it can actually be helpful in some
cases. For example, nothing except the code which actually handles the
dependency search should ever care about the setting value "as provided by
user". They should only ever care about the effective value -- shadowing can
ensure that. And this is a practice successfully used elsewhere in llvm --
e.g., we create a shadow for LLVM_TARGETS_TO_BUILD, which expands the "all"
pseudo-target provided by the user into the actual list of targets. However, I
don't recall seeing this approach in dependency management code...
The implementation of something like this could be:
# This assumes that LLDB_DISABLE_PYTHON is renamed to LLDB_ENABLE_PYTHON
across the board, which is something I've wanted to do for a while..
set(LLDB_ENABLE_PYTHON "ON" CACHE STRING "On, Off or Auto")
string(TOLOWER "${LLDB_ENABLE_PYTHON}" LLDB_ENABLE_PYTHON)
if (LLDB_ENABLE_PYTHON)
if ("${LLDB_ENABLE_PYTHON}" STREQUAL "auto")
set(maybe_required)
else
set(maybe_required REQUIRED)
endif()
find_package(Python ${maybe_required})
set(LLDB_ENABLE_PYTHON ${PYTHON_FOUND})
endif()
In D71306#1778472 <https://reviews.llvm.org/D71306#1778472>, @xiaobai wrote:
> I personally prefer the third approach. To make sure I understand correctly,
> I'll write it in my own words so you can correct me if I misunderstood.
> Try to find the dependency, and if we find it then use it. If not, then we
> can print out something like "Didn't find `DEPENDENCY`" and continue on our
> merry way. If the user overwrites the values and something goes wrong, send a
> fatal error and tell them that what the value they set isn't going to work
> without further work (e.g. explicitly enable python support but didn't find
> python? tell the user that you couldn't find python and maybe suggest setting
> some other CMake variables to help CMake find python).
How exactly does this "overwriting" work? Could you point me to the code that
does this? I don't remember seeing anything like this, but the llvm build is
not entirely consistent either, so it's possible we're looking at different
things...
================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:133
+ message(WARNING "LLDB will build without LibEdit support, because it
coulnd't be found.")
+ set(LLDB_DISABLE_LIBEDIT ON FORCE)
endif()
----------------
I don't think this does what you wanted it to do. This leaves the cache
variable intact, and sets the local cmake variable with the same name to
"ON;FORCE".
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71306/new/
https://reviews.llvm.org/D71306
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
