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