gh-155561: Build most _testlimitedcapi files with Py_TARGET_ABI3T - #155570
gh-155561: Build most _testlimitedcapi files with Py_TARGET_ABI3T#155570vstinner wants to merge 2 commits into
Conversation
The following files are not build with Py_TARGET_ABI3T: * Modules/_testlimitedcapi.c * Modules/_testlimitedcapi/heaptype_relative.c * Modules/_testlimitedcapi/vectorcall_limited.c
|
@encukou @ngoldbaum: Here is a first uncontroversial change to build most of the If this change lands, I plan to write one or multiple PRs to use The 3.15 branch is frozen for release candidate releases. IMO this change should be backported to 3.15, but it can wait for Python 3.15.1. So I will do that once the 3.15 branch will be unblocked. |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
Mostly nitpicks except maybe the comment in parts.h.
I can confirm that everything works as advertised here and the skipped modules besides parts.h all have nontrivial fixes.
| #if !defined(Py_GIL_DISABLED) && !defined(Py_LIMITED_API) | ||
| // need limited C API version 3.5 for PyModule_AddFunctions() | ||
| #ifdef Py_GIL_DISABLED | ||
| // FIXME: use the Py_TARGET_ABI3T |
There was a problem hiding this comment.
I'm not sure if there are also other nontrivial cases buried in here, but this guard is used in abstract.c, bytearray.c, bytes.c, capsule.c, complex.c, dict.c, eval.c, float.c, pyos.c, run.c, set.c, threadstate.c, tuple.c instead of a per-file guard. It's also not clear to me why you added this skip, I'm able to build correctly if I make this use a limited API builds like e.g. object.c.
There was a problem hiding this comment.
In the main branch, Py_TARGET_ABI3T macro is not set. So I'm not sure of what you mean when you say "added this skip"?
If I add # define Py_TARGET_ABI3T 0x030f0000 here, Modules/_testlimitedcapi/heaptype_relative.c and Modules/_testlimitedcapi.c fail to build. I would prefer to not have to add #undef Py_TARGET_ABI3T to these files.
As I wrote, anyway, I plan to write follow-up PRs to use Py_TARGET_ABI3T in all _testlimitedcapi C files.
There was a problem hiding this comment.
I'd prefer not putting FIXMEs in code; we have the issue tracker for that.
|
@ngoldbaum: I tried to address your review. Please review the updated PR. |
encukou
left a comment
encukou
left a comment
There was a problem hiding this comment.
Looks good; two details.
Ideally, run buildbots before merging this one.
| #include "pyconfig.h" // Py_GIL_DISABLED | ||
| #ifdef Py_GIL_DISABLED | ||
| # define Py_TARGET_ABI3T 0x030f0000 | ||
| #else | ||
| // Need limited C API version 3.15 for PyType_FromSlots() | ||
| # define Py_LIMITED_API 0x030f0000 | ||
| #endif |
There was a problem hiding this comment.
If they're the same we shouldn't need the if.
| #include "pyconfig.h" // Py_GIL_DISABLED | |
| #ifdef Py_GIL_DISABLED | |
| # define Py_TARGET_ABI3T 0x030f0000 | |
| #else | |
| // Need limited C API version 3.15 for PyType_FromSlots() | |
| # define Py_LIMITED_API 0x030f0000 | |
| #endif | |
| // Need limited C API version 3.15 for PyType_FromSlots() | |
| #define Py_LIMITED_API 0x030f0000 |
(same in sys.c)
| #if !defined(Py_GIL_DISABLED) && !defined(Py_LIMITED_API) | ||
| // need limited C API version 3.5 for PyModule_AddFunctions() | ||
| #ifdef Py_GIL_DISABLED | ||
| // FIXME: use the Py_TARGET_ABI3T |
There was a problem hiding this comment.
I'd prefer not putting FIXMEs in code; we have the issue tracker for that.
|
Thanks for doing this!
As a test-only change, Hugo would probably approve it -- but you're right, it can definitely wait until 3.15.1. |
3 participants
The following files are not build with Py_TARGET_ABI3T: