Skip to content

[Support] Remove terminfo dependency #92865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 24, 2024

Conversation

aaronmondal
Copy link
Member

The terminfo dependency introduces a significant nonhermeticity into the build. It doesn't respect --no-undefined-version meaning that it's not a dependency that can be built with Clang 17+. This forces maintainers of source-based distributions to implement patches or ignore linker errors.

Remove it to reduce the closure size and improve portability of LLVM-based tools. Users can still use command line arguments to toggle color support expliticly.

Fixes #75490
Closes #53294 #23355

@llvmbot llvmbot added cmake Build system in general and CMake in particular clang Clang issues not falling into any other category compiler-rt lldb clang:static analyzer xray llvm:support bazel "Peripheral" support tier build system: utils/bazel compiler-rt:sanitizer labels May 21, 2024
@llvmbot
Copy link
Member

llvmbot commented May 21, 2024

@llvm/pr-subscribers-xray
@llvm/pr-subscribers-clang-static-analyzer-1
@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-lldb

@llvm/pr-subscribers-clang

Author: Aaron Siddhartha Mondal (aaronmondal)

Changes

The terminfo dependency introduces a significant nonhermeticity into the build. It doesn't respect --no-undefined-version meaning that it's not a dependency that can be built with Clang 17+. This forces maintainers of source-based distributions to implement patches or ignore linker errors.

Remove it to reduce the closure size and improve portability of LLVM-based tools. Users can still use command line arguments to toggle color support expliticly.

Fixes #75490
Closes #53294 #23355


Patch is 20.59 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/92865.diff

25 Files Affected:

  • (modified) clang/cmake/caches/Fuchsia-stage2.cmake (-1)
  • (modified) clang/cmake/caches/Fuchsia.cmake (-7)
  • (modified) clang/cmake/caches/VectorEngine.cmake (+1-3)
  • (modified) clang/utils/analyzer/entrypoint.py (+1-1)
  • (modified) compiler-rt/cmake/config-ix.cmake (-15)
  • (modified) compiler-rt/lib/sanitizer_common/symbolizer/scripts/build_symbolizer.sh (-1)
  • (modified) compiler-rt/lib/xray/tests/CMakeLists.txt (-5)
  • (modified) lldb/docs/resources/build.rst (-1)
  • (modified) lldb/source/Core/CMakeLists.txt (-3)
  • (modified) llvm/CMakeLists.txt (-2)
  • (modified) llvm/cmake/config-ix.cmake (-10)
  • (removed) llvm/cmake/modules/FindTerminfo.cmake (-55)
  • (modified) llvm/cmake/modules/LLVMConfig.cmake.in (-5)
  • (modified) llvm/docs/ReleaseNotes.rst (+4)
  • (modified) llvm/include/llvm/Config/config.h.cmake (-3)
  • (modified) llvm/lib/Support/CMakeLists.txt (-11)
  • (modified) llvm/lib/Support/Unix/Process.inc (+4-56)
  • (modified) llvm/utils/gn/README.rst (+1-1)
  • (removed) llvm/utils/gn/build/libs/terminfo/BUILD.gn (-12)
  • (removed) llvm/utils/gn/build/libs/terminfo/enable.gni (-4)
  • (modified) llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn (-7)
  • (modified) llvm/utils/gn/secondary/llvm/lib/Support/BUILD.gn (-1)
  • (modified) llvm/utils/gn/secondary/llvm/tools/llvm-config/BUILD.gn (+1-5)
  • (modified) utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h (-3)
  • (modified) utils/bazel/llvm_configs/config.h.cmake (-3)
diff --git a/clang/cmake/caches/Fuchsia-stage2.cmake b/clang/cmake/caches/Fuchsia-stage2.cmake
index d5546e20873b3..66e764968e85c 100644
--- a/clang/cmake/caches/Fuchsia-stage2.cmake
+++ b/clang/cmake/caches/Fuchsia-stage2.cmake
@@ -19,7 +19,6 @@ set(LLVM_ENABLE_LLD ON CACHE BOOL "")
 set(LLVM_ENABLE_LTO ON CACHE BOOL "")
 set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR ON CACHE BOOL "")
 set(LLVM_ENABLE_PLUGINS OFF CACHE BOOL "")
-set(LLVM_ENABLE_TERMINFO OFF CACHE BOOL "")
 set(LLVM_ENABLE_UNWIND_TABLES OFF CACHE BOOL "")
 set(LLVM_ENABLE_Z3_SOLVER OFF CACHE BOOL "")
 set(LLVM_ENABLE_ZLIB ON CACHE BOOL "")
diff --git a/clang/cmake/caches/Fuchsia.cmake b/clang/cmake/caches/Fuchsia.cmake
index 30a3b9116a461..4d3af3ad3f403 100644
--- a/clang/cmake/caches/Fuchsia.cmake
+++ b/clang/cmake/caches/Fuchsia.cmake
@@ -12,7 +12,6 @@ set(LLVM_ENABLE_DIA_SDK OFF CACHE BOOL "")
 set(LLVM_ENABLE_LIBEDIT OFF CACHE BOOL "")
 set(LLVM_ENABLE_LIBXML2 OFF CACHE BOOL "")
 set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR ON CACHE BOOL "")
-set(LLVM_ENABLE_TERMINFO OFF CACHE BOOL "")
 set(LLVM_ENABLE_UNWIND_TABLES OFF CACHE BOOL "")
 set(LLVM_ENABLE_Z3_SOLVER OFF CACHE BOOL "")
 set(LLVM_ENABLE_ZLIB OFF CACHE BOOL "")
