Opened 10 years ago

Closed 9 years ago

#12112 closed defect (fixed)

Update the prereq script

Reported by: jdemeyer Owned by: GeorgSWeber
Priority: major Milestone: sage-5.0
Component: build Keywords:
Cc: mjo, jhpalmieri, drkirkby Merged in: sage-5.0.beta13
Authors: John Palmieri, Jeroen Demeyer Reviewers: Jeroen Demeyer, David Kirkby
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12739, to be merged with #12369 Stopgaps:

Status badges

Description (last modified by jdemeyer)

  1. Added a test for a C99 compiler, since some packages require that.
  1. Remove the useless checks for bison and flex (in the past, we did the checks but didn't fail if these programs weren't found).
  1. We don't need to check gcc/g++/gfortran versions if we're building GCC. Use the environment variable SAGE_BUILD_TOOLCHAIN (introduced in #12369) for this.
  1. Make the script portable, so it doesn't require bash (instead of #12621, which proposed to force bash).

Apply to the SAGE_ROOT repository:

  1. 12112_from_12576.patch (positive_review, see #12576)
  2. 12112_update_prereq.patch

Download http://boxen.math.washington.edu/home/jdemeyer/spkg/prereq-1.0.tar.gz to SAGE_ROOT/spkg/base and delete prereq-0.9.tar.gz (reviewers: look at prereq-1.0.diff for the changes)

See also: #12785 (update the FAQ)

Attachments (3)

12112_from_12576.patch (8.0 KB) - added by jdemeyer 9 years ago.
12112_update_prereq.patch (2.9 KB) - added by jdemeyer 9 years ago.
prereq-1.0.diff (29.3 KB) - added by jdemeyer 9 years ago.
Diff for the prereq tarball, for review only

Download all attachments as: .zip

Change History (39)

comment:1 Changed 10 years ago by kcrisman

Huh, that would be useful to know for the Cygwin thing as well. Currently we need it there, presumably for the same reason. Thanks.

comment:2 follow-up: Changed 10 years ago by mjo

  • Cc mjo added

Sage may not need it, but the rest of the prereq build system does:

prereq-0.9 $ make
(CDPATH="${ZSH_VERSION+.}:" && cd . && /bin/sh /home/mjo/src/sage-4.8.alpha3/spkg/build/prereq-0.9/missing --run autoheader)
autom4te-2.68: need GNU m4 1.4 or later: /usr/bin/m4
autoheader-2.68: '/usr/bin/autom4te-2.68' failed with exit status: 1
make: *** [config.h.in] Error 1

We could probably ship a pre-built configure and Makefile rather than rely on autotools, but this is black magic to me and I don't understand the tradeoffs involved.

comment:3 in reply to: ↑ 2 Changed 10 years ago by jdemeyer

Replying to mjo:

Sage may not need it, but the rest of the prereq build system does.

Not really, you just need to run it once, it is sufficient if the developer of the new prereq package runs it before shipping it into Sage.

comment:4 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from prereq should not check for m4 to Update the prereq script

comment:5 Changed 9 years ago by jdemeyer

  • Dependencies set to #12739

comment:6 Changed 9 years ago by jdemeyer

  • Authors changed from Jeroen Demeyer to John Palmieri, Jeroen Demeyer
  • Description modified (diff)
  • Reviewers set to Jeroen Demeyer

Changed 9 years ago by jdemeyer

comment:7 Changed 9 years ago by jdemeyer

  • Cc jhpalmieri added
  • Description modified (diff)

comment:8 Changed 9 years ago by jhpalmieri

What do you think about changing the prereq install script to use bash instead of sh? (See #12621: with non-bash versions of sh, e.g., on Solaris, at least one part of the script doesn't work properly.)

comment:9 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:10 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:11 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:12 Changed 9 years ago by jdemeyer

  • Status changed from new to needs_review

This seems to work like it should, so needs_review.

comment:13 follow-up: Changed 9 years ago by jhpalmieri

  • Cc drkirkby added

Dave, I'm cc'ing you. I don't know if you have any time these days, but since you worked on the original prereq script, you may be one of the most qualified people to review this, especially the changes in prereq-1.0.diff. (The other changes are pretty straightforward, I think.)

comment:14 follow-up: Changed 9 years ago by jdemeyer

Concerning bash: maybe we shoudn't require bash in the prereq script. The prereq script checks whether bash is available and gives a clear error message if it's not. So I would try to make the script portable, and not force bash.

comment:15 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:16 follow-up: Changed 9 years ago by jhpalmieri

If we don't use bash, then the code

if ! [ -f "$SAGE_ROOT/spkg/base/$TARGET.tar" ]; then

needs to be changed: it gives an error when run on Solaris:

base/prereq-0.9-install: !: not found

(The error is ignored and installation continues, but this should still be fixed.)

comment:17 in reply to: ↑ 16 Changed 9 years ago by jdemeyer

Okay, un-negated the test and swapped the "if" and "else" clauses.

comment:18 Changed 9 years ago by jhpalmieri

(After a little experimentation, I guess putting the ! inside the test would have worked, too: if [ ! -f ... ].)

comment:19 in reply to: ↑ 14 Changed 9 years ago by drkirkby

Replying to jdemeyer:

Concerning bash: maybe we shouldn't require bash in the prereq script. The prereq script checks whether bash is available and gives a clear error message if it's not. So I would try to make the script portable, and not force bash.

Agreed. Someone had ideas of converting sage to use the autotools (or autohell to William). The tools are carefully written to be portable, so the last thing we should do is force any non-portable code in there, as it would make it more difficult to use the current script as a basis for others.

comment:20 in reply to: ↑ 13 Changed 9 years ago by drkirkby

Replying to jhpalmieri:

Dave, I'm cc'ing you. I don't know if you have any time these days, but since you worked on the original prereq script, you may be one of the most qualified people to review this, especially the changes in prereq-1.0.diff. (The other changes are pretty straightforward, I think.)

I'll comment there.

comment:21 follow-ups: Changed 9 years ago by drkirkby

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, David Kirkby

A few comments:

1) Testing with:

if [ "$SAGE_BUILD_DIR" = "" ]; then 

is bad practice. Testing on "" can fail under some admittedly obscure circumstances. Better would be

if [ "x$SAGE_BUILD_DIR" = x ]; then 

2) Are we 100% sure m4 is not needed? I have some recollection of it being needed somewhere. If not, this http://www.sagemath.org/doc/faq/faq-usage.html needs modifying, as that specifically says to install m4. The configure script was clearly written with this in mind too.

I can't judge all the implications of all the changes, but looking at the diffs only, I can't see anything that looks as though it will be a problem.

A couple of minor points, one could address if one wanted to:

a) The Solaris page should now really be to Oracle and not Sun, but it's no big deal.

b) Although William was annoyed when Sage would not build with gcc 3.4 series due to "ratpoints", and was keen to get Sage working with gcc 3.4.x, I think the chances of that happening are very remote now. There is too much code that will not compile with such an old version of gcc. So one could consider removing all the checks associated with gcc 3.4.x, and simply accept that gcc 4.0.1 is the minimum version. That could take quite a bit of code out, and would mean that SAGE_USE_OLD_GCC could be removed from the list of environment variables.

So I can't see anything wrong, with the exception of how SAGE_BUILD_DIR is tested.

Assuming John and Jeroen are happy with each others changes, and the SAGE_BUILD_DIR issue is fixed, I think this should be set to positive review.

Dave

comment:22 Changed 9 years ago by jhpalmieri

  • Dependencies changed from #12739 to #12739, #12785

comment:23 Changed 9 years ago by jhpalmieri

See #12785 for the FAQ issue. Regarding tests like if [ x"..." = x ]; then, if we were sure to be using bash, I would feel comfortable omitting the x's, but since we're not using bash here, perhaps we should include the x's.

comment:24 Changed 9 years ago by jdemeyer

Ok, I'm making the following changes:

  • spkg/base/prereq-1.0-install

    a b  
    55
    66
    77TARGET=prereq-1.0
    8 if [ "$SAGE_BUILD_DIR" = "" ]; then
     8if [ "x$SAGE_BUILD_DIR" = x ]; then
    99    SAGE_BUILD_DIR="$SAGE_ROOT/spkg/build"
    1010fi
    1111mkdir -p "$SAGE_BUILD_DIR"
     
    2222echo "Starting prerequisite check."
    2323echo "Machine: `uname -a`"
    2424
    25 if [ "$SAGE_PORT" = "" ]; then
     25if [ "x$SAGE_PORT" = x ]; then
    2626    if [ `uname | sed -e 's/WIN.\+/WIN/'` = "CYGWIN" ]; then
    2727        echo "Unfortunately, building SAGE on Cygwin is not currently supported,"
    2828        echo "though we are actively working on supporting it.  If you would like"
     
    138138    fi
    139139
    140140    # If we are installing GCC, skip all compiler checks in ./configure
    141     if [ "$SAGE_BUILD_TOOLCHAIN" = yes ]; then
     141    if [ "x$SAGE_BUILD_TOOLCHAIN" = xyes ]; then
    142142        PREREQ_OPTIONS="--disable-compiler-checks $PREREQ_OPTIONS"
    143143    fi
    144144

