Skip to content

[lldb] Fix manual CURSES_LIBRARIES tinfo finding #128245

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
Feb 22, 2025

Conversation

ajordanr-google
Copy link
Contributor

@ajordanr-google ajordanr-google commented Feb 21, 2025

At present, we automatically detect terminfo symbols in CURSES_LIBRARIES after it is found through find_package. However, by introducing a check for TINFO_LIBRARIES, we break systems which pass all of CURSES_INCLUDE_DIRS, CURSES_LIBRARIES, and PANEL_LIBRARIES individually without passing TINFO_LIBRARIES. We'd rather not expose TINFO_LIBRARIES at all.

This commit preemptively attempts to fix issues encountered on systems that manually pass CURSES_LIBRARIES which already contain the necessary terminfo symbols (e.g. 'acs_map').

See this breakage in Google Fuchsia:
https://issues.fuchsia.dev/397455029

References Issue #101368

@llvmbot
Copy link
Member

llvmbot commented Feb 21, 2025

@llvm/pr-subscribers-lldb

Author: Jordan R AW (ajordanr-google)

Changes

At present, we automatically detect terminfo symbols in CURSES_LIBRARIES after it is found through find_package. However, by introducing a check for TINFO_LIBRARIES, we break systems which pass all of CURSES_INCLUDE_DIRS, CURSES_LIBRARIES, and PANEL_LIBRARIES individually without passing TINFO_LIBRARIES. We'd rather not expose TINFO_LIBRARIES at all.

This commit preemptively attempts to fix issues encountered on systems that manually pass CURSES_LIBRARIES which already contain the necessary terminfo symbols (e.g. 'acs_map').

Additionally, let's not make CURSES_LIBRARIES a list. That was odd to begin with.

See this breakage in Google Fuchsia:
https://issues.fuchsia.dev/397455029

References Issue #101368


Full diff: https://github.com/llvm/llvm-project/pull/128245.diff

1 Files Affected:

  • (modified) lldb/cmake/modules/FindCursesAndPanel.cmake (+15-4)
diff --git a/lldb/cmake/modules/FindCursesAndPanel.cmake b/lldb/cmake/modules/FindCursesAndPanel.cmake
index 75ebaa35d7ea1..315810c87b67b 100644
--- a/lldb/cmake/modules/FindCursesAndPanel.cmake
+++ b/lldb/cmake/modules/FindCursesAndPanel.cmake
@@ -6,15 +6,24 @@
 
 include(CMakePushCheckState)
 
-function(lldb_check_curses_tinfo CURSES_LIBRARIES CURSES_HAS_TINFO)
+function(lldb_check_curses_tinfo CURSES_HEADER CURSES_LIBRARIES CURSES_HAS_TINFO)
   cmake_reset_check_state()
   set(CMAKE_REQUIRED_LIBRARIES "${CURSES_LIBRARIES}")
   # acs_map is one of many symbols that are part of tinfo but could
   # be bundled in curses.
-  check_symbol_exists(acs_map "curses.h" CURSES_HAS_TINFO)
+  check_symbol_exists(acs_map "${CURSES_HEADER}" CURSES_HAS_TINFO)
 endfunction()
 
-if(CURSES_INCLUDE_DIRS AND CURSES_LIBRARIES AND TINFO_LIBRARIES AND PANEL_LIBRARIES)
+if(CURSES_INCLUDE_DIRS AND CURSES_LIBRARIES AND PANEL_LIBRARIES)
+  if(NOT HAS_TERMINFO_SYMBOLS)
+    lldb_check_curses_tinfo("${CURSES_INCLUDE_DIRS}/curses.h"
+                            "${CURSES_LIBRARIES}"
+                            CURSES_HAS_TINFO)
+    if (NOT CURSES_HAS_TINFO)
+      message(WARNING "CURSES_LIBRARIES was provided manually but is missing terminfo symbols")
+    endif()
+    mark_as_advanced(CURSES_HAS_TINFO)
+  endif()
   set(CURSESANDPANEL_FOUND TRUE)
 else()
   find_package(Curses QUIET)
@@ -25,7 +34,9 @@ else()
     # Sometimes the curses libraries define their own terminfo symbols,
     # other times they're extern and are defined by a separate terminfo library.
     # Auto-detect which.
-    lldb_check_curses_tinfo("${CURSES_LIBRARIES}" CURSES_HAS_TINFO)
+    lldb_check_curses_tinfo("${CURSES_INCLUDE_DIRS}/curses.h"
+                            "${CURSES_LIBRARIES}"
+                            CURSES_HAS_TINFO)
     if (NOT CURSES_HAS_TINFO)
       message(STATUS "curses library missing terminfo symbols, looking for tinfo separately")
       find_library(TINFO_LIBRARIES NAMES tinfo DOC "The curses tinfo library" QUIET)

At present, we automatically detect terminfo symbols in CURSES_LIBRARIES
after it is found through `find_package`. However, by introducing a
check for TINFO_LIBRARIES, we break systems which pass all of
CURSES_INCLUDE_DIRS, CURSES_LIBRARIES, and PANEL_LIBRARIES individually
without passing TINFO_LIBRARIES. We'd rather not expose TINFO_LIBRARIES
at all.

This commit preemptively attempts to fix issues encountered
on systems that manually pass CURSES_LIBRARIES which already
contain the necessary terminfo symbols (e.g. 'acs_map').

Additionally, let's not make CURSES_LIBRARIES a list. That was odd
to begin with.

See this breakage in Google Fuchsia:
https://issues.fuchsia.dev/397455029

References Issue llvm#101368
@ajordanr-google
Copy link
Contributor Author

Rebasing to fix merge conflict...

@ajordanr-google ajordanr-google force-pushed the ajordanr/terminfo-fix branch 2 times, most recently from 9b67d21 to f0d81ca Compare February 21, 2025 23:59
Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JDevlieghere JDevlieghere merged commit bb6a273 into llvm:main Feb 22, 2025
13 of 16 checks passed
ajordanr-google added a commit to ajordanr-google/llvm-project that referenced this pull request Feb 28, 2025
swift-ci pushed a commit to swiftlang/llvm-project that referenced this pull request Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants