Page MenuHomePhabricator

Use find_library for ncurses
ClosedPublic

Authored by haampie on Aug 12 2020, 2:00 AM.

Details

Summary

Currently it is hard to avoid having LLVM link to the system install of ncurses, since it uses check_library_exists to find e.g. libtinfo and not find_library or find_package.

With this change the ncurses lib is found with find_library, which also considers CMAKE_PREFIX_PATH. This solves an issue for the spack package manager, where we want to use the zlib installed by spack, and spack provides the CMAKE_PREFIX_PATH for it.

This is a similar change as https://reviews.llvm.org/D79219, which just landed in master.

Diff Detail

Event Timeline

haampie created this revision.Aug 12 2020, 2:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2020, 2:00 AM
haampie requested review of this revision.Aug 12 2020, 2:00 AM
haampie edited the summary of this revision. (Show Details)Aug 12 2020, 2:09 AM
JDevlieghere added inline comments.Aug 12 2020, 8:50 AM
llvm/cmake/config-ix.cmake
156–161

You could simplify this a little by using TERMINFO_LIBS directly.

haampie updated this revision to Diff 285161.Aug 12 2020, 12:39 PM

Simplified detection a bit

llvm/cmake/config-ix.cmake
156–161

Thanks, I made it even simpler

CMake already has FindCurses module so we could consider using find_package(Curses), unfortunately it doesn't define the imported target. I'd also prefer replacing HAVE_TERMINFO with LLVM_ENABLE_TERMINFO everywhere for consistency with zlib or libxml2 (see D84563).

I also considered set(CURSES_NEED_NCURSES true) and then find_package(Curses), but indeed CURSES_LIBRARIES is the only thing to work with, which lists too many libraries. I think find_library is cleanest. Thanks for the LLVM_ENABLE_TERMINFO, I can add that change.

haampie updated this revision to Diff 285307.Aug 13 2020, 3:19 AM

Use LLVM_ENABLE_TERMINFO and make it respect FORCE_ON.

Also add the same logic to compiler-rt, which was still using check_library_exists.

haampie added inline comments.Aug 13 2020, 3:42 AM
lldb/source/Core/CMakeLists.txt
14–16

I wonder what the logic is / should be here. lldbCore links against Support already, which includes terminfo when LLVM_ENABLE_TERMINFO was set. Should I just remove if(LLVM_ENABLE_TERMINFO) here?

haampie marked an inline comment as done and an inline comment as not done.Aug 13 2020, 3:43 AM
JDevlieghere added inline comments.Aug 13 2020, 9:01 AM
llvm/cmake/config-ix.cmake
167

If LLVM_ENABLE_TERMINFO is set you should pass REQUIRED to find_library and have CMake print the error rather than dealing with the error handling ourself.

haampie updated this revision to Diff 285592.Aug 14 2020, 2:09 AM

use REQUIRED when FORCE_ON

haampie marked an inline comment as done.Aug 14 2020, 2:09 AM
JDevlieghere accepted this revision.Aug 14 2020, 9:12 AM

One more comment about the duplicated find_library line, but otherwise this LGTM.

llvm/cmake/config-ix.cmake
160

Sorry to keep bothering you with these nits, but you could simplify this a bit, either by moving terminfo tinfo curses ncurses ncursesw into a variable and using that in the repeated find_library statement, or better, you could set a variable like this:

if(LLVM_ENABLE_TERMINFO STREQUAL FORCE_ON)
  set(MAYBE_REQUIRED REQUIRED)
else()
  set(MAYBE_REQUIRED)        
endif()
if (LLVM_ENABLE_TERMINFO)
  find_library(TERMINFO_LIB NAMES terminfo tinfo curses ncurses ncursesw ${MAYBE_REQUIRED})
endif()
This revision is now accepted and ready to land.Aug 14 2020, 9:12 AM
haampie updated this revision to Diff 285883.Aug 16 2020, 12:41 AM

DRY in find_library

haampie marked an inline comment as done.Aug 16 2020, 12:42 AM
JDevlieghere accepted this revision.Aug 16 2020, 12:59 PM
phosek accepted this revision.Aug 17 2020, 11:01 AM

LGTM

