gh-91349: Replace zlib with zlib-ng in Windows build by zooba · Pull Request #131438 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

"externalRefs": [
{
"referenceCategory": "SECURITY",
"referenceLocator": "cpe:2.3:a:zlib:zlib:1.3.1:::::::*",
"referenceLocator": "cpe:2.3:a:zlib-ng:zlib-ng:2.2.4:::::::*",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll admit I just guessed this, in order to unblock my testing (couldn't build at all with an invalid SBOM). If it's not right, let me know

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely the CPE that would get used, although we can't be for certain until a CVE exists with it... the only one I could find with some searching is cpe:...:zlib-ng:minizip-ng which is for a different component but at least the organization is a match.

@@ -2101,6 +2101,12 @@ zlib_exec(PyObject *mod)
PyUnicode_FromString(zlibVersion())) < 0) {
return -1;
}
#ifdef ZLIBNG_VERSION
if (PyModule_Add(mod, "ZLIBNG_VERSION",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured we'd want some way to detect this other than looking at the text in ZLIB_VERSION

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. annoying to have optional attributes that one would use hasattr or getattr on instead of blindly accessing, but realistically nobody should care as this is more internal informational so this is fine.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the main place it'll be used is in pythoninfo, which already handles absent attributes.

We have more offensive optional attributes in our extension modules 😆

I've reviewed the three header files added to PC and they seem generic. It's a shame they aren't part of the sources, but figured it's easier/safer to just copy them into our source tree so that the cpython-source-deps repo is a straight import.

The two failing buildbots appear unrelated, and everything else passed. All I changed in the last commit was docs, so I'm not going to rerun the buildbots unless someone thinks it's necessary or worthwhile.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SBOM changes LGTM!

"externalRefs": [
{
"referenceCategory": "SECURITY",
"referenceLocator": "cpe:2.3:a:zlib:zlib:1.3.1:::::::*",
"referenceLocator": "cpe:2.3:a:zlib-ng:zlib-ng:2.2.4:::::::*",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kicking off a pyperformance benchmarking run with this -- we have a non-zero usage of zlib in that suite.

@@ -2101,6 +2101,12 @@ zlib_exec(PyObject *mod)
PyUnicode_FromString(zlibVersion())) < 0) {
return -1;
}
#ifdef ZLIBNG_VERSION
if (PyModule_Add(mod, "ZLIBNG_VERSION",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has broken the tailcall builds on Windows: https://github.com/python/cpython/actions/runs/13967899073/job/39102464260

D:\a\cpython\cpython\externals\\zlib-ng-2.2.4\\functable.c(79,26): error : use of undeclared identifier 'slide_hash_sse2'; did you mean 'slide_hash_c'? [D:\a\cpython\cpython\PCbuild\zlib-ng.vcxproj]
D:\a\cpython\cpython\externals\\zlib-ng-2.2.4\\functable.c(123,26): error : use of undeclared identifier 'slide_hash_avx2'; did you mean 'slide_hash_c'? [D:\a\cpython\cpython\PCbuild\zlib-ng.vcxproj]

Probably needs some fixed preprocessor checks (hopefully only in what's in the PC directory) to better handle the intrinsics being present/absent.

Though I notice that build warns NOTE: Visual Studio not detected. LLVM does not provide a C/C++ standard library and may be unable to locate MSVC headers., which might also be relevant.

Though I notice that build warns NOTE: Visual Studio not detected. LLVM does not provide a C/C++ standard library and may be unable to locate MSVC headers., which might also be relevant.

FWIW, when building with the bundled clang-cl of VS I do not have these warnings, but get the same error.

Most likely zlib-ng would generate different versions of the header files I checked into the PC directory for Clang. Figuring out what differences belong in there and detecting it dynamically rather than using CMake is the way forward.

This should get a new issue (because clang-cl isn't a supported configuration, so we don't have to revert this change until it's working). Tag me on it and we can continue there.

NotUsing
$(zlibNgDir);$(PySourceDir)PC;$(GeneratedZlibNgDir);%(AdditionalIncludeDirectories)
%(PreprocessorDefinitions);ZLIB_COMPAT;WITH_GZFILEOP;NO_FSEEKO;HAVE_BUILTIN_ASSUME_ALIGNED;_CRT_SECURE_NO_DEPRECATE;_CRT_NONSTDC_NO_DEPRECATE;
%(PreprocessorDefinitions);X86_FEATURES;X86_HAVE_XSAVE_INTRIN;X86_SSE2;X86_SSSE3;X86_SSE42;X86_PCLMULQDQ_CRC;X86_AVX2;X86_AVX512;X86_AVX512VNNI;X86_VPCLMULQDQ_CRC

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that for all executables X86_AVX512 code will be emitted and used?
See e.g. adler32_avx512.c, where the whole file is guarded with X86_AVX512 and there is code like __m512i in it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's detected and called dynamically based on availability, but it's all compiled at compile time.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I hope so, too.

%(PreprocessorDefinitions);ZLIB_COMPAT;WITH_GZFILEOP;NO_FSEEKO;HAVE_BUILTIN_ASSUME_ALIGNED;_CRT_SECURE_NO_DEPRECATE;_CRT_NONSTDC_NO_DEPRECATE;
%(PreprocessorDefinitions);X86_FEATURES;X86_HAVE_XSAVE_INTRIN;X86_SSE2;X86_SSSE3;X86_SSE42;X86_PCLMULQDQ_CRC;X86_AVX2;X86_AVX512;X86_AVX512VNNI;X86_VPCLMULQDQ_CRC
%(PreprocessorDefinitions);ZLIB_DEBUG
AdvancedVectorExtensions2

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unconditionally letting the compiler generate code up to AVX2 for all *.c files here might result in the binary not running on all CPUs?

seehwan pushed a commit to seehwan/cpython that referenced this pull request

Apr 16, 2025