search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.33k stars 453 forks source link

ncurses fails to build with GCC 5.x #18301

Closed 83660e46-0051-498b-a8c1-f7a7bd232b5a closed 9 years ago

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago 
...
gcc-5.1 -DHAVE_CONFIG_H -I../ncurses -I../../ncurses -I. -I../include -I../../ncurses/../include -I/foo/sage-6.6-gcc-5.1.0/local/include  -D_GNU_SOURCE -DNDEBUG -O2 --param max-inline-insns-single=1200 -fPIC -c ../ncurses/lib_gen.c -o ../obj_s/lib_gen.o
In file included from ../../ncurses/curses.priv.h:321:0,
                 from ../ncurses/lib_gen.c:19:
_3081.c:837:15: error: expected ')' before 'int'
../include/curses.h:1622:56: note: in definition of macro 'mouse_trafo'
 #define mouse_trafo(y,x,to_screen) wmouse_trafo(stdscr,y,x,to_screen)
                                                        ^
Makefile:1323: recipe for target '../obj_s/lib_gen.o' failed
make[1]: *** [../obj_s/lib_gen.o] Error 1
make[1]: Leaving directory '/tmp/sage-build-tmp/ncurses-5.9.20131221/src/narrow/ncurses'
Makefile:111: recipe for target 'all' failed
make: *** [all] Error 2
Error building ncurses (narrow).

(build/pkgs/ncurses/patches/ncurses-5.9.20131221_work_around_broken_GNU_cpp_5.x.patch, self-explanatory:)

Building ncurses with GCC 5.1 (or more precisely, with its 'cpp') fails with
a syntax error, caused by earlier preprocessing.

(I'm not entirely sure whether it's a GCC bug or rather caused by a new
feature which breaks further processing with 'awk' and 'sed';  I *think*
at least the 'awk' inline script "AW2" simply isn't prepared for the changed
output of 'cpp' w.r.t. line directives [1].  Anyway, the patch fixes the issue.)

[1] https://gcc.gnu.org/gcc-5/porting_to.html

--- ncurses-5.9.20131221/ncurses/base/MKlib_gen.sh 2011-06-04 21:14:08.000000000 +0200
+++ ncurses-5.9.20131221/ncurses/base/MKlib_gen.sh 2015-04-26 00:47:06.911680782 +0200
@@ -62,7 +62,15 @@
 if test "${LC_CTYPE+set}"    = set; then LC_CTYPE=C;    export LC_CTYPE;    fi
 if test "${LC_COLLATE+set}"  = set; then LC_COLLATE=C;  export LC_COLLATE;  fi

-preprocessor="$1 -DNCURSES_INTERNALS -I../include"
+# Work around "unexpected" output of GCC 5.1.0's cpp w.r.t. #line directives
+# by simply suppressing them:
+case `$1 -dumpversion 2>/dev/null` in
+    5.[01].*)  # assume a "broken" one
+        preprocessor="$1 -P -DNCURSES_INTERNALS -I../include"
+        ;;
+    *)
+        preprocessor="$1 -DNCURSES_INTERNALS -I../include"
+esac
 AWK="$2"
 USE="$3"

CC: @jpflori @vbraun

Component: packages: standard

Keywords: syntax error, cpp

Author: Leif Leonhardy

Branch: 2201c9c

Reviewer: Jean-Pierre Flori

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

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago
comment:1

Haven't checked whether or how upstream already deals with this (probably even by adding -P unconditionally).

Perhaps J-P wants to... :P

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago

Commit: 2201c9c

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago

New commits:

2201c9cAdd patch to let ncurses build with GCC 5.x (changed output of 'cpp')
83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago

Branch: u/leif/ncurses_GCC_5.x

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago
comment:3

Meanwhile found an upstream patch for the issue (not yet part of a "stable" release):

--- ncurses-5.9.20131221/ncurses/base/MKlib_gen.sh  2011-06-04 21:14:08.000000000 +0200
+++ ncurses-5.9-20150418/ncurses/base/MKlib_gen.sh  2014-12-06 19:56:25.000000000 +0100
@@ -2,10 +2,10 @@
 #
 # MKlib_gen.sh -- generate sources from curses.h macro definitions
 #
-# ($Id: MKlib_gen.sh,v 1.46 2011/06/04 19:14:08 tom Exp $)
+# ($Id: MKlib_gen.sh,v 1.47 2014/12/06 18:56:25 tom Exp $)
 #
 ##############################################################################
-# Copyright (c) 1998-2010,2011 Free Software Foundation, Inc.                #
+# Copyright (c) 1998-2011,2014 Free Software Foundation, Inc.                #
 #                                                                            #
 # Permission is hereby granted, free of charge, to any person obtaining a    #
 # copy of this software and associated documentation files (the "Software"), #
@@ -474,11 +474,22 @@
    -e 's/gen_$//' \
    -e 's/  / /g' >>$TMP

