FIX: import internal cpython headers to fix for py311 - #4667
Conversation
|
I was fooled by faliing to fully rebuild before and missed in include (I had just removed the line number setting while testing 🤦🏻 ). I am very confused by I could not get the version getting in ModuleSetup.c to work so this is currently broken for every other version 😞 so made a draft. |
4130215 to
952fc89
Compare
|
@scoder I can confirm that this works and squashed it down to one commit. |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
Oh nice, I didn't know that there was already a PR! I wrote a simpler change: https://github.com/cython/cython/pull/4671/files (closed).
| //@requires: ModuleSetupCode.c::IncludeStructmemberH | ||
|
|
||
| #include <frameobject.h> | ||
| #if PY_VERSION_HEX >= 0x030b00a5 |
There was a problem hiding this comment.
Technically, the include will only be needed since 0x030b00a6 but it's fine to include it in Python 3.11 alpha 5. I suggest to define the Py_BUILD_CORE macro to include an internal C API header file. Right now, it's not needed, but I consider requiring it later.
# ifndef Py_BUILD_CORE
# define Py_BUILD_CORE 1
# endif
Same remarks for the other modified files.
There was a problem hiding this comment.
Fair, but then it would not work for me locally. What is the best way to test this sort of thing in the "interstitial" time? I guess I could just locally patch my CPython source to be the next release prematurely?
Should I also include the !CYTHON_COMPILING_IN_PYSTON check everywhere?
There was a problem hiding this comment.
3.11a6 will hopefully be out quite soon. Maybe faster than this gets merged. Not sure when it will become available on Github Actions, though.
In 18b5dd68c6b616257ae243c0b6bb965ffc885a23 / python/cpython#31530 / https://bugs.python.org/issue46836 The `_frame` struct was moved to an internal header, however the public API is primarily read-only, and cython needs to build _frame objects so still import the internal headers.
952fc89 to
fb678f3
Compare
|
I fixed the version gating and addeng the
I did not add this check as it looks like PYSTON is pinned to 3.8? |
I'm still not happy about the fact that we now have to do that. We currently set the macro at a somewhat late point, and since we do it in more than one place, it might happen that other CPython header files get included afterwards (e.g. @vstinner, I think we should unset it directly after including the frame header file. In that case, the
Pyston should be fine. It's based on CPython now. If they need something different in a later version, we'll adapt at that point. |
|
Thanks |
In python/cpython#31530 https://bugs.python.org/issue46836 the `_frame` struct was moved to an internal header, however the public API is primarily read-only, and Cython needs to build PyFrameObjects so still import the internal headers. Also sets the Py_BUILD_CORE define for py311a6, trying to restrict it to the frame header.
|
Backported to 0.29.x in afc00fc |
|
Great, thank you :-) |
|
Please comment on https://discuss.python.org/t/proposal-rename-pyinterpreterframe-struct-as-py-framedata/14213/7 if any of the adjustments proposed there would affect Cython. |
|
Don't worry about my previous comment - we'll make sure code that works against 3.11a6 keeps working until faster-cpython/ideas#309 provides a proper public API for the relevant frame manipulation tasks (although feel free to chime in there on the public API that you'd like to see to avoid having to delve into frame object internals) |
4 participants
In 18b5dd68c6b616257ae243c0b6bb965ffc885a23 /
python/cpython#31530 /
https://bugs.python.org/issue46836
The
_framestruct was moved to an internal header, however the public API isprimarily read-only, and cython needs to build _frame objects so still import
the internal headers.
The version bounding is wrong (the problem commit won't be out until a6, but this condition will compile against the current CPYthon main branch.
I'm not at all sure that this is the right fix, but it compiled!