Skip to content

cmake: link terminfo to cpptest/tvm_allvisible when required by LLVM/libedit #18700

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.

Sign up for GitHub

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

Open
at445 wants to merge 2 commits into
base: main
Choose a base branch
from
+11 −0
Open

cmake: link terminfo to cpptest/tvm_allvisible when required by LLVM/libedit #18700

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8987ced
cmake: link terminfo to cpptest/tvm_allvisible when required by LLVM/…
Jan 31, 2026
4c8743f
1. move terminfo linkage into the if(DEFINED LLVM_LIBS)\n 2.replace t…
Feb 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,18 @@ if(GTEST_FOUND)
if(DEFINED LLVM_SO OR HIDE_PRIVATE_SYMBOLS)
target_link_libraries(cpptest PRIVATE ${LLVM_LIBS})
endif()

# Ensure we link terminfo when required by LLVM / libedit at link time.
find_package(Curses)
if(CURSES_FOUND)
message(STATUS "Found Curses/terminfo library: ${CURSES_LIBRARIES}")
Copy link
Member

@tqchen tqchen Feb 3, 2026

Choose a reason for hiding this comment

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

i think llvm only requires tinfo but not curses library? we do want to avoid dependency bloat in this case

Copy link
Member

@tqchen tqchen Feb 3, 2026

Choose a reason for hiding this comment

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

The dependency should also be specified and captured through llvm config in findllm instead of here

Copy link
Author

@at445 at445 Feb 4, 2026

Choose a reason for hiding this comment

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

i think llvm only requires tinfo but not curses library? we do want to avoid dependency bloat in this case
this is suggested by gemini-code-assist, but I could switch back tinfo if you insist ^_^

image

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

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

You're right, tqchen raises a good point about dependency bloat. While find_package(Curses) often works because libtinfo is frequently bundled with ncurses, explicitly searching for libtinfo is more precise if that's the only required dependency. This helps avoid pulling in unnecessary parts of the Curses library.

My initial suggestion aimed for a common, robust way to find terminal-related libraries, but find_library(TINFO_LIB tinfo) directly addresses the specific missing dependency (libtinfo) as indicated by your problem description. This is a medium severity suggestion to improve precision and minimize dependencies.

Regarding tqchen's second point, ideally, llvm-config should provide all necessary linker flags for its dependencies. If it doesn't include -ltinfo on certain systems, then explicitly finding and linking it as a workaround is a pragmatic approach for now. The long-term solution would involve ensuring FindLLVM.cmake or the llvm-config output itself correctly reports this dependency.

Suggested change
message(STATUS "Found Curses/terminfo library: ${CURSES_LIBRARIES}")
find_library(TINFO_LIB tinfo)
if(TINFO_LIB)
message(STATUS "Found terminfo library: ${TINFO_LIB}")
target_link_libraries(cpptest PRIVATE ${TINFO_LIB})
if(TARGET tvm_allvisible)
target_link_libraries(tvm_allvisible PRIVATE ${TINFO_LIB})
endif()
endif()

Copy link
Author

@at445 at445 Feb 4, 2026

Choose a reason for hiding this comment

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

The dependency should also be specified and captured through llvm config in findllm instead of here

you are right! I will rethink about it

target_link_libraries(cpptest PRIVATE ${CURSES_LIBRARIES})
if(TARGET tvm_allvisible)
target_link_libraries(tvm_allvisible PRIVATE ${CURSES_LIBRARIES})
endif()
endif()
endif()

set_target_properties(cpptest PROPERTIES EXCLUDE_FROM_ALL 1)
set_target_properties(cpptest PROPERTIES EXCLUDE_FROM_DEFAULT_BUILD 1)
target_compile_definitions(cpptest PRIVATE "NDEBUG")
Expand Down