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

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

Closed
nexttime mannequin opened this issue Jul 16, 2011 · 16 comments
Closed

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

nexttime mannequin opened this issue Jul 16, 2011 · 16 comments

Comments

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jul 16, 2011

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)

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

CC: @jdemeyer @dimpase @kcrisman @mwhansen

Component: packages: standard

Keywords: PARI spkg Cygwin

Author: Dmitrii Pasechnik, Leif Leonhardy

Reviewer: Jeroen Demeyer

Merged: sage-4.7.1.rc1

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

@nexttime nexttime mannequin added this to the sage-4.7.1 milestone Jul 16, 2011
@nexttime
Copy link
Mannequin Author

nexttime mannequin commented Jul 16, 2011

comment:1

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

@nexttime nexttime mannequin added the s: needs review label Jul 16, 2011
@nexttime
Copy link
Mannequin Author

nexttime mannequin commented Jul 16, 2011

comment:2

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.

@nexttime
Copy link
Mannequin Author

nexttime mannequin commented Jul 16, 2011

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

@nexttime
Copy link
Mannequin Author

nexttime mannequin commented Jul 16, 2011

Attachment: trac_11605-pari_spkg-install_fixes.spkg.patch.gz

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

@nexttime
Copy link
Mannequin Author

nexttime mannequin commented Jul 16, 2011

Attachment: trac_11605_combined_with_10240.spkg.patch.gz

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

@nexttime
Copy link
Mannequin Author

nexttime mannequin commented Jul 16, 2011

comment:3

Attachment: pari-2.4.3.alpha.p5-p7.diff.gz

Replying to @nexttime:

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.

@nexttime
Copy link
Mannequin Author

nexttime mannequin commented Jul 16, 2011

Changed keywords from none to PARI spkg Cygwin

@nexttime

This comment has been minimized.

@nexttime nexttime mannequin added the c: porting: cygwin label Jul 16, 2011
@nexttime
Copy link
Mannequin Author

nexttime mannequin commented Jul 16, 2011

comment:5

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

@kcrisman
Copy link
Member

comment:6

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!

@nexttime
Copy link
Mannequin Author

nexttime mannequin commented Jul 16, 2011

comment:7

Replying to @kcrisman:

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

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

@nexttime
Copy link
Mannequin Author

nexttime mannequin commented Jul 16, 2011

Author: Dima Pasechnik, Leif Leonhardy

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@jdemeyer
Copy link

comment:8

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

@jdemeyer
Copy link

Merged: sage-4.7.1.rc1

@jdemeyer
Copy link

Changed author from Dima Pasechnik, Leif Leonhardy to Dmitrii Pasechnik, Leif Leonhardy

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

2 participants