Skip to content
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

spkg-configure.m4 for ncurses and readline #27277

Closed
dimpase opened this issue Feb 13, 2019 · 36 comments
Closed

spkg-configure.m4 for ncurses and readline #27277

dimpase opened this issue Feb 13, 2019 · 36 comments

Comments

@dimpase
Copy link
Member

dimpase commented Feb 13, 2019

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

Upstream: Reported upstream. No feedback yet.

CC: @embray @kiwifb

Component: build

Author: Dima Pasechnik

Branch/Commit: u/dimpase/packages/ncurses_readline-config @ 6828f7a

Reviewer: Matthias Koeppe, François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/27277

@dimpase dimpase added this to the sage-8.7 milestone Feb 13, 2019
@embray
Copy link
Contributor

embray commented Mar 25, 2019

comment:1

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

@embray embray modified the milestones: sage-8.7, sage-8.8 Mar 25, 2019
@dimpase
Copy link
Member Author

dimpase commented Apr 17, 2019

comment:2

test for use_tioctl to recognise ncurses version 6.

@embray
Copy link
Contributor

embray commented Apr 17, 2019

comment:3

? 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.

@dimpase
Copy link
Member Author

dimpase commented Apr 17, 2019

New commits:

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

@dimpase
Copy link
Member Author

dimpase commented Apr 17, 2019

Branch: u/dimpase/packages/ncurses-config

@dimpase
Copy link
Member Author

dimpase commented Apr 17, 2019

Commit: 3dbdddf

@dimpase
Copy link
Member Author

dimpase commented Apr 18, 2019

comment:5

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2019

Changed commit from 3dbdddf to 33a1ae4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2019

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

@dimpase

This comment has been minimized.

@dimpase
Copy link
Member Author

dimpase commented Apr 20, 2019

comment:8

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

@dimpase
Copy link
Member Author

dimpase commented Apr 21, 2019

comment:9

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

@dimpase
Copy link
Member Author

dimpase commented Apr 22, 2019

comment:10

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?

@dimpase
Copy link
Member Author

dimpase commented Apr 22, 2019

Upstream: Not yet reported upstream; Will do shortly.

@kiwifb
Copy link
Member

kiwifb commented Apr 22, 2019

comment:11

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

@dimpase
Copy link
Member Author

dimpase commented Apr 22, 2019

comment:12

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.

@dimpase
Copy link
Member Author

dimpase commented Apr 23, 2019

comment:13

Replying to @kiwifb:

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.

@dimpase
Copy link
Member Author

dimpase commented Apr 23, 2019

Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

@embray
Copy link
Contributor

embray commented Apr 23, 2019

comment:14

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.

@dimpase
Copy link
Member Author

dimpase commented Apr 24, 2019

comment:15

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

@dimpase
Copy link
Member Author

dimpase commented May 1, 2019

comment:18

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).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 1, 2019

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

6828f7aget termcap library name from the readline's configure

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 1, 2019

Changed commit from 79e37a0 to 6828f7a

@dimpase
Copy link
Member Author

dimpase commented May 1, 2019

comment:20

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

@mkoeppe
Copy link
Contributor

mkoeppe commented May 9, 2019

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented May 9, 2019

comment:21

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

@dimpase
Copy link
Member Author

dimpase commented May 15, 2019

comment:22

ping?

@kiwifb
Copy link
Member

kiwifb commented May 15, 2019

Changed reviewer from Matthias Koeppe to Matthias Koeppe, François Bissey

@kiwifb
Copy link
Member

kiwifb commented May 15, 2019

comment:23

LGTM

@embray
Copy link
Contributor

embray commented May 17, 2019

comment:24

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

@dimpase
Copy link
Member Author

dimpase commented May 17, 2019

comment:25

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.

@dimpase
Copy link
Member Author

dimpase commented May 24, 2019

comment:26

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants