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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: labath, mgorny, xiaobai.
Herald added a subscriber: abidh.
Herald added a project: LLDB.

Recently there has been some discussion about how we deal with optional dependencies in LLDB. The common approach in LLVM is to make things work out of the box. If the dependency isn't there, we just move on. This is not true in LLDB. Unless you explicitly disable the dependency with `LLDB_DISABLE_*`, you'll get a configuration-time error.

When a dependency is not found in LLVM, it prints a status message. This is something I don't like, because there are quite a lot, and they're easy to miss. I'd rather not build all of LLDB only to find out that it couldn't find Python and now I can't run the test suite. Given how "important" some of our dependencies really are, I think a warning isn't unreasonable.

For the first approach we'd need 3 variables for every CMake option.

1. The default value.
2. The user provided value, which can override the default value. If the user says they want to disable Python, we shouldn't print the aforementioned warning.
3. The value actually used by LLDB. This needs to be different from the one provided by the user, because maybe they enabled Python, but the library couldn't be found.

What I don't like about this approach is there needs to be a discrepancy between the variable provided by the user and the one used in the code. If I pass LLDB_DISABLE_PYTHON I want to grep for that and see what it affects.

The second approach is printing the warning once, and modifying the user-provided variable in the cache. So if you pass LLDB_DISABLE_PYTHON=OFF and CMake can't find it, it would update LLDB_DISABLE_PYTHON=ON, which is what you'd probably do in the current situation. This also means the warning is only printed once, because once LLDB_DISABLE_PYTHON is set, you wouldn't try looking for it again.

What I don't like about this approach is that this is messing with my user provided variables behind my back. Sure it prints a warning, but if there were other configuration issue I might need to rerun CMake and be oblivious what happened.

The third and final approach is doing what LLVM does. If the dependency is found, turn the option on (unless the value is overwritten by the user) and vice versa. This is by far the cleanest approach from a CMake and UX perspective, but I've already stated what I don't like about this.

PS: The diff shows what (2) would look like.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D71306

Files:
  lldb/cmake/modules/LLDBConfig.cmake


Index: lldb/cmake/modules/LLDBConfig.cmake
===================================================================
--- lldb/cmake/modules/LLDBConfig.cmake
+++ lldb/cmake/modules/LLDBConfig.cmake
@@ -110,23 +110,29 @@
 
 
 if (NOT LLDB_DISABLE_LIBEDIT)
-  find_package(LibEdit REQUIRED)
-
-  # Check if we libedit capable of handling wide characters (built with
-  # '--enable-widec').
-  set(CMAKE_REQUIRED_LIBRARIES ${libedit_LIBRARIES})
-  set(CMAKE_REQUIRED_INCLUDES ${libedit_INCLUDE_DIRS})
-  check_symbol_exists(el_winsertstr histedit.h LLDB_EDITLINE_USE_WCHAR)
-  set(CMAKE_EXTRA_INCLUDE_FILES histedit.h)
-  check_type_size(el_rfunc_t LLDB_EL_RFUNC_T_SIZE)
-  if (LLDB_EL_RFUNC_T_SIZE STREQUAL "")
-    set(LLDB_HAVE_EL_RFUNC_T 0)
+  find_package(LibEdit)
+
+  if (libedit_FOUND)
+    # Check if we libedit capable of handling wide characters (built with
+    # '--enable-widec').
+    set(CMAKE_REQUIRED_LIBRARIES ${libedit_LIBRARIES})
+    set(CMAKE_REQUIRED_INCLUDES ${libedit_INCLUDE_DIRS})
+    check_symbol_exists(el_winsertstr histedit.h LLDB_EDITLINE_USE_WCHAR)
+    set(CMAKE_EXTRA_INCLUDE_FILES histedit.h)
+    check_type_size(el_rfunc_t LLDB_EL_RFUNC_T_SIZE)
+    if (LLDB_EL_RFUNC_T_SIZE STREQUAL "")
+      set(LLDB_HAVE_EL_RFUNC_T 0)
+    else()
+      set(LLDB_HAVE_EL_RFUNC_T 1)
+    endif()
+    set(CMAKE_REQUIRED_LIBRARIES)
+    set(CMAKE_REQUIRED_INCLUDES)
+    set(CMAKE_EXTRA_INCLUDE_FILES)
   else()
-    set(LLDB_HAVE_EL_RFUNC_T 1)
+    message(WARNING "LLDB will build without LibEdit support, because it coulnd't be found.")
+    set(LLDB_DISABLE_LIBEDIT ON FORCE)
   endif()
-  set(CMAKE_REQUIRED_LIBRARIES)
-  set(CMAKE_REQUIRED_INCLUDES)
-  set(CMAKE_EXTRA_INCLUDE_FILES)
+
 endif()
 
 
@@ -488,18 +494,22 @@
 endif()
 
 if (NOT LLDB_DISABLE_CURSES)
-    find_package(Curses REQUIRED)
-
-    find_library(CURSES_PANEL_LIBRARY NAMES panel DOC "The curses panel library")
-    if (NOT CURSES_PANEL_LIBRARY)
-        message(FATAL_ERROR "A required curses' panel library not found.")
-    endif ()
-
-    # Add panels to the library path
-    set (CURSES_LIBRARIES ${CURSES_LIBRARIES} ${CURSES_PANEL_LIBRARY})
-
-    list(APPEND system_libs ${CURSES_LIBRARIES})
-    include_directories(${CURSES_INCLUDE_DIR})
+    find_package(Curses)
+    if (CURSES_FOUND)
+      find_library(CURSES_PANEL_LIBRARY NAMES panel DOC "The curses panel library")
+      if (NOT CURSES_PANEL_LIBRARY)
+          message(FATAL_ERROR "A required curses' panel library not found.")
+      endif ()
+
+      # Add panels to the library path
+      set (CURSES_LIBRARIES ${CURSES_LIBRARIES} ${CURSES_PANEL_LIBRARY})
+
+      list(APPEND system_libs ${CURSES_LIBRARIES})
+      include_directories(${CURSES_INCLUDE_DIR})
+    else()
+      message(WARNING "LLDB will build without Curses support, because it coulnd't be found.")
+      set(LLDB_DISABLE_CURSES ON FORCE)
+    endif()
 endif ()
 
 if ((CMAKE_SYSTEM_NAME MATCHES "Android") AND LLVM_BUILD_STATIC AND


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D71306.233197.patch
Type: text/x-patch
Size: 2969 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20191210/e4b4dab3/attachment.bin>