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

Upgrade lcalc to work with Pari svn snapshot 12577 - a pre-release of Pari 2.4.3 #9592

Closed
jdemeyer opened this issue Jul 24, 2010 · 42 comments
Closed

Comments

@jdemeyer
Copy link

After upgrading PARI/GP to a snapshot based on a pre-release of 2.4.3 (#9343), lcalc no longer compiles properly.

See http://wiki.sagemath.org/NewPARI for more information and links.

Upstream: Reported upstream. Little or no feedback.

CC: @JohnCremona

Component: packages: standard

Author: John Cremona, Jeroen Demeyer, Rishikesh

Reviewer: Jeroen Demeyer, Leif Leonhardy, Mitesh Patel

Merged: sage-4.6.alpha0

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

@jdemeyer
Copy link
Author

Attachment: lcalc-newpari.patch.gz

@jdemeyer
Copy link
Author

Upstream: Reported upstream. Little or no feedback.

@jdemeyer
Copy link
Author

comment:1

New version which works with PARI 2.4.3: http://cage.ugent.be/~jdemeyer/sage/lcalc-20100428-1.23.p1.spkg

I have also notified the upstream contact Michael Rubinstein and sent him the patch lcalc-newpari.patch

@jdemeyer jdemeyer self-assigned this Jul 24, 2010
@jdemeyer jdemeyer modified the milestones: sage-4.5.2, sage-4.6 Jul 29, 2010
@jdemeyer
Copy link
Author

Author: Jeroen Demeyer

@jdemeyer

This comment has been minimized.

@qed777
Copy link
Mannequin

qed777 mannequin commented Aug 13, 2010

comment:5

The patch level for the new spkg should be 2, since we used p1 for #9665. This should fix the "already installed" problem reported by John Cremona in comment 180 at #9343.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 13, 2010

comment:6

Replying to @qed777:

The patch level for the new spkg should be 2, since we used p1 for #9665. This should fix the "already installed" problem reported by John Cremona in comment 180 at #9343.

Just noticed that, too. :)

There are also post-merge comments at #9665, I don't know if they should be included here or if there's even a new ticket for these.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 13, 2010

comment:7

Jeroen, could you describe in more detail which failure(s) this ticket is intended to fix (including Sage version, operating system, processor etc.)?

John Cremona has successfully installed and tested #9343 (unintentionally) without this new spkg, and I've also successfully installed the other to spkgs and applied the patches from #9343 on top of Sage 4.5.3.alpha0 + #9475 and #9717 on Fedora 13 x86 (Pentium 4 Prescott, gcc 4.4.4).

Of course lcalc wasn't (re)built in the above tests. (I'll try that later, currently running ptestlong without the lcalc package from here.)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 13, 2010

comment:8

Just for the record: SPKG.txt currently lacks a Dependencies section, the package has no spkg-check, and spkg-install uses make, not $MAKE...

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 13, 2010

comment:9

In addition, it contains at least two or three files that should be deleted: some *.bak file, a *.swap.crap file and some static library (*.a) I think we don't need (correct me if I'm wrong here).

