Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#11605 closed defect (fixed)

Typos in PARI's spkg-install (2.4.3.alpha.p5)

Reported by: leif Owned by: tbd
Priority: blocker Milestone: sage-4.7.1
Component: packages: standard Keywords: PARI spkg Cygwin
Cc: jdemeyer, dimpase, kcrisman, mhansen Merged in: sage-4.7.1.rc1
Authors: Dmitrii Pasechnik, Leif Leonhardy Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by leif)

Someone should have noticed this:

./spkg-install: line 185: [: missing `]'
Installing PARI/GP...

Here's the culprit:

build()
{
    ...

    $MAKE $PARI_MAKEFLAGS gp
    if [ $? -ne 0]; then
        echo >&2 "Error: building PARI/GP failed."
    fi
}

Note that also an exit 1 (!) is missing there.

(Two patches apply with fuzz 2 btw.)


New spkg: http://spkg-upload.googlecode.com/files/pari-2.4.3.alpha.p7.spkg

md5sum: fd153c3ee354402bb6fc835b9e8ecd9a pari-2.4.3.alpha.p7.spkg

(This spkg is based on / includes the never merged p6 from #10240, see comments below and there.)


Changelog

pari-2.4.3.alpha.p7 (Leif Leonhardy, July 16th, 2011)

  • #11605: Fix typo in spkg-install and add exit 1 (again) in case the build fails.
  • Quote some more variables and filenames in messages.

pari-2.4.3.alpha.p6 (Dima Pasechnik, 22 April, 2011)

  • made a proper check for libpari.dll on Cygwin, as described in trac #10240

Attachments (3)

trac_11605-pari_spkg-install_fixes.spkg.patch (3.4 KB) - added by leif 8 years ago.
SPKG patch. For review only. (Based on a p6 by me with Dima's patch applied.)
trac_11605_combined_with_10240.spkg.patch (5.5 KB) - added by leif 8 years ago.
SPKG patch. Contains both the changes from here and #10240. Apply to pari-2.4.3.alpha.p5 if at all.
pari-2.4.3.alpha.p5-p7.diff (2.5 KB) - added by leif 8 years ago.
Diff between the p5 and the p7. For reference / review.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 8 years ago by leif

  • Status changed from new to needs_review

Patch is up. New spkg is on the way...

comment:2 follow-up: Changed 8 years ago by leif

  • Cc dimpase kcrisman mhansen added

Oh, just came across #10240, which also provided a p6 spkg, but now is marked duplicate since the change made there (for Cygwin) should go into a new PARI 2.5.0 at #11130.

Maybe I should merge that patch (which had positive review already) into my spkg and rename it to p7.

Changed 8 years ago by leif

SPKG patch. For review only. (Based on a p6 by me with Dima's patch applied.)

Changed 8 years ago by leif

SPKG patch. Contains both the changes from here and #10240. Apply to pari-2.4.3.alpha.p5 if at all.

Changed 8 years ago by leif

Diff between the p5 and the p7. For reference / review.

comment:3 in reply to: ↑ 2 Changed 8 years ago by leif

Replying to leif:

Maybe I should merge that patch (which had positive review already) into my spkg and rename it to p7.

Ok, merged the changes from #10240 (see attached diff / cumulative spkg patch) and now going to upload a p7 with all of them applied.

comment:4 Changed 8 years ago by leif

  • Description modified (diff)
  • Keywords PARI spkg Cygwin added

comment:5 Changed 8 years ago by leif

New spkg is up. Please review such that we can get this into the upcoming Sage 4.7.1.

comment:6 follow-up: Changed 8 years ago by kcrisman

I cannot actually test this package on Cygwin for at least two weeks (in fact, I can't on any computer right now), but the change in question for Cygwin looks right still (see #10240, as mentioned), and #10240 does in fact work (I've used it numerous times, and !RegB on the sage-windows list also was able to use it properly). That's as close as I can get to positive review for now, I'm sorry - is that enough? I would say that #10240 would have positive review regardless of whether it is "duplicate" or not.

As for the rest, it looks right, but I have not actually tested any of it, including the quoting etc. I'm sure that someone even a little more advanced in shell script could give it an immediate positive review (or needs work if there is something obvious missed).

Thanks so much for merging the #10240 stuff, by the way!

comment:7 in reply to: ↑ 6 Changed 8 years ago by leif

  • Authors set to Dima Pasechnik, Leif Leonhardy

Replying to kcrisman:

Thanks so much for merging the #10240 stuff, by the way!

I just merged it to attract more reviewers. ;-)

comment:8 Changed 8 years ago by jdemeyer

  • Priority changed from critical to blocker
  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

Good! Never mind the fuzz 2, it is harmless.

comment:9 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.1.rc1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:10 Changed 8 years ago by jdemeyer

  • Authors changed from Dima Pasechnik, Leif Leonhardy to Dmitrii Pasechnik, Leif Leonhardy
Note: See TracTickets for help on using tickets.