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:

Status badges

Description (last modified by jdemeyer)

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 jdemeyer

  • Branch set to u/jdemeyer/cython____do_not_include__pxi_files

comment:2 Changed 5 years ago by jdemeyer

  • Commit set to 0a622e6c9c1c5fc6b6c2b12bb7de4c6e7f2b4702
  • Status changed from new to needs_review

New commits:

0a622e6cython(): do not include .pxi files

comment:3 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Priority changed from major to blocker

comment:4 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:5 follow-up: Changed 5 years ago by tscrim

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 jdemeyer

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: Changed 5 years ago by tscrim

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 jdemeyer

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 tscrim

  • 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 vbraun

  • Status changed from positive_review to needs_work

See the patchbot errors, breaks spyx compilation

comment:11 Changed 5 years ago by jdemeyer

  • Dependencies set to #22695

comment:12 Changed 5 years ago by git

  • Commit changed from 0a622e6c9c1c5fc6b6c2b12bb7de4c6e7f2b4702 to 9c66c20b0e7424bde20c2b4e4ac60e940deb7980

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f2bcabdUpgrade to cysignals version 1.6.3
8c05957Fix doctest for cysignals upgrade
9745f93cython(): do not include .pxi files
9c66c20Fix .spyx doctest

comment:13 Changed 5 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:14 Changed 5 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:15 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/cython____do_not_include__pxi_files to 9c66c20b0e7424bde20c2b4e4ac60e940deb7980
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.