[Lldb-commits] [PATCH] D71306: [RFC] Change how we deal with optional dependencies

Wed Dec 11 03:51:34 PST 2019

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