Changed 9 years ago by jdemeyer

comment:25 in reply to: ↑ 21 Changed 9 years ago by jdemeyer

Replying to drkirkby:

b) Although William was annoyed when Sage would not build with gcc 3.4 series due to "ratpoints", and was keen to get Sage working with gcc 3.4.x, I think the chances of that happening are very remote now. There is too much code that will not compile with such an old version of gcc. So one could consider removing all the checks associated with gcc 3.4.x, and simply accept that gcc 4.0.1 is the minimum version. That could take quite a bit of code out, and would mean that SAGE_USE_OLD_GCC could be removed from the list of environment variables.

I would wait with this until #12369 is merged.

comment:26 in reply to: ↑ 21 Changed 9 years ago by jdemeyer

Replying to drkirkby:

Assuming John and Jeroen are happy with each others changes, and the SAGE_BUILD_DIR issue is fixed, I think this should be set to positive review.

Done!

comment:27 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:28 Changed 9 years ago by jdemeyer

  • Dependencies changed from #12739, #12785 to #12739
  • Description modified (diff)

I wouldn't say #12785 is a dependency, it's more like a follow-up.

comment:29 Changed 9 years ago by jdemeyer

  • Dependencies changed from #12739 to #12739, to be merged with #12369

comment:30 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_work

I was wrong about Sage not requiring m4. At least MPIR does require m4 (maybe older versions which I tested initially didn't require m4).

comment:31 Changed 9 years ago by jdemeyer

  • Description modified (diff)

Changed 9 years ago by jdemeyer

Diff for the prereq tarball, for review only

comment:32 Changed 9 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Restored the m4 check.

comment:33 Changed 9 years ago by jdemeyer

Can somebody please review this again? The only change with respect to the old version was to restore the check for m4.

comment:34 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to positive_review

David Kirkby sent this is an email:

Hi,
I see your comment about wanting the update reviewed, and the only
change was checking for m4 again. I'm at work now, and don't have my
Sage trac password with me, so I can't update the ticket. Please
consider I have given this a positive review. I suggest you copy this
email, and put it in the trac ticket, and change it to positive
yourself

Dave

comment:35 Changed 9 years ago by jdemeyer

See #12814 for a blocker follow-up: prereq-0.9-install should be added to .hgignore for upgraded Sage versions.

comment:36 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.0.beta13
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.