@@ -34,7 +33,6 @@ set(_FUCHSIA_BOOTSTRAP_PASSTHROUGH
   LibXml2_ROOT
   LLVM_ENABLE_CURL
   LLVM_ENABLE_HTTPLIB
-  LLVM_ENABLE_TERMINFO
   LLVM_ENABLE_LIBEDIT
   CURL_ROOT
   OpenSSL_ROOT
@@ -47,11 +45,6 @@ set(_FUCHSIA_BOOTSTRAP_PASSTHROUGH
   CURSES_LIBRARIES
   PANEL_LIBRARIES
 
-  # Deprecated
-  Terminfo_ROOT
-
-  Terminfo_LIBRARIES
-
   # Deprecated
   LibEdit_ROOT
 
diff --git a/clang/cmake/caches/VectorEngine.cmake b/clang/cmake/caches/VectorEngine.cmake
index 2f968a21cc407..b429fb0997d7a 100644
--- a/clang/cmake/caches/VectorEngine.cmake
+++ b/clang/cmake/caches/VectorEngine.cmake
@@ -13,9 +13,7 @@
 #   ninja
 #
 
-# Disable TERMINFO, ZLIB, and ZSTD for VE since there is no pre-compiled
-# libraries.
-set(LLVM_ENABLE_TERMINFO OFF CACHE BOOL "")
+# Disable ZLIB, and ZSTD for VE since there is no pre-compiled libraries.
 set(LLVM_ENABLE_ZLIB OFF CACHE BOOL "")
 set(LLVM_ENABLE_ZSTD OFF CACHE BOOL "")
 
diff --git a/clang/utils/analyzer/entrypoint.py b/clang/utils/analyzer/entrypoint.py
index ff877060bad69..4deb42db0a0b1 100644
--- a/clang/utils/analyzer/entrypoint.py
+++ b/clang/utils/analyzer/entrypoint.py
@@ -54,7 +54,7 @@ def is_cmake_needed():
     "cmake -G Ninja -DCMAKE_BUILD_TYPE=Release "
     "-DCMAKE_INSTALL_PREFIX=/analyzer -DLLVM_TARGETS_TO_BUILD=X86 "
     '-DLLVM_ENABLE_PROJECTS="clang;openmp" -DLLVM_BUILD_RUNTIME=OFF '
-    "-DLLVM_ENABLE_TERMINFO=OFF -DCLANG_ENABLE_ARCMT=OFF "
+    "-DCLANG_ENABLE_ARCMT=OFF "
     "-DCLANG_ENABLE_STATIC_ANALYZER=ON"
 )
 
diff --git a/compiler-rt/cmake/config-ix.cmake b/compiler-rt/cmake/config-ix.cmake
index 42edbe15edafb..bddaa37579fd7 100644
--- a/compiler-rt/cmake/config-ix.cmake
+++ b/compiler-rt/cmake/config-ix.cmake
@@ -182,21 +182,6 @@ check_library_exists(m pow "" COMPILER_RT_HAS_LIBM)
 check_library_exists(pthread pthread_create "" COMPILER_RT_HAS_LIBPTHREAD)
 check_library_exists(execinfo backtrace "" COMPILER_RT_HAS_LIBEXECINFO)
 
-# Look for terminfo library, used in unittests that depend on LLVMSupport.
-if(LLVM_ENABLE_TERMINFO STREQUAL FORCE_ON)
-  set(MAYBE_REQUIRED REQUIRED)
-else()
-  set(MAYBE_REQUIRED)
-endif()
-if(LLVM_ENABLE_TERMINFO)
-  find_library(COMPILER_RT_TERMINFO_LIB NAMES terminfo tinfo curses ncurses ncursesw ${MAYBE_REQUIRED})
-endif()
-if(COMPILER_RT_TERMINFO_LIB)
-  set(LLVM_ENABLE_TERMINFO 1)
-else()
-  set(LLVM_ENABLE_TERMINFO 0)
-endif()
-
 if (ANDROID AND COMPILER_RT_HAS_LIBDL)
   # Android's libstdc++ has a dependency on libdl.
   list(APPEND CMAKE_REQUIRED_LIBRARIES dl)
diff --git a/compiler-rt/lib/sanitizer_common/symbolizer/scripts/build_symbolizer.sh b/compiler-rt/lib/sanitizer_common/symbolizer/scripts/build_symbolizer.sh
index 005bd6d584c59..b4702339db59c 100755
--- a/compiler-rt/lib/sanitizer_common/symbolizer/scripts/build_symbolizer.sh
+++ b/compiler-rt/lib/sanitizer_common/symbolizer/scripts/build_symbolizer.sh
@@ -139,7 +139,6 @@ if [[ ! -f ${LLVM_BUILD}/build.ninja ]]; then
     -DLLVM_INCLUDE_TESTS=OFF \
     -DLLVM_ENABLE_ZLIB=ON \
     -DLLVM_ENABLE_ZSTD=OFF \
-    -DLLVM_ENABLE_TERMINFO=OFF \
     -DLLVM_ENABLE_THREADS=OFF \
   $LLVM_SRC
 fi
diff --git a/compiler-rt/lib/xray/tests/CMakeLists.txt b/compiler-rt/lib/xray/tests/CMakeLists.txt
index 0a428b9a30b18..4c7e92b6ecc3d 100644
--- a/compiler-rt/lib/xray/tests/CMakeLists.txt
+++ b/compiler-rt/lib/xray/tests/CMakeLists.txt
@@ -54,11 +54,6 @@ set(XRAY_UNITTEST_LINK_FLAGS
   ${COMPILER_RT_CXX_LINK_LIBS})
 
 if (NOT APPLE)
-  # Needed by LLVMSupport.
-  append_list_if(
-    LLVM_ENABLE_TERMINFO
-    -l${COMPILER_RT_TERMINFO_LIB} XRAY_UNITTEST_LINK_FLAGS)
-
   # We add the library directories one at a time in our CFLAGS.
   foreach (DIR ${LLVM_LIBRARY_DIR})
     list(APPEND XRAY_UNITTEST_LINK_FLAGS -L${DIR})
diff --git a/lldb/docs/resources/build.rst b/lldb/docs/resources/build.rst
index 09d3d15a94083..33b6a6f79def4 100644
--- a/lldb/docs/resources/build.rst
+++ b/lldb/docs/resources/build.rst
@@ -477,7 +477,6 @@ further by passing the appropriate cmake options, such as:
   -DLLDB_ENABLE_PYTHON=0
   -DLLDB_ENABLE_LIBEDIT=0
   -DLLDB_ENABLE_CURSES=0
-  -DLLVM_ENABLE_TERMINFO=0
 
 (see :ref:`Optional Dependencies` for more)
 
diff --git a/lldb/source/Core/CMakeLists.txt b/lldb/source/Core/CMakeLists.txt
index 10525ac39e6ef..f24dbbd45a8e8 100644
--- a/lldb/source/Core/CMakeLists.txt
+++ b/lldb/source/Core/CMakeLists.txt
@@ -11,9 +11,6 @@ set(LLDB_LIBEDIT_LIBS)
 
 if (LLDB_ENABLE_CURSES)
   list(APPEND LLDB_CURSES_LIBS ${PANEL_LIBRARIES} ${CURSES_LIBRARIES})
-  if(LLVM_ENABLE_TERMINFO)
-    list(APPEND LLDB_CURSES_LIBS ${Terminfo_LIBRARIES})
-  endif()
   if (LLVM_BUILD_STATIC)
     list(APPEND LLDB_CURSES_LIBS gpm)
   endif()
diff --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt
index c06e661573ed4..f6279e02eb04b 100644
--- a/llvm/CMakeLists.txt
+++ b/llvm/CMakeLists.txt
@@ -539,8 +539,6 @@ set(FFI_INCLUDE_DIR "" CACHE PATH "Additional directory, where CMake should sear
 set(LLVM_TARGET_ARCH "host"
   CACHE STRING "Set target to use for LLVM JIT or use \"host\" for automatic detection.")
 
-option(LLVM_ENABLE_TERMINFO "Use terminfo database if available." ON)
-
 set(LLVM_ENABLE_LIBXML2 "ON" CACHE STRING "Use libxml2 if available. Can be ON, OFF, or FORCE_ON")
 
 option(LLVM_ENABLE_LIBEDIT "Use libedit if available." ON)
diff --git a/llvm/cmake/config-ix.cmake b/llvm/cmake/config-ix.cmake
index bf1b110245bb2..743d0d3011ad5 100644
--- a/llvm/cmake/config-ix.cmake
+++ b/llvm/cmake/config-ix.cmake
@@ -240,21 +240,11 @@ if(NOT LLVM_USE_SANITIZER MATCHES "Memory.*")
     else()
       set(HAVE_LIBEDIT 0)
     endif()
-    if(LLVM_ENABLE_TERMINFO)
-      if(LLVM_ENABLE_TERMINFO STREQUAL FORCE_ON)
-        find_package(Terminfo REQUIRED)
-      else()
-        find_package(Terminfo)
-      endif()
-      set(LLVM_ENABLE_TERMINFO "${Terminfo_FOUND}")
-    endif()
   else()
     set(HAVE_LIBEDIT 0)
-    set(LLVM_ENABLE_TERMINFO 0)
   endif()
 else()
   set(HAVE_LIBEDIT 0)
-  set(LLVM_ENABLE_TERMINFO 0)
 endif()
 
 # function checks
diff --git a/llvm/cmake/modules/FindTerminfo.cmake b/llvm/cmake/modules/FindTerminfo.cmake
deleted file mode 100644
index 163af66970677..0000000000000
--- a/llvm/cmake/modules/FindTerminfo.cmake
+++ /dev/null
@@ -1,55 +0,0 @@
-# Attempts to discover terminfo library with a linkable setupterm function.
-#
-# Example usage:
-#
-# find_package(Terminfo)
-#
-# If successful, the following variables will be defined:
-# Terminfo_FOUND
-# Terminfo_LIBRARIES
-#
-# Additionally, the following import target will be defined:
-# Terminfo::terminfo
-
-find_library(Terminfo_LIBRARIES NAMES terminfo tinfo curses ncurses ncursesw)
-
-if(Terminfo_LIBRARIES)
-  include(CMakePushCheckState)
-  cmake_push_check_state()
-  list(APPEND CMAKE_REQUIRED_LIBRARIES ${Terminfo_LIBRARIES})
-  set(Terminfo_LINKABLE_SRC [=[
-    #ifdef __cplusplus
-    extern "C" {
-    #endif
-    int setupterm(char *term, int filedes, int *errret);
-    #ifdef __cplusplus
-    }
-    #endif
-    int main(void) { return setupterm(0, 0, 0); }
-    ]=])
-  if(DEFINED CMAKE_C_COMPILER)
-    include(CheckCSourceCompiles)
-    check_c_source_compiles("${Terminfo_LINKABLE_SRC}" Terminfo_LINKABLE)
-  else()
-    include(CheckCXXSourceCompiles)
-    check_cxx_source_compiles("${Terminfo_LINKABLE_SRC}" Terminfo_LINKABLE)
-  endif()
-  cmake_pop_check_state()
-endif()
-
-include(FindPackageHandleStandardArgs)
-find_package_handle_standard_args(Terminfo
-                                  FOUND_VAR
-                                    Terminfo_FOUND
-                                  REQUIRED_VARS
-                                    Terminfo_LIBRARIES
-                                    Terminfo_LINKABLE)
-mark_as_advanced(Terminfo_LIBRARIES
-                 Terminfo_LINKABLE)
-
-if(Terminfo_FOUND)
-  if(NOT TARGET Terminfo::terminfo)
-    add_library(Terminfo::terminfo UNKNOWN IMPORTED)
-    set_target_properties(Terminfo::terminfo PROPERTIES IMPORTED_LOCATION "${Terminfo_LIBRARIES}")
-  endif()
-endif()
diff --git a/llvm/cmake/modules/LLVMConfig.cmake.in b/llvm/cmake/modules/LLVMConfig.cmake.in
index 397bd5815b64e..7e1501a89354c 100644
--- a/llvm/cmake/modules/LLVMConfig.cmake.in
+++ b/llvm/cmake/modules/LLVMConfig.cmake.in
@@ -60,11 +60,6 @@ if(LLVM_ENABLE_LIBEDIT)
   find_package(LibEdit)
 endif()
 
-set(LLVM_ENABLE_TERMINFO @LLVM_ENABLE_TERMINFO@)
-if(LLVM_ENABLE_TERMINFO)
-  find_package(Terminfo)
-endif()
-
 set(LLVM_ENABLE_THREADS @LLVM_ENABLE_THREADS@)
 
 set(LLVM_ENABLE_UNWIND_TABLES @LLVM_ENABLE_UNWIND_TABLES@)
diff --git a/llvm/docs/ReleaseNotes.rst b/llvm/docs/ReleaseNotes.rst
index cba36c7177daa..f9540cd273579 100644
--- a/llvm/docs/ReleaseNotes.rst
+++ b/llvm/docs/ReleaseNotes.rst
@@ -62,6 +62,10 @@ Changes to LLVM infrastructure
 Changes to building LLVM
 ------------------------
 
+- The ``LLVM_ENABLE_TERMINFO`` flag has been removed. LLVM no longer depends on
+  terminfo and now always uses the ``TERM`` environment variable for color
+  support autodetection.
+
 Changes to TableGen
 -------------------
 
diff --git a/llvm/include/llvm/Config/config.h.cmake b/llvm/include/llvm/Config/config.h.cmake
index 977c182e9d2b0..ff30741c8f360 100644
--- a/llvm/include/llvm/Config/config.h.cmake
+++ b/llvm/include/llvm/Config/config.h.cmake
@@ -209,9 +209,6 @@
 /* Define to 1 if you have the <sys/types.h> header file. */
 #cmakedefine HAVE_SYS_TYPES_H ${HAVE_SYS_TYPES_H}
 
-/* Define if the setupterm() function is supported this platform. */
-#cmakedefine LLVM_ENABLE_TERMINFO ${LLVM_ENABLE_TERMINFO}
-
 /* Define to 1 if you have the <termios.h> header file. */
 #cmakedefine HAVE_TERMIOS_H ${HAVE_TERMIOS_H}
 
diff --git a/llvm/lib/Support/CMakeLists.txt b/llvm/lib/Support/CMakeLists.txt
index 03e888958a071..be4badc09efa5 100644
--- a/llvm/lib/Support/CMakeLists.txt
+++ b/llvm/lib/Support/CMakeLists.txt
@@ -56,9 +56,6 @@ elseif( CMAKE_HOST_UNIX )
     STRING(REGEX REPLACE "^lib" "" Backtrace_LIBFILE ${Backtrace_LIBFILE})
     set(system_libs ${system_libs} ${Backtrace_LIBFILE})
   endif()
-  if( LLVM_ENABLE_TERMINFO )
-    set(imported_libs ${imported_libs} Terminfo::terminfo)
-  endif()
   set(system_libs ${system_libs} ${LLVM_ATOMIC_LIB})
   set(system_libs ${system_libs} ${LLVM_PTHREAD_LIB})
   if( UNIX AND NOT (BEOS OR HAIKU) )
@@ -325,14 +322,6 @@ if(LLVM_ENABLE_ZSTD)
   set(llvm_system_libs ${llvm_system_libs} "${zstd_library}")
 endif()
 
-if(LLVM_ENABLE_TERMINFO)
-  if(NOT terminfo_library)
-    get_property(terminfo_library TARGET Terminfo::terminfo PROPERTY LOCATION)
-  endif()
-  get_library_name(${terminfo_library} terminfo_library)
-  set(llvm_system_libs ${llvm_system_libs} "${terminfo_library}")
-endif()
-
 set_property(TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS "${llvm_system_libs}")
 
 
diff --git a/llvm/lib/Support/Unix/Process.inc b/llvm/lib/Support/Unix/Process.inc
index ae90924cae1b9..84b10ff5d1d08 100644
--- a/llvm/lib/Support/Unix/Process.inc
+++ b/llvm/lib/Support/Unix/Process.inc
@@ -341,17 +341,9 @@ unsigned Process::StandardErrColumns() {
   return getColumns();
 }
 
-#ifdef LLVM_ENABLE_TERMINFO
-// We manually declare these extern functions because finding the correct
-// headers from various terminfo, curses, or other sources is harder than
-// writing their specs down.
-extern "C" int setupterm(char *term, int filedes, int *errret);
-extern "C" struct term *set_curterm(struct term *termp);
-extern "C" int del_curterm(struct term *termp);
-extern "C" int tigetnum(char *capname);
-#endif
-
-bool checkTerminalEnvironmentForColors() {
+static bool terminalHasColors() {
+  // Check if the current terminal is one of terminals that are known to support
+  // ANSI color escape codes.
   if (const char *TermStr = std::getenv("TERM")) {
     return StringSwitch<bool>(TermStr)
         .Case("ansi", true)
@@ -368,54 +360,10 @@ bool checkTerminalEnvironmentForColors() {
   return false;
 }
 
-static bool terminalHasColors(int fd) {
-#ifdef LLVM_ENABLE_TERMINFO
-  // First, acquire a global lock because these C routines are thread hostile.
-  static std::mutex TermColorMutex;
-  std::lock_guard<std::mutex> G(TermColorMutex);
-
-  struct term *previous_term = set_curterm(nullptr);
-  int errret = 0;
-  if (setupterm(nullptr, fd, &errret) != 0)
-    // Regardless of why, if we can't get terminfo, we shouldn't try to print
-    // colors.
-    return false;
-
-  // Test whether the terminal as set up supports color output. How to do this
-  // isn't entirely obvious. We can use the curses routine 'has_colors' but it
-  // would be nice to avoid a dependency on curses proper when we can make do
-  // with a minimal terminfo parsing library. Also, we don't really care whether
-  // the terminal supports the curses-specific color changing routines, merely
-  // if it will interpret ANSI color escape codes in a reasonable way. Thus, the
-  // strategy here is just to query the baseline colors capability and if it
-  // supports colors at all to assume it will translate the escape codes into
-  // whatever range of colors it does support. We can add more detailed tests
-  // here if users report them as necessary.
-  //
-  // The 'tigetnum' routine returns -2 or -1 on errors, and might return 0 if
-  // the terminfo says that no colors are supported.
-  int colors_ti = tigetnum(const_cast<char *>("colors"));
-  bool HasColors =
-      colors_ti >= 0 ? colors_ti : checkTerminalEnvironmentForColors();
-
-  // Now extract the structure allocated by setupterm and free its memory
-  // through a really silly dance.
-  struct term *termp = set_curterm(previous_term);
-  (void)del_curterm(termp); // Drop any errors here.
-
-  // Return true if we found a color capabilities for the current terminal.
-  return HasColors;
-#else
-  // When the terminfo database is not available, check if the current terminal
-  // is one of terminals that are known to support ANSI color escape codes.
-  return checkTerminalEnvironmentForColors();
-#endif
-}
-
 bool Process::FileDescriptorHasColors(int fd) {
   // A file descriptor has colors if it is displayed and the terminal has
   // colors.
-  return FileDescriptorIsDisplayed(fd) && terminalHasColors(fd);
+  return FileDescriptorIsDisplayed(fd) && terminalHasColors();
 }
 
 bool Process::StandardOutHasColors() {
diff --git a/llvm/utils/gn/README.rst b/llvm/utils/gn/README.rst
index 9ca545061099d..52d03be533e55 100644
--- a/llvm/utils/gn/README.rst
+++ b/llvm/utils/gn/README.rst
@@ -131,7 +131,7 @@ configure is used for three classes of feature checks:
 
 For the last two points, it would be nice if LLVM didn't have a single
 ``config.h`` header, but one header per toggle. That way, when e.g.
-``llvm_enable_terminfo`` is toggled, only the 3 files caring about that setting
+``llvm_enable_zlib`` is toggled, only the 3 files caring about that setting
 would need to be rebuilt, instead of everything including ``config.h``.
 
 GN doesn't believe in users setting arbitrary cflags from an environment
diff --git a/llvm/utils/gn/build/libs/terminfo/BUILD.gn b/llvm/utils/gn/build/libs/terminfo/BUILD.gn
deleted file mode 100644
index 10003d61c4df9..0000000000000
--- a/llvm/utils/gn/build/libs/terminfo/BUILD.gn
+++ /dev/null
@@ -1,12 +0,0 @@
-import("//llvm/utils/gn/build/libs/terminfo/enable.gni")
-
-config("terminfo_config") {
-  visibility = [ ":terminfo" ]
-  libs = [ "ncurses" ]
-}
-
-group("terminfo") {
-  if (llvm_enable_terminfo) {
-    public_configs = [ ":terminfo_config" ]
-  }
-}
diff --git a/llvm/utils/gn/build/libs/terminfo/enable.gni b/llvm/utils/gn/build/libs/terminfo/enable.gni
deleted file mode 100644
index 79ea2b601857f..0000000000000
--- a/llvm/utils/gn/build/libs/terminfo/enable.gni
+++ /dev/null
@@ -1,4 +0,0 @@
-declare_args() {
-  # Whether to link against terminfo.
-  llvm_enable_terminfo = false
-}
diff --git a/llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn b/llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
index 80a91507fcc69..e93130eacdc74 100644
--- a/llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
+++ b/llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
@@ -10,7 +10,6 @@ import("//llvm/utils/gn/build/buildflags.gni")
 import("//llvm/utils/gn/build/libs/curl/enable.gni")
 import("//llvm/utils/gn/build/libs/edit/enable.gni")
 import("//llvm/utils/gn/build/libs/pthread/enable.gni")
-import("//llvm/utils/gn/build/libs/terminfo/enable.gni")
 import("//llvm/utils/gn/build/libs/xar/enable.gni")
 import("//llvm/utils/gn/build/libs/xml/enable.gni")
 import("//llvm/utils/gn/build/libs/zlib/enable.gni")
@@ -294,12 +293,6 @@ write_cmake_config("config") {
     values += [ "HAVE_LIBEDIT=" ]
   }
 
-  if (llvm_enable_terminfo) {
-    values += [ "LLVM_ENABLE_TERMINFO=1" ]
-  } else {
-    values += [ "LLVM_ENABLE_TERMINFO=" ]
-  }
-
   if (llvm_enable_libxml2) {
     values += [ "LLVM_ENABLE_LIBXML2=1" ]
   } else {
diff --git a/llvm/utils/gn/secondary/llvm/lib/Support/BUILD.gn b/llvm/utils/gn/secondary/llvm/lib/Support/BUILD.gn
index 941d448b3367c..7728455499bf3 100644
--- a/llvm/utils/gn/secondary/llvm/lib/Support/BUILD.gn
+++ b/llvm/utils/gn/secondary/llvm/lib/Support/BUILD.gn
@@ -6,7 +6,6 @@ static_library("Support") {
     "//llvm/include/llvm/Support:write_vcsrevision",
     "//llvm/lib/Demangle",
     "//llvm/utils/gn/build/libs/pthread",
-    "//llvm/utils/gn/build/libs/terminfo",
     "//llvm/utils/gn/build/libs/zlib",
   ]
 
diff --git a/llvm/utils/gn/secondary/llvm/tools/llvm-config/BUILD.gn b/llvm/utils/gn/secondary/llvm/tools/llvm-config/BUILD.gn
index bf50cd0fce46b..711e4e3b43151 100644
--- a/llvm/utils/gn/secondary/llvm/tools/llvm-config/BUILD.gn
+++ b/llvm/utils/gn/secondary/llvm/tools/llvm-config/BUILD.gn
@@ -1,7 +1,6 @@
 import("//llvm/lib/Target/targets_string.gni")
 import("//llvm/utils/gn/build/buildflags.gni")
 import("//llvm/utils/gn/build/libs/pthread/enable.gni")
-import("//llvm/utils/gn/build/libs/terminfo/enable.gni")
 import("//llvm/utils/gn/build/libs/xml/enable.gni")
 import("//llvm/utils/gn/build/libs/zlib/enable.gni")
 import("//llvm/utils/gn/build/write_cmake_config.gni")
@@ -36,7 +35,7 @@ write_cmake_config("BuildVariables.inc") {
     lib = ""
   }
 
-  # Windows doesn't use any of libxml2, terminfo, zlib by default.
+  # Windows doesn't use any of libxml2, zlib by default.
   # Make GN not warn about these variables being unused.
   not_needed([
                "l",
@@ -63,9 +62,6 @@ write_cmake_config("BuildVariables.inc") {
   if (llvm_enable_libxml2) {
     system_libs += " ${l}xml2${lib}"
   }
-  if (llvm_enable_terminfo) {
-    system_libs += " ${l}ncurses${lib}"
-  }
   if (llvm_enable_zlib) {
     system_libs += " ${l}z${lib}"
   }
diff --git a/utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h b/utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
index e9385f45c5e5c..a4fb47d677ab1 100644
--- a/utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
+++ b/utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
@@ -222,9 +222,6 @@
 /* Define to 1 if you have the <sys/types.h> header file. */
 #define HAVE_SYS_TYPES_H 1
 
-/* Define if the setupterm() function is supported this platform. */
-/* LLVM_ENABLE_TERMINFO defined in Bazel */
-
 /* Define to 1 if you have the <termios.h> header file. */
 #define HAVE_TERMIOS_H ...
[truncated]

@keith
Copy link
Member

keith commented May 21, 2024

looks like the .bazelrc still has mentions of this

@JDevlieghere
Copy link
Member

No objections in the context of LLDB. We don't use terminfo directly (although I think editline does, but that isn't affected by this) and if we want the TUI we depend on curses anyway.

The terminfo dependency introduces a significant nonhermeticity into the
build. It doesn't respect `--no-undefined-version` meaning that it's not
a dependency that can be built with Clang 17+. This forces maintainers
of source-based distributions to implement patches or ignore linker
errors.

Remove it to reduce the closure size and improve portability of
LLVM-based tools. Users can still use command line arguments to toggle
color support expliticly.

Fixes llvm#75490
Closes llvm#53294 llvm#23355
@aaronmondal aaronmondal force-pushed the remove-nonhermetic-terminfo branch from 404dcfc to 124be68 Compare May 23, 2024 06:05
@aaronmondal aaronmondal merged commit 6bf450c into llvm:main May 24, 2024
4 checks passed
@gulfemsavrun
Copy link
Contributor

gulfemsavrun commented May 27, 2024

We started seeing the following issue after this patch in our Clang toolchain builders:

[1448/1517](45) Linking CXX executable unittests/LineEditor/LineEditorTests
FAILED: unittests/LineEditor/LineEditorTests 
: && /b/s/w/ir/x/w/cipd/bin/clang++ --sysroot=/b/s/w/ir/x/w/cipd/linux -stdlib=libc++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -ffat-lto-objects -ffile-prefix-map=/b/s/w/ir/x/w/llvm_build=../llvm-llvm-project -ffile-prefix-map=/b/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes -O3 -DNDEBUG -static-libstdc++ -stdlib=libc++ -static-libstdc++ -fuse-ld=lld -Wl,--color-diagnostics -ffat-lto-objects    -Wl,--gc-sections unittests/LineEditor/CMakeFiles/LineEditorTests.dir/LineEditor.cpp.o -o unittests/LineEditor/LineEditorTests  lib/libLLVMLineEditor.a  lib/libLLVMSupport.a  lib/libLLVMSupport.a  -lpthread  lib/libllvm_gtest_main.a  lib/libllvm_gtest.a  -lpthread  /b/s/w/ir/x/w/libedit_install/lib/libedit.a  lib/libLLVMSupport.a  -lrt  -ldl  -lpthread  -lm  /b/s/w/ir/x/w/zlib_install_target/lib/libz.a  /b/s/w/ir/x/w/zstd_install/lib/libzstd.a  -pthread  lib/libLLVMDemangle.a  -lpthread && :
ld.lld: error: undefined symbol: tgetent
>>> referenced by terminal.c
>>>               terminal.o:(terminal_set) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8747030306803673361/+/u/clang/test/stdout

@aaronmondal
Copy link
Member Author

@gulfemsavrun The file terminal.c that is causing this linker error is not in LLVM but in libedit. The configure step doesn't find libedit:

Not searching for unused variables given on the command line.
-- Could NOT find LibEdit (missing: LibEdit_INCLUDE_DIRS LibEdit_LIBRARIES) 
-- Could NOT find ZLIB (missing: ZLIB_LIBRARY ZLIB_INCLUDE_DIR) 
-- Could NOT find zstd (missing: zstd_LIBRARY zstd_INCLUDE_DIR) 
-- Could NOT find LibXml2 (missing: LIBXML2_LIBRARY LIBXML2_INCLUDE_DIR) 
-- Could NOT find CURL (missing: CURL_LIBRARY CURL_INCLUDE_DIR) 

...

-- Could NOT find LibEdit (missing: LibEdit_INCLUDE_DIRS LibEdit_LIBRARIES) 
-- Could NOT find ZLIB (missing: ZLIB_LIBRARY ZLIB_INCLUDE_DIR) 
-- Could NOT find zstd (missing: zstd_LIBRARY zstd_INCLUDE_DIR) 
-- Could NOT find LibXml2 (missing: LIBXML2_LIBRARY LIBXML2_INCLUDE_DIR) 
-- Could NOT find CURL (missing: CURL_LIBRARY CURL_INCLUDE_DIR) 

...

-- Building with -fPIC
-- LLVM host triple: x86_64-unknown-linux-gnu
-- LLVM default target triple: x86_64-unknown-linux-gnu
-- Using libunwind testing configuration: /b/s/w/ir/x/w/llvm-llvm-project/libunwind/test/configs/llvm-libunwind-static.cfg.in
-- Failed to locate sphinx-build executable (missing: SPHINX_EXECUTABLE) 
-- Using libc++abi testing configuration: /b/s/w/ir/x/w/llvm-llvm-project/libcxxabi/test/configs/llvm-libc++abi-static.cfg.in
-- Using libc++ testing configuration: /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/configs/llvm-libc++-static.cfg.in
-- Could NOT find LibEdit (missing: LibEdit_INCLUDE_DIRS LibEdit_LIBRARIES) 
-- Could NOT find ZLIB (missing: ZLIB_LIBRARY ZLIB_INCLUDE_DIR) 
-- Could NOT find zstd (missing: zstd_LIBRARY zstd_INCLUDE_DIR) 
-- Could NOT find LibXml2 (missing: LIBXML2_LIBRARY LIBXML2_INCLUDE_DIR) 

My guess is that some other mechanism is adding libedit into this build with an explicit -ledit flag instead of using pkgconfig. This causing the /usr/lib64/pkgconfig/libedit.pc (or similar) file to be ignored. That file should contain a line like Libs.private: -ltinfo if it's a libedit that was configured to depend on tinfo.

FAILED: unittests/LineEditor/LineEditorTests 
: && /b/s/w/ir/x/w/cipd/bin/clang++ --sysroot=/b/s/w/ir/x/w/cipd/linux -stdlib=libc++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -ffat-lto-objects -ffile-prefix-map=/b/s/w/ir/x/w/llvm_build=../llvm-llvm-project -ffile-prefix-map=/b/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes -O3 -DNDEBUG -static-libstdc++ -stdlib=libc++ -static-libstdc++ -fuse-ld=lld -Wl,--color-diagnostics -ffat-lto-objects    -Wl,--gc-sections unittests/LineEditor/CMakeFiles/LineEditorTests.dir/LineEditor.cpp.o -o unittests/LineEditor/LineEditorTests  lib/libLLVMLineEditor.a  lib/libLLVMSupport.a  lib/libLLVMSupport.a  -lpthread  lib/libllvm_gtest_main.a  lib/libllvm_gtest.a  -lpthread  /b/s/w/ir/x/w/libedit_install/lib/libedit.a  lib/libLLVMSupport.a  -lrt  -ldl  -lpthread  -lm  /b/s/w/ir/x/w/zlib_install_target/lib/libz.a  /b/s/w/ir/x/w/zstd_install/lib/libzstd.a  -pthread  lib/libLLVMDemangle.a  -lpthread && :
ld.lld: error: undefined symbol: tgetent
>>> referenced by terminal.c
>>>               terminal.o:(terminal_set) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a

ld.lld: error: undefined symbol: tgetflag
>>> referenced by terminal.c
>>>               terminal.o:(terminal_set) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a
>>> referenced by terminal.c
>>>               terminal.o:(terminal_set) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a
>>> referenced by terminal.c
>>>               terminal.o:(terminal_set) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a
>>> referenced 3 more times

ld.lld: error: undefined symbol: tgetnum
>>> referenced by terminal.c
>>>               terminal.o:(terminal_set) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a
>>> referenced by terminal.c
>>>               terminal.o:(terminal_set) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a

ld.lld: error: undefined symbol: tgetstr
>>> referenced by terminal.c
>>>               terminal.o:(terminal_set) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a
>>> referenced by terminal.c
>>>               terminal.o:(terminal_echotc) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a

ld.lld: error: undefined symbol: tputs
>>> referenced by terminal.c
>>>               terminal.o:(terminal_move_to_line) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a
>>> referenced by terminal.c
>>>               terminal.o:(terminal_move_to_line) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a
>>> referenced by terminal.c
>>>               terminal.o:(terminal_move_to_char) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a
>>> referenced 16 more times

ld.lld: error: undefined symbol: tgoto
>>> referenced by terminal.c
>>>               terminal.o:(terminal_move_to_line) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a
>>> referenced by terminal.c
>>>               terminal.o:(terminal_move_to_char) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a
>>> referenced by terminal.c
>>>               terminal.o:(terminal_move_to_char) in archive /b/s/w/ir/x/w/libedit_install/lib/libedit.a
>>> referenced 5 more times

If this setup doesn't make use of pkgconfig the way to fix this would be to either use a libedit that doesn't depend on tinfo, or to add tinfo as a dependency to the libedit_install target. In both cases this would be configuration outside of the LLVM build graph.

The previous tinfo dependency in LLVM hid this misconfiguration and happened to satisfy the libedit setup by chance.

@gulfemsavrun
Copy link
Contributor

Ok, thanks for your analysis @aaronmondal!

@mysterymath
Copy link
Contributor

I did some brief investigation on our end. The pkg-config file for libedit includes a private dependency on libncurses, which provides the terminfo functions. The issue here seems to be one uncovered in LLVM's FindLibEdit.cmake. We're building against a static libedit, but that doesn't use the LibEdit_STATIC_LIBRARIES variable that would be aware of the private dependency of libedit on libncurses. We build everything as statically as we can, which is probably why we're the first to notice. @petrhosek mentioned that we've had to deal with similar concerns with ZSTD.

I'll take a look at seeing if there's a straightforward to update FindLibEdit.cmake to handle this case.

@jimingham
Copy link
Collaborator

This patch has been causing the Darwin bots on GreenDragon to fail consistently since it was submitted.

The problem is that the lldb/unitest/EditLine needs setupterm which comes from libcurses.dylib. However that was finding its way into this build line for this test, it's no longer doing so on Darwin, so the test fails to compile. I can fix this by adding curses explicitly to the LINK_LIBS for this directory, but that's clearly the wrong way to do it. I thought LLDB_CURSES_LIBS would be the right thing, but at present that's empty.

The file that uses this setupterm is in Host code (not part of this test) but the flags for building LLDB.framework are correct. This seems to be specific to this unit test.

@JDevlieghere
Copy link
Member

LLDB_CURSES_LIBS is scoped to lldbCore, which the EditLine unit test isn't linking. If its also necessary in Host, then we should hoist it up and link both libraries against it.

Anyway, that also means my initial assessment was wrong. I looked for the header include but the code in our editline wrapper forward declares setupterm which explains why I missed it. Maybe we should reconsider? Anyway, if I cannot figure this out in the next hour or so, I think we should revert this to get our bots green again.

@Michael137
Copy link
Member

Michael137 commented May 29, 2024

I'm going to revert this for now since the bots have been red for a while now

Michael137 added a commit that referenced this pull request May 29, 2024
Michael137 added a commit that referenced this pull request May 29, 2024
This reverts commit 6bf450c.

It breaks LLDB CI: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/4762/execution/node/97/log/

```
/Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -Wdocumentation -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-deprecated-register -Wno-vla-extension -O3 -DNDEBUG -arch arm64 -isysroot /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk -mmacosx-version-min=14.1 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -Wl,-dead_strip -Wl,-no_warn_duplicate_libraries tools/lldb/unittests/Editline/CMakeFiles/EditlineTests.dir/EditlineTest.cpp.o -o tools/lldb/unittests/Editline/EditlineTests  lib/libLLVMSupport.a  lib/libllvm_gtest_main.a  lib/libllvm_gtest.a  lib/liblldbHost.a  lib/liblldbUtility.a  lib/libLLVMTestingSupport.a  /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/libxml2.tbd  /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/libedit.tbd  lib/liblldbHostMacOSXObjCXX.a  lib/liblldbUtility.a  -framework Foundation  -framework CoreFoundation  -framework CoreServices  -framework Security  lib/libLLVMObject.a  lib/libLLVMIRReader.a  lib/libLLVMBitReader.a  lib/libLLVMAsmParser.a  lib/libLLVMCore.a  lib/libLLVMRemarks.a  lib/libLLVMBitstreamReader.a  lib/libLLVMMCParser.a  lib/libLLVMMC.a  lib/libLLVMDebugInfoCodeView.a  lib/libLLVMTextAPI.a  lib/libLLVMBinaryFormat.a  lib/libLLVMTargetParser.a  lib/libllvm_gtest.a  lib/libLLVMSupport.a  -lm  /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/libz.tbd  /opt/homebrew/lib/libzstd.dylib  lib/libLLVMDemangle.a  -lpthread && cd /Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/tools/lldb/unittests/Editline && /opt/homebrew/Cellar/cmake/3.28.3/bin/cmake -E make_directory /Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/tools/lldb/unittests/Editline/./Inputs
ld: Undefined symbols:
  _setupterm, referenced from:
      lldb_private::Editline::Editline(char const*, __sFILE*, __sFILE*, __sFILE*, std::__1::recursive_mutex&) in liblldbHost.a[35](Editline.cpp.o)
clang: error: linker command failed with exit code 1 (use -v to see invocation)
```
aaronmondal added a commit to aaronmondal/llvm-project that referenced this pull request May 30, 2024
This reverts commit fe82a3d.

This broke LLDB on MacOS due to a missing symbol during linking.

The fix has been applied in c6c08ee.
aaronmondal added a commit that referenced this pull request May 30, 2024
This reverts commit fe82a3d.

This broke LLDB on MacOS due to a missing symbol during linking.

The fix has been applied in c6c08ee.

Original commit message:

The terminfo dependency introduces a significant nonhermeticity into the
build. It doesn't respect `--no-undefined-version` meaning that it's not
a dependency that can be built with Clang 17+. This forces maintainers
of source-based distributions to implement patches or ignore linker
errors.

Remove it to reduce the closure size and improve portability of
LLVM-based tools. Users can still use command line arguments to toggle
color support expliticly.

Fixes #75490
Closes #53294 #23355
@bernhardkaindl
Copy link

@JDevlieghere, you wrote:

No objections in the context of LLDB. We don't use terminfo directly (although I think editline does, but that isn't affected by this) and if we want the TUI we depend on curses anyway.

You truly use libtinfo directly in LLDB:

# git grep define_key
lldb/source/Core/IOHandlerCursesGUI.cpp:    define_key("\033[Z", KEY_SHIFT_TAB);
lldb/source/Core/IOHandlerCursesGUI.cpp:    define_key("\033\015", KEY_ALT_ENTER);
# git grep stdscr
lldb/source/Core/IOHandlerCursesGUI.cpp:          ::touchwin(stdscr);
lldb/source/Core/IOHandlerCursesGUI.cpp:      ::touchwin(stdscr);
lldb/source/Core/IOHandlerCursesGUI.cpp:    ::keypad(stdscr, TRUE);
lldb/source/Core/IOHandlerCursesGUI.cpp:      m_window_sp = std::make_shared<Window>("main", stdscr, false);
lldb/source/Core/IOHandlerCursesGUI.cpp:    touchwin(stdscr);
lldb/source/Core/IOHandlerCursesGUI.cpp:      touchwin(stdscr);
lldb/source/Core/IOHandlerCursesGUI.cpp:      touchwin(stdscr);
lldb/source/Core/IOHandlerCursesGUI.cpp:      touchwin(stdscr);
# nm -D /usr/lib/x86_64-linux-gnu/libtinfo.so |grep stdscr
00000000000318e0 B stdscr@@NCURSES6_TINFO_5.0.19991023

# grep define_key /usr/lib/x86_64-linux-gnu/lib{ncurses,tinfo}.*
grep: /usr/lib/x86_64-linux-gnu/libtinfo.a: binary file matches
grep: /usr/lib/x86_64-linux-gnu/libtinfo.so: binary file matches
grep: /usr/lib/x86_64-linux-gnu/libtinfo.so.5: binary file matches
grep: /usr/lib/x86_64-linux-gnu/libtinfo.so.5.9: binary file matches
grep: /usr/lib/x86_64-linux-gnu/libtinfo.so.6: binary file matches
grep: /usr/lib/x86_64-linux-gnu/libtinfo.so.6.3: binary file matches

When not adding libtinfo into the link directly on a https://github.com/spack/spack build using gcc-11.4.0/binutils-2.42, with ncurses-6.5, I'm getting undefined references to those.

@JDevlieghere
Copy link
Member

What I said was that, to the best of my knowledge, we don't have places in LLDB that rely on terminfo but not on ncruses. I think that statement still holds true. The assumption behind it was that ncurses provides a superset of the functionality provided by terminfo. At least on macOS, ncruses exports stdscr and define_key. What you're showing, that doesn't appear to be the case on your platform. It would be reasonable to special case that and link terminfo if not all symbols are provided by ncurses.

@bernhardkaindl
Copy link

Thanks @JDevlieghere! Could you add this special case?

The authoritatively established and supported way to link with ncurses on most Linux platforms would be to use pkg-config --libs ncurses to the LDFLAGS for ncurses for the given compile target.

@JDevlieghere
Copy link
Member

Thanks @JDevlieghere! Could you add this special case?

I'm happy to help but I wouldn't feel comfortable making such a change without being able to reproduce and test it.

The authoritatively established and supported way to link with ncurses on most Linux platforms would be to use pkg-config --libs ncurses to the LDFLAGS for ncurses for the given compile target.

I'm confused, I thought the problem was that we are linking ncurses and that's not sufficient because it doesn't export all the symbols. Wouldn't the fix consist of linking terminfo, in addition to ncurses on the platform you're observing this on? I presume this isn't an issue on all Linux systems as we have bots running Ubuntu that do not have this issue.

@davide-q
Copy link

Is there a way to revert and rework this PR to avoid issue #101368 which this PR introduced?

searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Sep 19, 2024
The terminfo dependency introduces a significant nonhermeticity into the
build. It doesn't respect `--no-undefined-version` meaning that it's not
a dependency that can be built with Clang 17+. This forces maintainers
of source-based distributions to implement patches or ignore linker
errors.

Remove it to reduce the closure size and improve portability of
LLVM-based tools. Users can still use command line arguments to toggle
color support expliticly.

Fixes llvm#75490
Closes llvm#53294 llvm#23355

Change-Id: Ib4a5c725f845d77780472b7608fa587c1ad3ead6
@bernhardkaindl
Copy link

bernhardkaindl commented Sep 25, 2024

@davide-q: The patch in this comment avoids this issue #101368: spack/spack#46504 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel clang:static analyzer clang Clang issues not falling into any other category cmake Build system in general and CMake in particular compiler-rt:sanitizer compiler-rt lldb llvm:support xray
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release builds dynamically linked to missing libtinfo.so.5 llvm-config does not respect LLVM_ENABLE_TERMINFO