+cat >$ED1 <<EOF
+s/  / /g
+s/^ //
+s/ $//
+s/P_NCURSES_BOOL/NCURSES_BOOL/g
+EOF
+
+# A patch discussed here:
+#  https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02185.html
+# introduces spurious #line markers.  Work around that by ignoring the system's
+# attempt to define "bool" and using our own symbol here.
+sed -e 's/bool/P_NCURSES_BOOL/g' $TMP > $ED2
+cat $ED2 >$TMP
+
 $preprocessor $TMP 2>/dev/null \
-| sed \
-   -e 's/  / /g' \
-   -e 's/^ //' \
-   -e 's/_Bool/NCURSES_BOOL/g' \
+| sed -f $ED1 \
 | $AWK -f $AW2 \
 | sed -f $ED3 \
 | sed \

Still think my solution is a bit more robust (and easier to follow ;-) ). (Haven't actually tried upstream's fix.)

Cc'ing the spkg maintainer...

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago
comment:4

P.S.: Doesn't look like that single upstream patch alone would suffice, we'd presumably have to include more (but our package is already patched).

I'm not really keen to figure out which other parts we may need, and whether those may in turn break other things.

kiwifb commented 9 years ago
comment:5

Well the corresponding gentoo bug is https://bugs.gentoo.org/show_bug.cgi?id=545114 since the same dev opened and closed it, I would say they very much have tested upstream patch.

I'll have to say I like yours better, it feels more comprehensible. But upstream will probably stick to its own fix.

Current version of ncurses in sage is a dev version from upstream (available at ftp://invisible-island.net/ncurses/current/ the one we have is old enough to be off the list) may be we should just upgrade to one of the latest dev version? Jean-Pierre Flori did the last upgrade may be he has an opinion on the process?

kiwifb commented 9 years ago
comment:6

Well using ncurses-5.9-20150425 got me through. I had to remove the patch for ncurses included in sage. The first part about aclocal.m4 applied with fuzz, but the second part about configure was rejected as appearing to be a reverse patch. I take that as a news that the patch in question is no longer necessary.

I'll report back about whether or not that upgrade breaks any parts of sage.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago
comment:7

Replying to @kiwifb:

Current version of ncurses in sage is a dev version from upstream (available at ftp://invisible-island.net/ncurses/current/ the one we have is old enough to be off the list) may be we should just upgrade to one of the latest dev version?

Well, that's the problem. Last time J-P had to add a patch again because something got borked again which was fixed in the previous version, and I cannot (and don't want to) test on various platforms, with various compilers or setups.

(The diff above is btw. from the latest devel version, but only for that file, which got fixed in an earlier version.)

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago
comment:8

I'd rather upgrade on another (less important) ticket, i.e., get some working fix into Sage quickly, without risking other issues or endless review.

jpflori commented 9 years ago
comment:9

I agree, let's update in a follow up ticket.

And I fear the XOPEN_SOURCE thing if still present is still broken on some archs. The patch surely does not apply anymore because configure was regenerated...

jpflori commented 9 years ago

Reviewer: Jean-Pierre Flori

jpflori commented 9 years ago
comment:11

And yes we should upgrade asap as well if possible.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago
comment:12

Thanks.

Where did you have the XOPEN_SOURCE problems?

I currently cannot test on anything but Linux x86/x86_64 (skynet and bsd.math, RIP), so I didn't even try to upgrade.

jpflori commented 9 years ago
comment:13

The XOPEN_SOURCE problem was on Solaris... See #15268 comment:10 and #15796 for some info.

Note sure what is in latest upstream right now... See http://invisible-island.net/ncurses/NEWS.html#index-t20131116 and http://invisible-island.net/ncurses/NEWS.html#index-t20140209 and http://invisible-island.net/ncurses/NEWS.html#index-t20150221 for some recent changes, especially the second one. I'd say the patch is not needed anymore, but we'll have to check the source or test on Solaris.

vbraun commented 9 years ago

Changed branch from u/leif/ncurses_GCC_5.x to 2201c9c

saraedum commented 9 years ago
comment:15

This is still an issue with GCC 5.2.0. See #18977.

saraedum commented 9 years ago

Changed commit from 2201c9c to none

fchapoton commented 9 years ago

Description changed:

--- 
+++ 
@@ -30,8 +30,8 @@
 [1] https://gcc.gnu.org/gcc-5/porting_to.html

---- ncurses-5.9.20131221/ncurses/base/MKlib_gen.sh 2011-06-04 21:14:08.000000000 +0200
-+++ ncurses-5.9.20131221/ncurses/base/MKlib_gen.sh 2015-04-26 00:47:06.911680782 +0200
+--- ncurses-5.9.20131221/ncurses/base/MKlib_gen.sh 2011-06-04 21:14:08.000000000 +0200
++++ ncurses-5.9.20131221/ncurses/base/MKlib_gen.sh 2015-04-26 00:47:06.911680782 +0200
 @@ -62,7 +62,15 @@
  if test "${LC_CTYPE+set}"    = set; then LC_CTYPE=C;    export LC_CTYPE;    fi
  if test "${LC_COLLATE+set}"  = set; then LC_COLLATE=C;  export LC_COLLATE;  fi