Opened 4 years ago

Closed 4 years ago

#26402 closed enhancement (fixed)

Set language_level for cython() command

Reported by: Jeroen Demeyer Owned by:
Priority: major Milestone: sage-8.4
Component: python3 Keywords:
Cc: Erik Bray, Frédéric Chapoton Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 3bb37c1 (Commits, GitHub, GitLab) Commit: 3bb37c135c55197ac824c7b019065f9a438acd5e
Dependencies: Stopgaps:

Status badges

Description

Explicitly set the default language_level for the cython() to that of the currently running Python version. This is needed because Cython 0.29 will warn if no language_level is given.

It makes sense to use language_level=3 when running Sage on Python 3 to have the user's Cython code run more consistently with the user's Python code. By keeping language_level=2 on Python 2, this is not a breaking change.

Change History (15)

comment:1 Changed 4 years ago by Jeroen Demeyer

Branch: u/jdemeyer/set_language_level_for_cython___command

comment:2 Changed 4 years ago by Jeroen Demeyer

Commit: 3bb37c135c55197ac824c7b019065f9a438acd5e
Status: newneeds_review

New commits:

3bb37c1Explicitly set Cython language level to current Python version

comment:3 Changed 4 years ago by Jeroen Demeyer

(edit: wrong ticket)

Last edited 4 years ago by Jeroen Demeyer (previous) (diff)

comment:4 Changed 4 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

Makes sense.

comment:5 Changed 4 years ago by Erik Bray

I thought you were against this, last I proposed it. What changed your mind? Are you sure this won't cause a huge regression in the ongoing Python 3 port?

comment:6 Changed 4 years ago by Erik Bray

Nevermind, I misunderstood. This is just for cython(...), not the Sage sources themselves.

comment:7 Changed 4 years ago by Frédéric Chapoton

Has this been tested against a python3 build of sage, just to be sure ?

comment:8 Changed 4 years ago by Travis Scrimshaw

No, I don't have a python3 build.

comment:9 Changed 4 years ago by Travis Scrimshaw

Well, to be specific, I did not test it on python3. I don't know if Jeroen did or not.

comment:10 Changed 4 years ago by Jeroen Demeyer

I haven't really been following the Python 3 porting effort lately, so I don't even know what testing against a python3 build of sage would mean.

comment:11 Changed 4 years ago by Frédéric Chapoton

This would mean

1) check that sage still builds with python3 with this branch applied. If not, I would not agree with the ticket.

2) check that the number of failing modules and doctests does not grow. If it grows or explodes, I would be rather unhappy. There is a script on #26212 that can extract the data from a doctest log.

Instructions for building sage with python3 are at the bottom of

https://wiki.sagemath.org/Python3-compatible%20code

comment:12 Changed 4 years ago by Jeroen Demeyer

Can't we just apply this branch and then fix any failing doctests later? There are only a few tests using cython(), so there cannot be a lot of breakage.

comment:13 Changed 4 years ago by Jeroen Demeyer

In Python 3, I manually checked all doctests from the regular expression sage:.*cython[(] and didn't see any breakage.

comment:14 Changed 4 years ago by Erik Bray

I agree; I think it's low probability of breaking much.

comment:15 Changed 4 years ago by Volker Braun

Branch: u/jdemeyer/set_language_level_for_cython___command3bb37c135c55197ac824c7b019065f9a438acd5e
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.