search

sagemath / sage

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

update readline to 6.2 #11882

Closed jhpalmieri closed 13 years ago

jhpalmieri commented 13 years ago

readline 6.2 is available. Upgrade Sage to use this version. This also includes a fix so it builds on OS X 10.7 Lion.

The home base for this ticket is the Lion ticket #11881.

Apply http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p1.spkg

CC: @kini @nexttime @nathanncohen @jdemeyer @sagetrac-gostrc @alexanderdreyer @alexanderdreyer

Component: packages: standard

Keywords: readline upgrade spkg lion Darwin 11 MacOS X 10.7

Author: John Palmieri

Reviewer: Leif Leonhardy, Alexander Dreyer

Merged: sage-4.8.alpha0

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

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

Changed keywords from readline lion to readline upgrade spkg lion Darwin 11 MacOS X 10.7

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

According to the attached spkg patch, the version in the changelog entry heading doesn't match the package's one, i.e., the .p0 is missing.

jhpalmieri commented 13 years ago
comment:5

Oops, sorry. Updated now.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:6

While you're at it:

   CFLAGS="$CFLAGS -g -O2"

should be

   CFLAGS="-g -O2 $CFLAGS"

(This is IMHO a regression from my previous spkg.)

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

s/IMHO/IIRC/ :)

jhpalmieri commented 13 years ago
comment:8

And presumably the same for CFLAGS="$CFLAGS -g -O0"?

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

Replying to @jhpalmieri:

And presumably the same for CFLAGS="$CFLAGS -g -O0"?

Nope. In that case, we want to override a user's setting.

jhpalmieri commented 13 years ago

Attachment: trac_11882-readline.patch.gz

patch for readline spkg, for review only

jhpalmieri commented 13 years ago
comment:10

Okay, the spkg has been updated.

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

Reviewer: Leif Leonhardy

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:11

The files (mostly Sage's, including the root directory) have weird permissions; run

$ chmod -R +rX . .hg* *

Copy-paste accident:

$ sed -n -e '/^== readline-.* ==$/s/==/===/gp' SPKG.txt 
=== readline-6.2.p0 (John Palmieri, 30 Sept 2011) ===
=== readline-6.1 (David Kirkby, 11th November 2010) ===

(Btw., a new spkg sanity check in Jeroen's merger3 might reject the spkg otherwise, or at least later versions of it. :-) )

s/## Readline 6.1/## Readline 6.2/ in spkg-install, or just remove the three lines.

More important,

  DYLIB_NAME="$SAGE_LOCAL"/lib/libreadline.so.6.1  # Untested. David Kirkby, 11th November 2010

should now be

  DYLIB_NAME="$SAGE_LOCAL"/lib/libreadline.so.6.2  # Untested. David Kirkby, 11th November 2010

I guess the Arch Linux and OpenSUSE 11.x fixes are no longer necessary with readline 6.2, but unless Sage breaks (with the system's libreadline), we may keep them at the moment. In principle we should really check the versions and decide based on these.

We should only delete Sage's previous version upon a successful build.

If one day make check is supported, we may have to set up CFLAGS et al. in spkg-check as well. Perhaps add a comment on that.

Optionally:

We should replace the ugly

if [ "x$CFLAG64" = x ] ; then
   CFLAG64=-m64
fi

by

: ${CFLAG64=-m64} # Only sets it to '-m64' if it's not set at all, not even to "".

which is shorter and faster (and maybe safer ;-) ); otherwise I'd use [ -z "$CFLAG64" ] or [[ -z $CFLAG64 ]] (see below; we could use the latter also for $SAGE_LOCAL).

You could also use

   ...
   CFLAGS="$CFLAGS ${CFLAG64=-m64}"
   # The above *sets* CFLAG64 in case it is undefined,
   # so it's sufficient to use just $CFLAG64 in the following assignments.
   ...

and omit the whole if [ -z "$CFLAG64" ] ... (or whatever) block.

if [ "x$SAGE64" = xyes ] ; then
...

could be

if [ "${SAGE64:-no}" = yes ]; then
...

or, perhaps more readable, and even better, using the safe (and faster) shell built-in version of test:

if [[ $SAGE64 = yes ]]; then
...

(Same for $SAGE_DEBUG.)

"Naturally", I haven't yet tested the new spkg at all... (And I won't be able to test it on Lion and other strange OSs or Linux distributions either. ;-) )

jhpalmieri commented 13 years ago
comment:12

Replying to @nexttime:

The files (mostly Sage's, including the root directory) have weird permissions;

That's strange. Looks like that was true for the 6.1 spkg, too. Easy enough to fix.

Copy-paste accident:

Also from the last spkg, but easy to fix.

s/## Readline 6.1/## Readline 6.2/ in spkg-install, or just remove the three lines.

Removed.

More important,

  DYLIB_NAME="$SAGE_LOCAL"/lib/libreadline.so.6.1  # Untested. David Kirkby, 11th November 2010

fixed.

I guess the Arch Linux and OpenSUSE 11.x fixes are no longer necessary with readline 6.2, but unless Sage breaks (with the system's libreadline), we may keep them at the moment. In principle we should really check the versions and decide based on these.

Right. I don't access to any machines running those operating systems, so it feels safest to leave things as they are.

We should only delete Sage's previous version upon a successful build.

Done.

If one day make check is supported, we may have to set up CFLAGS et al. in spkg-check as well. Perhaps add a comment on that.

Done.

Optionally:

I replaced all of tests with the [[ blah ]] version.

jhpalmieri commented 13 years ago

Attachment: trac_11882-readline.v2.patch.gz

patch for readline spkg, for review only

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

P.S.:

On Silius, we get (with the current 6.1 spkg):

...
OpenSuSE detected
... but not OpenSuSE 11 -> building Sage's version of libreadline.
...

which is wrong, because it runs SLES, but -- just luckily I think -- grep -q 11\\. /etc/SuSE-release yields false because we there have

SUSE Linux Enterprise Server 11 (ppc64)
VERSION = 11
PATCHLEVEL = 1

(/bin/bash is dynamically linked against libreadline there, too, but it only has libreadline.so.5[.2] and no devel version installed.)

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:14

Replying to @jhpalmieri:

Replying to @nexttime: [...]

(Ok.)

I guess the Arch Linux and OpenSUSE 11.x fixes are no longer necessary with readline 6.2, but unless Sage breaks (with the system's libreadline), we may keep them at the moment. In principle we should really check the versions and decide based on these.

Right. I don't access to any machines running those operating systems, so it feels safest to leave things as they are.

IIRC Nathann Cohen is our OpenSUSE master...

[...]

If one day make check is supported, we may have to set up CFLAGS et al. in spkg-check as well. Perhaps add a comment on that.

Done.

I was thinking of "Special Update/Build Instructions" as well.

Optionally:

I replaced all of tests with the [[ blah ]] version.

Ok. By the way, this is also supported by MacOS X 10.4's outdated bash 2.04.

jhpalmieri commented 13 years ago
comment:15

Just for fun, I thought I would actually test out the spkg, instead of just creating one which theoretically ought to work. It builds successfully on the following machines, and it seems to run successfully on all of the ones where I can actually run Sage (that is, all of them except silius and OS X 10.7):

I couldn't log in to the other skynet machines (except for sextus2, where I now have a build starting from scratch, and for which this spkg claims to have built successfully). I also couldn't log in to David Kirkby's machine hawk. So I haven't been able to test on OpenSolaris yet.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:16

Ok, I'm mostly happy with the new spkg (I'll eventually upload a reviewer patch / spkg with minor changes, but that's not very likely), so positive review from me, modulo testing on Arch Linux and OpenSUSE, perhaps also OpenSolaris, none of which I'm currently able to.

Jeroen should decide whether we can merge this without having tested it on some of the mentioned platforms.

Also someone who had problems with readline and R on Ubuntu 11.10 could test whether this fixes the issue; I'm not aware of any detailed report regarding that, and others did not have such problems when building Sage on Oneiric Ocelot.

My new R spkg (see #9906) might by the way also fix such issues; haven't investigated that.

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

Work Issues: Test on OpenSUSE, OpenSolaris, perhaps also Arch Linux.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:17

Replying to @nexttime:

Also someone who had problems with readline and R on Ubuntu 11.10 could test whether this fixes the issue; I'm not aware of any detailed report regarding that, and others did not have such problems when building Sage on Oneiric Ocelot.

My new R spkg (see #9906) might by the way also fix such issues; haven't investigated that.

Doesn't look like it would change anything regarding that, i.e., our current R spkg already properly configures R with --with-readline=..., prepends Sage's include directory to CPPFLAGS etc., and prepends -L$SAGE_LOCAL/lib to LDFLAGS.

I was just guessing that might have been the issue.

jdemeyer commented 13 years ago
comment:18

I can try merging it in sage-4.7.3.alpha0 and see if people complain...

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:19

Replying to @jdemeyer:

I can try merging it in sage-4.7.3.alpha0 and see if people complain...

Well, or into a (Sage 4.7.2) release candidate; that's what they're for... ;-)

I'm still curious whether we get trouble on Oneiric Ocelot with our current readline (and R, as mentioned above).

jhpalmieri commented 13 years ago
comment:20

Replying to @jdemeyer:

I can try merging it in sage-4.7.3.alpha0 and see if people complain...

I think that's fine. Is it likely to cause problems? If so, you could mention it particularly in the release announcement.

jdemeyer commented 13 years ago
comment:21

Replying to @nexttime:

Jeroen should decide whether we can merge this without having tested it on some of the mentioned platforms.

Absolutely! I really want to support giving tickets positive_review without testing on all systems. I don't want to give the author nor the reviewer the burden of too much testing. We do have automated buildbots, we do have Sage developers using more different systems than you can imagine, let's use their power. Of course, some reasonable testing needs to be done, but that was clearly the case here. Also, there is nothing that can go catastrophically wrong with this ticket. I mean, Sage is not going to answer "1 + 1 = 3" because of a readline update.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:22

Regarding the build on Arch Linux, someone (h/t csousa) on the IRC pointed me to this:

http://projects.archlinux.org/svntogit/community.git/plain/trunk/PKGBUILD?h=packages/sage-mathematics

...
build() {
  cd sage-${pkgver}

  # modularization of sage, sort of :)
  # fixes the following error:
  # bash: symbol lookup error: bash: undefined symbol: rl_filename_rewrite_hook
  # remove this hack when sage uses a readline 6.1 or greater, or when sage uses its own internal bash
  # this is for people who have custom kernels (sage works this around by checking uname -r)
  #mkdir -p spkg/installed
  #touch spkg/installed/readline-6.1
...

So readline 6.2 is likely to work as well, unless they meanwhile have an even newer version in Arch.

Thus we should change

if [ -f /etc/arch-release ]; then
    ...
fi

to

if false && [ -f /etc/arch-release ]; then
    ...
fi

Or leave the Arch Linux detection and just print a message that we're building Sage's readline since we assume it to work...

(I wouldn't delete the whole stuff yet.)

jhpalmieri commented 13 years ago
comment:23

Should we "comment out" both the !OpenSUSE and Arch Linux checks (if false && ...) and ask people to test it in the upcoming 4.7.3.alpha0? The cleaner the spkg, the better, so this might let us know whether we need the fixes anymore.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:24

Replying to @jhpalmieri:

Should we "comment out" both the !OpenSUSE and Arch Linux checks (if false && ...) and ask people to test it in the upcoming 4.7.3.alpha0? The cleaner the spkg, the better, so this might let us know whether we need the fixes anymore.

I think so. We can keep the detection, such that we see whether that still works appropriately, but I would disable using the system's libreadline completely.

I just asked Alexander Dreyer to test this, since he apparently has access to an [Open?]SuSE 11 system.

jhpalmieri commented 13 years ago
comment:25

Okay, a new version is up, with accompanying patch here.

jhpalmieri commented 13 years ago
comment:26

The differences: in SPKG.txt, record the new changes, and also put in a note about self tests and modifying CFLAGS etc. in spkg-check. In spkg-install, turn off both the Arch Linux and OpenSuSE tests with "if false && ...".

jhpalmieri commented 13 years ago

Attachment: trac_11882-readline.v3.patch.gz

patch for readline spkg, for review only

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:27

Yep, patch looks good. (Now also prints "Arch Linux detected"; I had just reviewed the intermediate version. :) )

[X] leave as positive_review

d46d2cb1-860f-484d-8329-8ebfc9f5d004 commented 13 years ago

Changed work issues from Test on OpenSUSE, OpenSolaris, perhaps also Arch Linux. to Test on OpenSolaris, perhaps also Arch Linux.

d46d2cb1-860f-484d-8329-8ebfc9f5d004 commented 13 years ago
comment:28

The spkg installs fine on OpenSuSE 11, so side effects on runtime visible.

d46d2cb1-860f-484d-8329-8ebfc9f5d004 commented 13 years ago

Changed reviewer from Leif Leonhardy to Leif Leonhardy, Alexander Dreyer

jdemeyer commented 13 years ago

Merged: sage-4.7.3.alpha0

8580ca9c-4022-4fe4-ab99-5527ae6ab575 commented 13 years ago
comment:30

I've tested new readline-6.2.p0.spkg in an updated Arch Linux with Sage 4.7.1. Everything seems ok.

I was not completely sure of what should be tested, but sage, sage --sh and sage --notebook run ok.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:31

Replying to @sagetrac-cdsousa:

I've tested new readline-6.2.p0.spkg in an updated Arch Linux with Sage 4.7.1. Everything seems ok.

I was not completely sure of what should be tested, but sage, sage --sh and sage --notebook run ok.

Ok, thanks.

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

Changed work issues from Test on OpenSolaris, perhaps also Arch Linux. to none

jhpalmieri commented 13 years ago
comment:32

I'm considering modifying the spkg with the following patch:

diff --git a/SPKG.txt b/SPKG.txt
--- a/SPKG.txt
+++ b/SPKG.txt
@@ -32,13 +32,17 @@ Website: http://tiswww.case.edu/php/chet
  * Deleted some files from the doc directory from the standard distro,
    since it took tons of space; didn't delete anything else.
  * Work around some MacOSX dynamic lib flags
-* TODO: When we add self-tests, we may have to set up variables like
+ * TODO: in #11882, we turned off the fixes for Arch Linux and
+   OpenSuSE by changing various tests in spkg-install from "if ..."
+   to "if false && ...".  After further testing, these sections of
+   spkg-install should be removed completely.
+ * TODO: When we add self-tests, we may have to set up variables like
    CFLAGS in spkg-check.

 == Changelog ==

 === readline-6.2.p0 (John Palmieri, 30 Sept 2011) ===
- * Update to version 6.2.
+ * Update to version 6.2 (trac #11882).
  * Fix for Mac OS X 10.7 Lion.  This fix is taken from
    https://trac.macports.org/browser/trunk/dports/devel/readline/files/patch-sh
  * spkg-install: Turn off all hacks and fixes for Arch Linux and OpenSuSE.

Is this a good idea?

Jeroen: if I did this, would you be able to use a new version in 4.7.3.alpha0?

Or should I create a ".p1" version of the spkg with changes like these?

jdemeyer commented 13 years ago
comment:33

Replying to @jhpalmieri:

Is this a good idea?

Probably yes, but I will leave it up to others to judge. But maybe we should be bold and simply remove the "if false" code and then we don't need the comment. Adding the ticket number in the spkg is also a very good idea.

Jeroen: if I did this, would you be able to use a new version in 4.7.3.alpha0?

Yes, especially if the change won't affect the functionality of the spkg. Just don't wait too long please.

Or should I create a ".p1" version of the spkg with changes like these?

As you wish, .p0 or .p1 doesn't matter for me.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:34

Replying to @jdemeyer:

Replying to @jhpalmieri:

Is this a good idea?

Probably yes, but I will leave it up to others to judge. But maybe we should be bold and simply remove the "if false" code and then we don't need the comment. Adding the ticket number in the spkg is also a very good idea.

It is likely that we'll have to reenable the code (with slight changes) when Arch or SuSE again have newer versions (with new symbols), so I wouldn't delete it.

It could be smarter or more generic of course, but I wouldn't make such changes here and now.

Jeroen: if I did this, would you be able to use a new version in 4.7.3.alpha0?

Yes, especially if the change won't affect the functionality of the spkg. Just don't wait too long please.

Or should I create a ".p1" version of the spkg with changes like these?

As you wish, .p0 or .p1 doesn't matter for me.

In general I'd prefer a p1 to avoid confusion, but there are no changes to the code, and as long as its location is the same I won't mind having the changes made to the p0.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 13 years ago
comment:35

... so I'd change your suggested SPKG.txt phrase accordingly.

Also, "When we add self-tests ..." should read "If upstream adds self-tests ...", since we already run make check, but that's currently a no-op.

jhpalmieri commented 13 years ago
comment:36

I've uploaded "p1" version of the spkg; the only difference is some comments in SPKG.txt. If the new version is acceptable, feel free to remove the link to the "p0" version from the ticket description.

jhpalmieri commented 13 years ago

Description changed:

--- 
+++ 
@@ -3,4 +3,5 @@
 New spkg available for testing:

 - [http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p0.spkg](http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p0.spkg)
+- [http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p1.spkg](http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p1.spkg)
jdemeyer commented 13 years ago
comment:37

Two minor comments:

  1. The date of the p1 is "14 Oct Sept 2011"
  2. Also add the ticket number to the p1 in SPKG.txt
jhpalmieri commented 13 years ago
comment:38

Okay, fixed.

jhpalmieri commented 13 years ago

Attachment: trac_11882-readline.p1.patch.gz

diff between p0 and p1

kcrisman commented 13 years ago

Description changed:

--- 
+++ 
@@ -5,3 +5,4 @@
 - [http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p0.spkg](http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p0.spkg)
 - [http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p1.spkg](http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p1.spkg)

+The home base for this ticket is the Lion ticket #11881.
jdemeyer commented 13 years ago

Description changed:

--- 
+++ 
@@ -1,8 +1,5 @@
 readline 6.2 is available.  Upgrade Sage to use this version.  This also includes [a fix](https://trac.macports.org/browser/trunk/dports/devel/readline/files/patch-shobj-conf.diff) so it builds on OS X 10.7 Lion.

-New spkg available for testing:
+The home base for this ticket is the Lion ticket #11881.

-- [http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p0.spkg](http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p0.spkg)
-- [http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p1.spkg](http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p1.spkg)
-
-The home base for this ticket is the Lion ticket #11881.
+**Apply** [http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p1.spkg](http://sage.math.washington.edu/home/palmieri/SPKG/readline-6.2.p1.spkg)
jdemeyer commented 13 years ago

Milestone sage-4.7.3 deleted

jdemeyer commented 13 years ago

Changed merged from sage-4.7.3.alpha0 to sage-4.8.alpha0