(In case Jeroen's patch here is now obsolete due to a meanwhile newer PARI version at #9343, which John Cremona is currently investigating, we could recycle this ticket to address the above mentioned issues.)

@JohnCremona
Copy link
Member

comment:10

Updated spkg: http://www.warwick.ac.uk/staff/J.E.Cremona/lcalc-20100428-1.23.p2.spkg

(This version is based on lcalc-20100428-1.23.p1.spkg as merged in 4.5.2)

I made this before seeing the recent comments here. Feel free to add the Dependencies section etc -- I will not have time to do that for at least a day.

@qed777
Copy link
Mannequin

qed777 mannequin commented Aug 13, 2010

comment:11

Replying to @nexttime:

There are also post-merge comments at #9665, I don't know if they should be included here or if there's even a new ticket for these.

I've added a note at #9665 about this ticket.

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Aug 13, 2010

comment:12

I've changed the title and description a bit, to reflect the fact that #9343 is not Pari 2.4.3.

Not even Pari 2.4.2 has ever been released - there is only an alpha of that available.

Dave

@sagetrac-drkirkby

This comment has been minimized.

@sagetrac-drkirkby sagetrac-drkirkby mannequin changed the title Upgrade lcalc to pari 2.4.3 Upgrade lcalc to work with Pari svn snapshot 12577 - a pre-release of Pari 2.4.3 Aug 13, 2010
@sagetrac-drkirkby

This comment has been minimized.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 13, 2010

comment:14

Replying to @sagetrac-drkirkby:

I've changed the title and description a bit, to reflect the fact that #9343 is not Pari 2.4.3.

You should perhaps have updated the spkg link to point to John Cremona's new p2 spkg, too. ;-)

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Aug 14, 2010

comment:15

Replying to @nexttime:

Replying to @sagetrac-drkirkby:

I've changed the title and description a bit, to reflect the fact that #9343 is not Pari 2.4.3.

You should perhaps have updated the spkg link to point to John Cremona's new p2 spkg, too. ;-)

I don't know where it is. In any case, it should not be a .p2, since the current one in Sage is lcalc-20100428-1.23.p0.spkg, to a revision should be called lcalc-20100428-1.23.p1.spkg.

I hope the

gcc -Wa,-W

(to suppress warnings from the assembler), has not got back in, as -W is not recognised by the Sun assembler and it creates an error.

Dave

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 14, 2010

comment:16

Replying to @sagetrac-drkirkby:

Replying to @nexttime:

Replying to @sagetrac-drkirkby:

I've changed the title and description a bit, to reflect the fact that #9343 is not Pari 2.4.3.

You should perhaps have updated the spkg link to point to John Cremona's new p2 spkg, too. ;-)

I don't know where it is. In any case, it should not be a .p2, since the current one in Sage is lcalc-20100428-1.23.p0.spkg, to a revision should be called lcalc-20100428-1.23.p1.spkg.

Browser cache issue?

See #9592 comment:10 (where)

and #9592 comment:5 (why).

(lcalc-20100428-1.23.p1.spkg from #9665 was merged into Sage 4.5.2.rc1)

@jdemeyer
Copy link
Author

comment:17

I removed some .DS_Store and ._.DS_Store files from John's spkg and uploaded the result to http://cage.ugent.be/~jdemeyer/sage/lcalc-20100428-1.23.p2.spkg

@jdemeyer

This comment has been minimized.

@rishikesha
Copy link
Mannequin

rishikesha mannequin commented Aug 14, 2010

comment:19

I can see that this spkg does not depend on the upgrade to the new pari. This can be included before the latest version of pari is accepted. In couple of month, I will try to get Mike to use autotools for building. This will eliminate a lot of problems with spkg as of now. I am changing the status to needs review if it is ok with you.

@rishikesha rishikesha mannequin added the s: needs review label Aug 14, 2010
@rishikesha
Copy link
Mannequin

rishikesha mannequin commented Aug 14, 2010

comment:20

Following suggestion of Mitesh, I have small some small cleaning up of unnecessary files in patches and few lines in spkg-install over the changes of jdemeyer.

http://sage.math.washington.edu/home/rishikesh/lcalc/lcalc-20100428-1.23.p2.spkg

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 14, 2010

comment:21

Replying to @rishikesha:

I can see that this spkg does not depend on the upgrade to the new pari. This can be included before the latest version of pari is accepted. In couple of month, I will try to get Mike to use autotools for building. This will eliminate a lot of problems with spkg as of now. I am changing the status to needs review if it is ok with you.

Perhaps do some of the clean-up I suggested above?

There are further minor things (like the date/version at the top of spkg-install; SAGE_DEBUG=yes usually disables optimization, unquoted environment variables, etc.).

I wonder if we should add (a) further patch(es) to get rid of some of the annoying warnings (cf. #9343 comment:191 ff.), but we probably shouldn't do too much at this ticket.

I'm not sure if Cygwin support is required yet... ;-)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 14, 2010

comment:22

Replying to @rishikesha:

Following suggestion of Mitesh, I have small some small cleaning up of unnecessary files in patches and few lines in spkg-install over the changes of jdemeyer.

http://sage.math.washington.edu/home/rishikesh/lcalc/lcalc-20100428-1.23.p2.spkg

Could you upload an spkg patch for your changes (except file deletions) here?

It's a bit more convenient for reviewing and adding further changes...

@jdemeyer
Copy link
Author

comment:23

Replying to @rishikesha:

Following suggestion of Mitesh, I have small some small cleaning up of unnecessary files in patches and few lines in spkg-install over the changes of jdemeyer.

http://sage.math.washington.edu/home/rishikesh/lcalc/lcalc-20100428-1.23.p2.spkg

I copied your spkg to http://cage.ugent.be/~jdemeyer/sage/lcalc-20100428-1.23.p2.spkg (like this, I don't have to update the descriptions of #9343 and #9592).

@rishikesha
Copy link
Mannequin

rishikesha mannequin commented Aug 14, 2010

comment:24

Attachment: spkg-install.diff.gz

I am not sure what you want to be done. Can you make the changes and attach it here.

Replying to @nexttime:

Replying to @rishikesha:

I can see that this spkg does not depend on the upgrade to the new pari. This can be included before the latest version of pari is accepted. In couple of month, I will try to get Mike to use autotools for building. This will eliminate a lot of problems with spkg as of now. I am changing the status to needs review if it is ok with you.

Perhaps do some of the clean-up I suggested above?

There are further minor things (like the date/version at the top of spkg-install; SAGE_DEBUG=yes usually disables optimization, unquoted environment variables, etc.).

I wonder if we should add (a) further patch(es) to get rid of some of the annoying warnings (cf. #9343 comment:191 ff.), but we probably shouldn't do too much at this ticket.

I'm not sure if Cygwin support is required yet... ;-)

@qed777 qed777 mannequin added p: blocker / 1 and removed p: major / 3 labels Aug 19, 2010
@jdemeyer
Copy link
Author

Attachment: lcalc-spkg.patch.gz

Complete spkg patch (for reference)

@jdemeyer

This comment has been minimized.

@mwhansen
Copy link
Contributor

comment:27

I've made an spkg that builds on Cygwin at #9775 based on the one here. It might make more sense to make any additional changes to the SPKG there.

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 2, 2010

comment:28

Replying to @mwhansen:

I've made an spkg that builds on Cygwin at #9775 based on the one here. It might make more sense to make any additional changes to the SPKG there.

See #9845, too.

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 2, 2010

comment:29

Rishi, do Jeroen's changes look good to you? If they are, I suggest that we leave further changes for other tickets.

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 2, 2010

Changed author from Jeroen Demeyer to Jeroen Demeyer, Rishi

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 2, 2010

Changed author from Jeroen Demeyer, Rishi to Jeroen Demeyer, Rishikesh

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 3, 2010

comment:31

Also, the dist/ (Debian) directory should be removed, cf. #5903.

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 4, 2010

comment:32

Replying to @nexttime:

Also, the dist/ (Debian) directory should be removed, cf. #5903.

I suggest that we do this at #9845, unless we otherwise need to update the package here.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 4, 2010

comment:33

Replying to @qed777:

Replying to @nexttime:

Also, the dist/ (Debian) directory should be removed, cf. #5903.

I suggest that we do this at #9845, unless we otherwise need to update the package here.

Yes; hopefully #9845 gets reviewed soon s.t. this ticket won't get merged at all (positively reviewed though), since the former contains all changes from here.

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 9, 2010

comment:34

Do we have a positive review here?

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 9, 2010

Changed author from Jeroen Demeyer, Rishikesh to John Cremona, Jeroen Demeyer, Rishikesh

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 9, 2010

comment:35

Doesn't bother me, but should we keep

# disable Cygwin build for now
if [ "$UNAME" = "CYGWIN" ]; then
#    cp ../../patches/Lcommandline_elliptic.cc .
    echo "Sorry, the lcalc build is currently broken"
    echo 1
fi

? (In case I by luck have looked at the current .p2 / its current patches/diffs.)

Fortunately there's a follow-up ticket to address the rest...

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 10, 2010

comment:36

Is patches/Lcommandline_elliptic.cc.cygwin.diff up to date? Do we still use patches/Lcommandline_elliptic.cc.cygwin.*. Let's do any necessary updates at #9845.

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 10, 2010

Reviewer: Jeroen Demeyer, Leif Leonhardy, Mitesh Patel

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 10, 2010

Merged: sage-4.6.alpha0

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

3 participants