Opened 11 years ago

Last modified 11 years ago

#11761 closed enhancement

Upgrade Cython to 0.15.1 — at Version 29

Reported by: Robert Bradshaw Owned by: tbd
Priority: major Milestone: sage-4.8
Component: packages: standard Keywords:
Cc: François Bissey, Jean-Pierre Flori Merged in:
Authors: Robert Bradshaw Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Change History (33)

comment:1 in reply to:  description Changed 11 years ago by Leif Leonhardy

Replying to robertwb:

... including generators.

Yeah!

Changed 11 years ago by Robert Bradshaw

Attachment: 11761-cython-0.15.patch added

comment:2 Changed 11 years ago by Robert Bradshaw

Status: newneeds_review

comment:3 Changed 11 years ago by François Bissey

Cc: François Bissey added

comment:4 Changed 11 years ago by Jean-Pierre Flori

Cc: Jean-Pierre Flori added

comment:5 Changed 11 years ago by Robert Bradshaw

Note that 0.15.1 is around the corner, but it's not as big of a jump.

comment:6 Changed 11 years ago by Leif Leonhardy

Authors: Robert Bradshaw
Description: modified (diff)

Would 0.15.1 require additional changes to Sage?

comment:7 Changed 11 years ago by Jason Grout

Just FYI, 0.15.1 is now released.

comment:8 in reply to:  7 ; Changed 11 years ago by Leif Leonhardy

Replying to jason:

Just FYI, 0.15.1 is now released.

Oh, did you tell Robert?

(Although he said "it's not as big of a jump".)

comment:9 in reply to:  8 ; Changed 11 years ago by Jason Grout

Replying to leif:

Replying to jason:

Just FYI, 0.15.1 is now released.

Oh, did you tell Robert?

Oh, I'm sure he knows, since he's the BFDL for Cython, if there is such a position ;).

(Although he said "it's not as big of a jump".)

comment:10 in reply to:  9 Changed 11 years ago by Leif Leonhardy

Replying to jason:

Replying to leif:

Replying to jason:

Just FYI, 0.15.1 is now released.

Oh, did you tell Robert?

Oh, I'm sure he knows, since he's the BFDL for Cython, if there is such a position ;).

I was just kidding.

(Wonder whether he updates the spkg... :P)

comment:11 Changed 11 years ago by Leif Leonhardy

Summary: Upgrade Cython to 0.15Upgrade Cython to 0.15 or later

:)

comment:12 Changed 11 years ago by Robert Bradshaw

Description: modified (diff)
Summary: Upgrade Cython to 0.15 or laterUpgrade Cython to 0.15.1

No further patches needed for 0.15.1.

Changed 11 years ago by Jeroen Demeyer

comment:13 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)
Milestone: sage-4.7.2sage-4.7.3

Changed 11 years ago by Jeroen Demeyer

Attachment: cython-0.15.1.diff added

Diff for the new Cython spkg, for review only

comment:14 Changed 11 years ago by Jeroen Demeyer

Minor comment: spkg-install is not executable (I know it is made executable upon building, but better fix this in the spkg anyway).

comment:15 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

comment:16 Changed 11 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

Why is the directory tests missing from the Cython source? This should either be fixed, or be documented.

comment:17 Changed 11 years ago by Jeroen Demeyer

What should be done with:

cdef mpz_t denom
mpz_init_set_si(denom, 1)

This code is correct but gives a local variable 'denom' referenced before assignment warning. (there are many of these in sage/matrix/misc.pyx for example)

comment:18 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)
Reviewers: Jeroen Demeyer

comment:19 Changed 11 years ago by Jeroen Demeyer

This new Cython really gives a lot more warnings than the old one, in many cases for code which is perfectly fine. For me, this isn't really a problem, but it is certainly annoying.

comment:20 Changed 11 years ago by Robert Bradshaw

Status: needs_workneeds_review

I've fixed the executable bit on spkg-install and added the tests to the spkg. (I remember someone complaining way back that they were too big, but I don't think it's an issue and we've regularly shipped them before, and it's easier to just have them there.) The spkg at the above link has been refreshed.

The warnings are primarily due to the fact that mpz_t et al are declared as void* rather than arrays of size 1. http://hg.sagemath.org/sage-main/file/2a2abbcad325/sage/libs/gmp/types.pxd#l1 Once this is fixed the warnings should go away (and they are correct with respect to the current declarations). All the other warnings I've noticed are correct too, I simply haven't had the time to fix/update Sage's code.

Your review patch looks fine. You obviously looked at this, is the window closed for 4.7.2?

comment:21 in reply to:  20 Changed 11 years ago by Jeroen Demeyer

Replying to robertwb:

Your review patch looks fine.

In fact, it doesn't. I removed one line too much, fixed now.

You obviously looked at this, is the window closed for 4.7.2?

This will certainly not be merged in sage-4.7.2.

Changed 11 years ago by Jeroen Demeyer

Attachment: 11761_review.patch added

comment:22 in reply to:  20 Changed 11 years ago by Jeroen Demeyer

Status: needs_reviewpositive_review

Replying to robertwb:

The warnings are primarily due to the fact that mpz_t et al are declared as void* rather than arrays of size 1. http://hg.sagemath.org/sage-main/file/2a2abbcad325/sage/libs/gmp/types.pxd#l1 Once this is fixed the warnings should go away (and they are correct with respect to the current declarations).

If there is an easy way to fix this, then we should do this. I'm saying we should fix every single warning but if it is possible to declare mpz_t such that there are no warnings, we should certainly do this.

comment:23 Changed 11 years ago by Jeroen Demeyer

Of course, that should have been "I'm not saying we should fix every single warning"

comment:24 in reply to:  20 Changed 11 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

Replying to robertwb:

I've fixed the executable bit on spkg-install and added the tests to the spkg.

One more thing: changing permissions is a change which should be committed. So could you please do a hg commit and re-upload the spkg? Otherwise, the package and patch looks good.

comment:25 Changed 11 years ago by Jeroen Demeyer

Milestone: sage-4.7.3

Milestone sage-4.7.3 deleted

comment:26 Changed 11 years ago by Robert Bradshaw

Milestone: sage-4.8
Status: needs_workneeds_review

Spkg updated with executable bit set commit.

comment:27 Changed 11 years ago by Robert Bradshaw

Yes, I agree these warnings should be fixed. #11992 .

comment:28 in reply to:  26 Changed 11 years ago by Jeroen Demeyer

Replying to robertwb:

Spkg updated with executable bit set commit.

Looks you forgot to update the spkg, it is exactly the same as before (last modified on 1 november):

$ ls -l /home/robertwb/cython/spkg/cython-0.15.1.spkg
-rw-r--r-- 1 robertwb robertwb 1420704 2011-11-01 06:57 /home/robertwb/cython/spkg/cython-0.15.1.spkg

Never mind, I will make the change.

comment:29 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)
Status: needs_reviewpositive_review
Note: See TracTickets for help on using tickets.