Opened 2 years ago

Closed 22 months ago

#27277 closed enhancement (duplicate)

spkg-configure.m4 for ncurses and readline

Reported by: dimpase Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: build Keywords: spkg-configure
Cc: embray, fbissey Merged in:
Authors: Dima Pasechnik Reviewers: Matthias Koeppe, François Bissey
Report Upstream: Reported upstream. No feedback yet. Work issues:
Branch: u/dimpase/packages/ncurses_readline-config (Commits, GitHub, GitLab) Commit: 6828f7ad4fc5533ba7987f4806440b90f9a08856
Dependencies: Stopgaps:

Status badges

Description (last modified by dimpase)

use pkg-config and test that the versions are as new as in Sage:

SAGE_SPKG_CONFIGURE([ncurses], [
    dnl First try checking for ncurses with pkg-config
    PKG_CHECK_MODULES([NCURSES], [ncurses >= 6.0], [sage_spkg_install_ncurses=no], [sage_spkg_install_ncurses=yes])
    ])

and

SAGE_SPKG_CONFIGURE([readline], [
    dnl First try checking for readline with pkg-config
    PKG_CHECK_MODULES([READLINE], [readline >= 6.3], [sage_spkg_install_readline=no], [sage_spkg_install_readline=yes])
    ])

readline 8.0 tarball: ftp://ftp.cwru.edu/pub/bash/readline-8.0.tar.gz

Change History (27)

comment:1 Changed 2 years ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:2 Changed 2 years ago by dimpase

test for use_tioctl to recognise ncurses version 6.

Last edited 2 years ago by dimpase (previous) (diff)

comment:3 Changed 2 years ago by embray

? Do we even need that if we can use PKG_CHECK_MODULES? Or do you mean for OSX?

For the time being I don't even really care that much about this working on OSX but if you or someone else wants to make it work I certainly won't complain as long as it doesn't make things enormously more complex.

comment:4 Changed 2 years ago by dimpase

  • Branch set to u/dimpase/packages/ncurses-config
  • Commit set to 3dbdddff063fe2743f69d10ca5e2a9b94623c107

New commits:

3dbdddfspkg-configure for ncurses, request version >= 5.7

comment:5 Changed 2 years ago by dimpase

macos native ncurses is OK, but readline is a faked one, libedit, with many functions from readline missing. Homebrew provides readline 8.0 linked with native macos ncurses. So we can try to replicate this.

comment:6 Changed 2 years ago by git

  • Commit changed from 3dbdddff063fe2743f69d10ca5e2a9b94623c107 to 33a1ae4735abd402ae204018ecc2be7cceaf5d36

Branch pushed to git repo; I updated commit sha1. New commits:

a190207readline try on osx
33a1ae4new readline 8.0 and checks in its spkg-configure

comment:7 Changed 2 years ago by dimpase

  • Description modified (diff)

comment:8 Changed 2 years ago by dimpase

  • Status changed from new to needs_review

OK, so this works on OSX - with native ncurses and readline built by Sage.

comment:9 Changed 2 years ago by dimpase

  • Status changed from needs_review to needs_work

Perhaps the readline version bump is not needed.

On Fedora I get this error building python2

gcc -pthread -shared -L. -L/home/scratch2/dimpase/sage/sage/local/lib -Wl,-rpath,/home/scratch2/dimpase/sage/sage/local/lib -L. -L/home/scratch2/dimpase/sage/sage/local/lib -Wl,-rpath,/home/scratch2/dimpase/sage/sage/local/lib build/temp.linux-x86_64-2.7/home/scratch2/dimpase/sage/sage/local/var/tmp/sage/build/python2-2.7.15.p1/src/Modules/readline.o -L/usr/lib/termcap -L/home/scratch2/dimpase/sage/sage/local/lib -L/usr/local/lib -L. -lreadline -lpython2.7 -o build/lib.linux-x86_64-2.7/readline.so
*** WARNING: renaming "readline" since importing it failed: /home/scratch2/dimpase/sage/sage/local/lib/libreadline.so.8: undefined symbol: UP

comment:10 Changed 2 years ago by dimpase

  • Cc fbissey added
  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

a patch related to libtinfo is still needed. E.g.

--- a/support/shobj-conf
+++ b/support/shobj-conf
@@ -130,6 +130,7 @@ linux*-*|gnu*-*|k*bsd*-gnu-*|freebsd*-gentoo)
 
        SHLIB_XLDFLAGS='-Wl,-rpath,$(libdir) -Wl,-soname,`basename $@ $(SHLIB_MINOR)`'
        SHLIB_LIBVERSION='$(SHLIB_LIBSUFF).$(SHLIB_MAJOR)$(SHLIB_MINOR)'
+       SHLIB_LIBS='-ltinfo'
        ;;
 
 freebsd2*)

fixes the build on Fedora.


Actually, I have spotted the bug (2, in fact) in the readline source.

  • The first is in the macro BASH_CHECK_LIB_TERMCAP in readline's aclocal.m4 (this is not an autogenerated file, it's hand-written). If --with-curses is given to ./configure then it still first checks libtermcap, and if it's installed on the system then libtinfo is skipped. After fixing this, one sees -ltinfo in TERMCAP_LIB variable, as it should be.

  • It's not enough to fix the above, as TERMCAP_LIB is only used on cygwin/mingw, for a reason I fail to comprehend. Setting SHLIB_LIBS to '$(TERMCAP_LIB)' unconditionally seems to be the right thing.

