Opened 5 years ago
Closed 5 years ago
#22805 closed enhancement (fixed)
cython(): do not include .pxi files
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-8.0 |
Component: | cython | Keywords: | |
Cc: | Merged in: | ||
Authors: | Jeroen Demeyer | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | 9c66c20 (Commits, GitHub, GitLab) | Commit: | 9c66c20b0e7424bde20c2b4e4ac60e940deb7980 |
Dependencies: | #22695 | Stopgaps: |
Description (last modified by )
The function cython()
currently adds a header to every .pyx
file:
include "cdefs.pxi" include "cysignals/signals.pxi" # ctrl-c interrupt block support include "stdsage.pxi"
If we want cython()
to become less Sage-specific (see for example #22461), we should stop doing this. The user can still manually add the includes.
Moreover, Cython is trying to phase out .pxi
files in general, which is an additional reason to get of those headers.
Note that I don't see an easy way to make this backwards-compatible. The only thing that we can do is to give a good error message (which is what this patch does). Because it's a backwards-incompatible change, it should preferably be done in a major version bump (i.e. Sage 8.0). Therefore I am making this a blocker.
Change History (15)
comment:1 Changed 5 years ago by
- Branch set to u/jdemeyer/cython____do_not_include__pxi_files
comment:2 Changed 5 years ago by
- Commit set to 0a622e6c9c1c5fc6b6c2b12bb7de4c6e7f2b4702
- Status changed from new to needs_review
comment:3 Changed 5 years ago by
- Description modified (diff)
- Priority changed from major to blocker
comment:4 Changed 5 years ago by
- Description modified (diff)
comment:5 follow-up: ↓ 6 Changed 5 years ago by
There isn't a way to add the corresponding cimport
statements to the code before sending it off to Cython? I'm not against this change, but I don't see why we can't have the same functionality available from Sage (mostly out of ignorance).
comment:6 in reply to: ↑ 5 Changed 5 years ago by
Replying to tscrim:
There isn't a way to add the corresponding
cimport
statements to the code before sending it off to Cython?
Yes, of course there is. We could just add cimport
statements instead of include
statements.
But my point is that it shouldn't be the business of Sage to add that. This is especially true in the light of #22461. I think that the user can easily add those cimports
himself.
comment:7 follow-up: ↓ 8 Changed 5 years ago by
Why shouldn't we have Sage do a little bit extra on top of standard Cython to make things easier for our users? Even with this change, I don't think we could completely separate cython()
from standard Cython because we give the users access to our standard Sage classes. I guess I don't see the point of making things harder for (more basic) users.
comment:8 in reply to: ↑ 7 Changed 5 years ago by
Replying to tscrim:
we give the users access to our standard Sage classes.
First of all, the relevant .pxi
files have almost nothing to do with Sage: they add support for cysignals
(i.e. the sig_check
/sig_on
stuff and memory functions like sig_malloc
), some arbitrary set of C functions like memset
and fopen
and all GMP functions like mpz_init
. So there is really nothing Sage-specific about this.
Second, even if you argue that cython()
should add those things, it should still be possible to just use plain Cython without any extra cimports. This could be implemented with an additional option to cython()
. I already thought about this when starting this ticket, but then thought that Sage changing Cython files is just a misfeature.
comment:9 Changed 5 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Okay, then I'm willing to set this to a positive review. I doubt it was a much used (mis)feature, but we will see in the beta stages I think.
comment:10 Changed 5 years ago by
- Status changed from positive_review to needs_work
See the patchbot errors, breaks spyx compilation
comment:11 Changed 5 years ago by
- Dependencies set to #22695
comment:12 Changed 5 years ago by
- Commit changed from 0a622e6c9c1c5fc6b6c2b12bb7de4c6e7f2b4702 to 9c66c20b0e7424bde20c2b4e4ac60e940deb7980
comment:13 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:14 Changed 5 years ago by
- Status changed from needs_review to positive_review
comment:15 Changed 5 years ago by
- Branch changed from u/jdemeyer/cython____do_not_include__pxi_files to 9c66c20b0e7424bde20c2b4e4ac60e940deb7980
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
cython(): do not include .pxi files