bpo-42111: Make the xxlimited module an example of best extension module practices #23226
Conversation
The old module (compiled with 3.5 API) is available as xxlimited_35.
setup.py
Outdated
| define_macros=[('Py_LIMITED_API', '0x03050000')])) | ||
| define_macros=[('Py_LIMITED_API', '0x03100000')])) | ||
| self.add(Extension('xxlimited_35', ['xxlimited_35.c'], | ||
| define_macros=[('Py_LIMITED_API', '0x03100000')])) |
There was a problem hiding this comment.
Hum, it should be 3.5 rather than 3.10 no?
Why Python 3.5 in specific and not 3.6 for example? Nowadays, 3.6 became the minimum version of many projects, and it's also the Python 3 version of RHEL8 ;-)
There was a problem hiding this comment.
Good catch!
The module uses limited API from Python 3.5. The corresponding stable ABI needs to be still compatible with current Python versions.
| define_macros=[('Py_LIMITED_API', '0x03100000')])) | ||
| else: | ||
| # Debug mode: Build xxlimited with the full API | ||
| # (which is compatible with the limited one) |
There was a problem hiding this comment.
Why not also building with the limited C API?
There was a problem hiding this comment.
./Include/object.h:61:2: error: #error Py_LIMITED_API is incompatible with Py_DEBUG, Py_TRACE_REFS, and Py_REF_DEBUG
I guess that can be fixed, but that's out of scope here (and I don't know what the situation around this is on Windows)
There was a problem hiding this comment.
Oh. Maybe it was needed when the debug build was not ABI compatible with the release build. It has been fixed in Python 3.8. But yeah, it can be addressed in a separated PR.
| self.add(Extension('xxlimited', ['xxlimited.c'], | ||
| define_macros=[('Py_LIMITED_API', '0x03050000')])) | ||
| define_macros=[('Py_LIMITED_API', '0x03100000')])) |
There was a problem hiding this comment.
Is it supposed to be updated when the python version is increased? If yes, maybe PEP 101 should be updated for release managers. (after this PR is merged)
There was a problem hiding this comment.
No. When new things are needed, we should copy xxlimited to e.g. xxlimited_310, add tests, and update xxlimited to the newest version. If new stuff isn't needed in a release, this can stay on the old limited API version.
|
|
| def test_null(self): | ||
| null1 = self.module.Null() | ||
| null2 = self.module.Null() | ||
| self.assertNotEqual(null1, null2) |
There was a problem hiding this comment.
No, it's not a requirement. It's just that other test_xxx.py files have it. You can leave this file as it is.
| define_macros=[('Py_LIMITED_API', '0x03100000')])) | ||
| else: | ||
| # Debug mode: Build xxlimited with the full API | ||
| # (which is compatible with the limited one) |
There was a problem hiding this comment.
Oh. Maybe it was needed when the debug build was not ABI compatible with the release build. It has been fixed in Python 3.8. But yeah, it can be addressed in a separated PR.
| self.add(Extension('xxlimited', ['xxlimited.c'], | ||
| define_macros=[('Py_LIMITED_API', '0x03050000')])) | ||
| define_macros=[('Py_LIMITED_API', '0x03100000')])) |
|
Thank you for the review! |
…ule practices (pythonGH-23226) - Copy existing xxlimited to xxlimited53 (named for the limited API version it uses) - Build both modules, both in debug and release - Test both modules
https://bugs.python.org/issue42111
Automerge-Triggered-By: GH:encukou