The thing works on OSX as there there is an unconditional SHLIB_LIBS='-lncurses'.

Should this be reported upstream?

comment:11 follow-up: Changed 2 years ago by fbissey

It feels like an upstream work around mostly but the stuff on fedora is probably worth reporting. I very much doubt it is fedora specific. Looking at the gentoo ebuild we have that telling section for readline 8.0

	# Force ncurses linking. #71420
	# Use pkg-config to get the right values. #457558
	local ncurses_libs=$($(tc-getPKG_CONFIG) ncurses --libs)
	sed -i \
		-e "/^SHLIB_LIBS=/s:=.*:='${ncurses_libs}':" \
		support/shobj-conf || die
	sed -i \
		-e "/^[[:space:]]*LIBS=.-lncurses/s:-lncurses:${ncurses_libs}:" \
		examples/rlfe/configure || die

	# fix building under Gentoo/FreeBSD; upstream FreeBSD deprecated
	# objformat for years, so we don't want to rely on that.
	sed -i -e '/objformat/s:if .*; then:if true; then:' support/shobj-conf || die

	ln -s ../.. examples/rlfe/readline || die # for local readline headers

comment:12 Changed 2 years ago by dimpase

No, I really don't see how the upstream stuff can work correctly---of course one can like an idiot insert -ltinfo everywhere -lreadline appears, but this does not make it right.

comment:13 in reply to: ↑ 11 Changed 2 years ago by dimpase

  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

Replying to fbissey:

It feels like an upstream work around mostly but the stuff on fedora is probably worth reporting. I very much doubt it is fedora specific.

No, it's readline's "not overlinking" (I'd rather call it "blatant underlinking) policy, plus "always try termcap first", see http://lists.gnu.org/archive/html/bug-readline/2014-04/msg00024.html (underlinking is very bad IMHO, it potentially means that a wrong termcap lib is used, resulting in all sorts of hiccups).

I just sent a followup there, but I don't have much hope for accepting this as bugs on upstream side.

comment:14 Changed 2 years ago by embray

I'll test this on Cygwin once I've confirmed that some other build failures introduced by #27212 are fixed (#27713 and #27714 so far...).

I don't expect any problems though, at least not with the system lib. Less sure about readline-8.0. My Cygwin still has readline 7.0.

comment:15 Changed 2 years ago by dimpase

please support me arguing against underlinking here: http://lists.gnu.org/archive/html/bug-readline/2019-04/msg00013.html :-)

comment:16 Changed 2 years ago by dimpase

And with readline version 4 GAP doesn't build:

ld: error: undefined symbol: rl_bind_keyseq
>>> referenced by sysfiles.c:2343 (src/sysfiles.c:2343)
>>>               obj/src/.libs/sysfiles.o:(initreadline)

So we need AC_SEARCH_LIBS on rl_bind_keyseq.

Last edited 2 years ago by dimpase (previous) (diff)

comment:17 Changed 2 years ago by dimpase

  • Branch changed from u/dimpase/packages/ncurses-config to u/dimpase/packages/ncurses_readline-config
  • Commit changed from 33a1ae4735abd402ae204018ecc2be7cceaf5d36 to 79e37a0e4c53ab24dd33a8f8f5ddc4e8c82984ce
  • Status changed from needs_work to needs_review

I'd like to postpone update to readline 8, see if we can do with what we have now.


New commits:

8d4157bspkg-configure for ncurses, request version >= 5.7
7b78256readline try on osx
2b3a5c7new readline 8.0 and checks in its spkg-configure
228bba0Revert "new readline 8.0 and checks in its spkg-configure"
79e37a0test for rl_bind_keyseq to detect old or fake readlines

comment:18 Changed 2 years ago by dimpase

  • Status changed from needs_review to needs_work

oops,now I recall that I forgot to update readline's patches, as a result it won't build with external ncurses (unless there is a library named libtinfo).

comment:19 Changed 2 years ago by git

  • Commit changed from 79e37a0e4c53ab24dd33a8f8f5ddc4e8c82984ce to 6828f7ad4fc5533ba7987f4806440b90f9a08856

Branch pushed to git repo; I updated commit sha1. New commits:

6828f7aget termcap library name from the readline's configure

comment:20 Changed 2 years ago by dimpase

  • Status changed from needs_work to needs_review

OK, this works now, external (n)curses or not.

comment:21 Changed 2 years ago by mkoeppe

  • Reviewers set to Matthias Koeppe

Works well on macOS Mojave: ncurses is not built, readline is built.

comment:22 Changed 2 years ago by dimpase

ping?

comment:23 Changed 2 years ago by fbissey

  • Reviewers changed from Matthias Koeppe to Matthias Koeppe, François Bissey
  • Status changed from needs_review to positive_review

LGTM

comment:24 follow-up: Changed 2 years ago by embray

  • Status changed from positive_review to needs_review

I still haven't had a chance to test this one.

comment:25 in reply to: ↑ 24 Changed 2 years ago by dimpase

Replying to embray:

I still haven't had a chance to test this one.

please test this as a part of #27825 - where I merged this and libgd with its deps.

comment:26 Changed 23 months ago by dimpase

  • Milestone changed from sage-8.8 to sage-duplicate/invalid/wontfix
  • Status changed from needs_review to positive_review

changing the milestone to concentrate the rest of this work on #27825

comment:27 Changed 22 months ago by chapoton

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.