-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-132983: Introduce _zstd
bindings module
#133027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
_zstd
module_zstd
bindings module
E: now fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Some comments at a glance.
Please remove the module state macros. They make this really difficult to review, and don't really add much (is MS_MEMBER(whatever)
really that much easier to type than state->whatever
?)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few preliminary comments, although I'm not sure it can be modified under the given license.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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 |
* origin/main: (111 commits) pythongh-91048: Add filename and line number to external inspection routines (pythonGH-133385) pythongh-131178: Add tests for `ast` command-line interface (python#133329) Regenerate pcbuild.sln in Visual Studio 2022 (python#133394) pythongh-133042: disable HACL* HMAC on Emscripten (python#133064) pythongh-133351: Fix remote PDB's multi-line block tab completion (python#133387) pythongh-109700: Improve stress tests for interpreter creation (pythonGH-109946) pythongh-81793: Skip tests for os.link() to symlink on Android (pythonGH-133388) pythongh-126835: Rename `ast_opt.c` to `ast_preprocess.c` and related stuff after moving const folding to the peephole optimizier (python#131830) pythongh-91048: Relax test_async_global_awaited_by to fix flakyness (python#133368) pythongh-132457: make staticmethod and classmethod generic (python#132460) pythongh-132805: annotationlib: Fix handling of non-constant values in FORWARDREF (python#132812) pythongh-132426: Add get_annotate_from_class_namespace replacing get_annotate_function (python#132490) pythongh-81793: Always call linkat() from os.link(), if available (pythonGH-132517) pythongh-122559: Synchronize C and Python implementation of the io module about pickling (pythonGH-122628) pythongh-69605: Add PyREPL import autocomplete feature to 'What's New' (python#133358) bpo-44172: Keep reference to original window in curses subwindow objects (pythonGH-26226) pythonGH-133231: Changes to executor management to support proposed `sys._jit` module (pythonGH-133287) pythongh-133363: Fix Cmd completion for lines beginning with `! ` (python#133364) pythongh-132983: Introduce `_zstd` bindings module (pythonGH-133027) pythonGH-91048: Add utils for printing the call stack for asyncio tasks (python#133284) ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emmatyping, @gpshead, @hugovk Some showstopper problems with trying to build _zstd
.
@@ -5415,6 +5415,40 @@ PKG_CHECK_MODULES([LIBLZMA], [liblzma], [have_liblzma=yes], [ | |||
]) | |||
]) | |||
|
|||
have_libzstd=no | |||
AC_DEFUN([TEST_ZSTD_VERSION],[ | |||
AC_MSG_CHECKING([if libzstd is new enough]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seem to be two problems with the new enough
check:
- There should be
AC_MSG_RESULT([yes])
andAC_MSG_RESULT([no])
calls to complete the messages:
checking for libzstd... yes
checking if libzstd is new enough... checking for hstrerror... yes
have_libzstd=yes | ||
]) | ||
|
||
PKG_CHECK_MODULES([LIBZSTD], [libzstd], [TEST_ZSTD_VERSION()], [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and 2. There's something funky going on here with the two calls to TEST_ZSTD_VERSION()
such that version number test fails and the failure is sort of hidden in config.log
:
configure:22400: checking for libzstd
configure:22407: $PKG_CONFIG --exists --print-errors "libzstd"
configure:22410: $? = 0
configure:22424: $PKG_CONFIG --exists --print-errors "libzstd"
configure:22427: $? = 0
configure:22727: result: yes
configure:22730: checking if libzstd is new enough
configure:22756: gcc -o conftest conftest.c -ldl >&5
conftest.c:325:10: fatal error: 'zstd.h' file not found
325 | #include "zstd.h"
| ^~~~~~~~
1 error generated.
configure:22756: $? = 1
configure: program exited with status 1
This later results in:
checking for stdlib extension module _zstd... disabled
I suspect the problem is the call to TEST_ZSTD_VERSION()
on line 5437 fails because at that point CPPFLAGS
is no longer set to include $LIBZSTD_CFLAGS
and the test compile would succeed only if the header files are installed in a default system location, like /usr/include
, which won't be the case, for example, on builds on macOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Postscript: a likely confirmation of my suspicion above. For our CI runs on macOS, we use Homebrew to supply third-party libs (like libzstd
) not supplied by Apple. If you look at CI logs for current PRs (i.e. after this PR was merged), _zstd
is disabled (as above) on the ghcr.io/cirruslabs/macos-runner:sonoma
runner but succeeds on the macos-13
runner. The difference is that the former is Apple Silicon (arm64) hardware where Homebrew installs to a non-standard /opt/homebrew
prefix, whereas the macos-13
runner is Intel hardware where Homebrew continues to install to the legacy /usr/local
prefix which, no doubt, is included in the compiler's default search path.
To follow up on autoconf issues Ned raised, those were addressed in #133479 |
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:
_zstd
module inModules/_zstd
. Tests will be included when the Pythoncompression.zstd
module is added. If people think the tests should be included, I can merge thecompression.zstd
changes 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:
_zstd
gets installed📚 Documentation preview 📚: https://cpython-previews--133027.org.readthedocs.build/