Opened 4 years ago
Closed 4 years ago
#26402 closed enhancement (fixed)
Set language_level for cython() command
| Reported by: | jdemeyer | Owned by: | |
|---|---|---|---|
| Priority: | major | Milestone: | sage-8.4 |
| Component: | python3 | Keywords: | |
| Cc: | embray, 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
- Branch set to u/jdemeyer/set_language_level_for_cython___command
comment:2 Changed 4 years ago by
- Commit set to 3bb37c135c55197ac824c7b019065f9a438acd5e
- Status changed from new to needs_review
comment:4 Changed 4 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Makes sense.
comment:5 Changed 4 years ago by
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
Nevermind, I misunderstood. This is just for cython(...), not the Sage sources themselves.
comment:7 Changed 4 years ago by
Has this been tested against a python3 build of sage, just to be sure ?
comment:8 Changed 4 years ago by
No, I don't have a python3 build.
comment:9 Changed 4 years ago by
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
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
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
comment:12 Changed 4 years ago by
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
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
I agree; I think it's low probability of breaking much.
comment:15 Changed 4 years ago by
- Branch changed from u/jdemeyer/set_language_level_for_cython___command to 3bb37c135c55197ac824c7b019065f9a438acd5e
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Explicitly set Cython language level to current Python version