gh-132983: Introduce _zstd bindings module
#133027
Conversation
_zstd module_zstd bindings module
|
E: now fixed |
|
Thank you for the review @ZeroIntensity! Some of the decisions are just how things were in |
|
There's probably not too much to read about it other than somewhere in PEP-703. You'll just want to replace the locks with critical sections, which have some magic to prevent deadlocks. |
There was a problem hiding this comment.
Some other comments I managed to find. I didn't review the decompressor but I think most of my comments that I said for the compressor would apply. The most important ones are essentially function signatures where I want you to use PyObject *self and do a manual cast to the expected type inside the function:
#define MyType_CAST(op) ((MyType *)(op))
static void
my_method(PyObject *op) {
MyType *self = MyType_CAST(op);
...
}The use of a macro (or a static inline function, up to you) is to allow future runtime type checks.
This commit introduces the `_zstd` module, with bindings to libzstd from the pyzstd project. It also includes the unix build system configuration. Windows build system support will be integrated independently as it depends on integration with cpython-source-deps.
Also removes module state references from the classes in the _zstd module and instead uses PyType_GetModuleState()
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
This should avoid races and deadlocks.
The `compress`/`decompress` functions will be moved to Python code for simplicity. C implementations can always be re-added in the future. Also, mark _zstd as not requiring the GIL.
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
|
🤖 New build scheduled with the buildbot fleet by @emmatyping for commit d149b2c 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133027%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
|
🤖 New build scheduled with the buildbot fleet by @emmatyping for commit 30b3934 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133027%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
|
Okay, latest commit should actually work correctly on RHEL 8, I tested it in a container and verified |
|
🤖 New build scheduled with the buildbot fleet by @emmatyping for commit 10e2e80 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133027%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
|
Okay, configure should now work across all platforms, that was a bit more involved than I was expecting. I wish the configure feedback loop wasn't so long :( |
This comment was marked as abuse.
This comment was marked as abuse.
|
The RHEL8 s390x LTO+PGO failure is unrelated, should be #133261 (comment) (other RHEL 8 s390x may fail too for the same reason?) But the changes have worked on RHEL8: https://buildbot.python.org/#/builders/420/builds/1735
|
I normally pass |
TIL! Thanks that is really handy. |
|
Yay! Congrats for getting this accepted and into 3.14 on time @emmatyping |
|
Thank you, as well as everyone who reviewed! The feedback was incredibly valuable. Now on to merging the Python code: #133365 |
This is part 2 (but parallel to part 1) for implementing PEP 784. This is probably the meat of the implementation making up ~half of the LOC in the overall diff.
This change:
_zstdmodule inModules/_zstd. Tests will be included when the Pythoncompression.zstdmodule is added. If people think the tests should be included, I can merge thecompression.zstdchanges into the PR, but I wanted to separate them to make the PRs a bit more manageable.I added skip news as I'd like to write a holistic NEWS/What's New entry once the entire implementation has landed. If people think each PR should have NEWS I can write something up.
TODO before merge:
_zstdgets installed📚 Documentation preview 📚: https://cpython-previews--133027.org.readthedocs.build/