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>
@ZeroIntensity just added GC tracking for these objects, as well as made sure the tests passed under refleak on my machine. Hopefully the changes look correct, the GC docs are a bit unclear in a few cases. |
|
🤖 New build scheduled with the buildbot fleet by @AA-Turner for commit 552da2d 🤖 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. |
|
For the buildbot failures, it seems that |
|
makes sense. |
|
If it is feasible to check the zstd lib version being >= 1.4.5 in configure.ac that is also fine (just not building or testing it in that situation). |
|
🤖 New build scheduled with the buildbot fleet by @emmatyping for commit c0648d8 🤖 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. |
|
Commit c0648d8 should fix the failed builds on RHEL 8. I opted to use the newest symbol we use, but I will investigate checking the version itself which I think is long term a better solution. |
|
🤖 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. |
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/