Let me know if you need me to land this for you.

Let me know if you need me to land this for you.

That would be great @JDevlieghere :) is there anything you need from me?

This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 17 2020, 7:56 PM
Herald added subscribers: lldb-commits, Restricted Project. · View Herald Transcript

It looks like this change is causing a "regression" for MLIR tests:

https://mlir.llvm.org/getting_started/

git clone https://github.com/llvm/llvm-project.git
mkdir llvm-project/build
cd llvm-project/build
cmake -G Ninja ../llvm \
   -DLLVM_ENABLE_PROJECTS=mlir \
   -DLLVM_BUILD_EXAMPLES=ON \
   -DLLVM_TARGETS_TO_BUILD="X86;NVPTX;AMDGPU" \
   -DCMAKE_BUILD_TYPE=Release \
   -DLLVM_ENABLE_ASSERTIONS=ON \
#  -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_ENABLE_LLD=ON

cmake --build . --target check-mlir

Build results:

********************
********************
Failed Tests (1):
  MLIR :: Examples/standalone/test.toy


Testing Time: 17.98s
  Unsupported:  22
  Passed     : 553
  Failed     :   1

Has anyone hit this issue or tested MLIR? Thanks!

srj added a subscriber: srj.Aug 18 2020, 2:26 PM

This seems to have injected a link-time dependency on setupterm etc. even if you configure CMake with LLVM_ENABLE_TERMINFO=OFF (which was not the case before) -- this is breaking some builds of Halide as a result. (Should the #idef LLVM_ENABLE_TERMINFO actually if #if instead of #ifdef?)

Please consider reverting this change if a fix-forward isn't available soon.

srj added inline comments.Aug 18 2020, 2:37 PM
llvm/include/llvm/Config/config.h.cmake
215

In my local build of LLVM, I end up with

#define LLVM_ENABLE_TERMINFO 1

in the generated config.h file, even thought I configured LLVM's CMake build with -D LLVM_ENABLE_TERMINFO=OFF -- surely this is a bug.

srj added inline comments.Aug 18 2020, 2:42 PM
llvm/include/llvm/Config/config.h.cmake
215

(fyi, prior to this commit, the corresponding line was /* #undef HAVE_TERMINFO */)

haampie added inline comments.Aug 18 2020, 2:47 PM
llvm/include/llvm/Config/config.h.cmake
215

@srj you are completely right, the find_library lost its if (LLVM_ENABLE_TERMINFO) when the last review comments were addressed. Let me submit a patch asap

@srj could you please try / review https://reviews.llvm.org/D86173 for a fix for your problem?

@gribozavr2: can you try https://reviews.llvm.org/D86134? It should fix your issue, as well as the same issue w.r.t. the ZLIB changes of https://reviews.llvm.org/D79219 on macOS.

gkistanova reopened this revision.Aug 24 2020, 9:40 PM
gkistanova added a subscriber: gkistanova.

@haampie Are you working on fixing the http://lab.llvm.org:8011/builders/lld-perf-testsuite bot? This patch has broken it.

FAILED: bin/llvm-tblgen 
: && /usr/bin/c++  -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 -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3  -static -fno-pie -Wl,-allow-shlib-undefined    -Wl,-rpath-link,/home/buildslave/slave_as-bldslv8/lld-perf-testsuite/build/./lib  -Wl,-O3 -Wl,--gc-sections utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmMatcherEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterInst.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/Attributes.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CallingConvEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeEmitterGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenDAGPatterns.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenHwModes.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenInstruction.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenMapTable.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenRegisters.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenSchedule.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenTarget.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherOpt.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcher.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DFAEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DFAPacketizerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DirectiveEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/DisassemblerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/ExegesisEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/FastISelEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/FixedLenDecoderEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/GICombinerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/GlobalISelEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/InfoByHwMode.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/InstrInfoEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/InstrDocsEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/IntrinsicEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptParserEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptRSTEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/PredicateExpander.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/PseudoLoweringEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/RISCVCompressInstEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/RegisterBankEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/RegisterInfoEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SDNodeProperties.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SearchableTableEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SubtargetEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/SubtargetFeatureInfo.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/TableGen.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/Types.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86DisassemblerTables.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86EVEX2VEXTablesEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86FoldTablesEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86ModRMFilters.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86RecognizableInstr.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/WebAssemblyDisassemblerEmitter.cpp.o utils/TableGen/CMakeFiles/llvm-tblgen.dir/CTagsEmitter.cpp.o  -o bin/llvm-tblgen  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMSupport.a  lib/libLLVMTableGen.a  -lpthread  lib/libLLVMTableGenGlobalISel.a  lib/libLLVMTableGen.a  lib/libLLVMSupport.a  -lrt  -ldl  -lpthread  -lm  /usr/lib/x86_64-linux-gnu/libtinfo.so  lib/libLLVMDemangle.a && :
/usr/bin/ld: attempted static link of dynamic object `/usr/lib/x86_64-linux-gnu/libtinfo.so'

It seems the patch is quite problematic. Maybe reverting it at this point and making another approach is the way to go?

This revision is now accepted and ready to land.Aug 24 2020, 9:40 PM
gkistanova requested changes to this revision.Aug 24 2020, 9:40 PM
This revision now requires changes to proceed.Aug 24 2020, 9:40 PM

@gkistanova It's true that this change has lead to more issues I could ever imagine, but I think the link you provided is the last remaining problem.

Pinging @phosek for a similar issue w.r.t. zlib: since https://reviews.llvm.org/D79219 zlib gets disabled on static builds of LLVM. The reason is find_package(ZLIB) finds a shared library, and check_symbol_exists(compress2 zlib.h HAVE_ZLIB) tries to statically link to that -- it can't so zlib is disabled. I guess that's a regression too?

What would be the best way forward @phosek? A quick fix for ncurses is to add a simple check_symbol_exists test too, but that would in practice just disable ncurses in static builds. And additionally we could find static libs by adding explicit names like libtinfo.a to find_library when LLVM_BUILD_STATIC=ON. This trick won't help for zlib though, the find_library stuff is hidden inside find_package, and there is no way to target static libs it seems.

@gkistanova It's true that this change has lead to more issues I could ever imagine, but I think the link you provided is the last remaining problem.

Pinging @phosek for a similar issue w.r.t. zlib: since https://reviews.llvm.org/D79219 zlib gets disabled on static builds of LLVM. The reason is find_package(ZLIB) finds a shared library, and check_symbol_exists(compress2 zlib.h HAVE_ZLIB) tries to statically link to that -- it can't so zlib is disabled. I guess that's a regression too?

What would be the best way forward @phosek? A quick fix for ncurses is to add a simple check_symbol_exists test too, but that would in practice just disable ncurses in static builds. And additionally we could find static libs by adding explicit names like libtinfo.a to find_library when LLVM_BUILD_STATIC=ON. This trick won't help for zlib though, the find_library stuff is hidden inside find_package, and there is no way to target static libs it seems.

@haampie find_package and find_library uses CMAKE_FIND_LIBRARY_SUFFIXES to determine the suffix of the library (the default is ".so" ".a" which is why it always picks shared library). I think we should extend this block https://github.com/llvm/llvm-project/blob/master/llvm/CMakeLists.txt#L626 and filter out CMAKE_SHARED_LIBRARY_SUFFIX from CMAKE_FIND_LIBRARY_SUFFIXES when LLVM_BUILD_STATIC is enabled which will handle this case globally.

haampie updated this revision to Diff 288864.Aug 30 2020, 11:57 AM

Another take on this isuse, incorporating @phosek's comment on making find_library not find shared libs when compiling with LLVM_BUILD_STATIC=ON, and making sure that get_system_libname does not add empty items to a regex capture group, as CMake's regex engine does not support this.

@gkistanova, can you test this on your build bot with the static llvm build?

phosek accepted this revision.Aug 31 2020, 1:12 PM

LGTM

gkistanova accepted this revision.Aug 31 2020, 2:13 PM

Statically linked LLVM + LLD builds with this patch.

This revision is now accepted and ready to land.Aug 31 2020, 2:13 PM
This revision was automatically updated to reflect the committed changes.

Please note that ncurses is not the only supported curses library. NetBSD uses its original BSD curses for LLVM projects.