-
-
Notifications
You must be signed in to change notification settings - Fork 236
Add zstd support for Python 3.14+ on Unix #682
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
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
8c6ba15 to
1e20c6f
Compare
0d98044 to
c0ee532
Compare
79e21bd to
a294ae1
Compare
c02be2c to
0ef4ea9
Compare
|
@Cyan4973 there is a musl workaround patch in this PR that may interest you. Our build environment is quite esoteric though (clang on ancient Debian), so please apply scrutiny. |
| pushd cpython-source-deps-zstd-${ZSTD_VERSION}/lib | ||
|
|
||
| if [ "${CC}" = "musl-clang" ]; then | ||
| # In order to build the library with SSE2, BMI, and AVX2 intrinstics, we need musl-clang to find |
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.
This is modeled off of
python-build-standalone/cpython-unix/build-cpython.sh
Lines 410 to 422 in 13731b0
| # In order to build the _blake2 extension module with SSE3+ instructions, we need | |
| # musl-clang to find headers that provide access to the intrinsics, as they are not | |
| # provided by musl. These are part of the include files that are part of clang. | |
| # But musl-clang eliminates them from the default include path. So copy them into | |
| # place. | |
| for h in /tools/${TOOLCHAIN}/lib/clang/*/include/*intrin.h /tools/${TOOLCHAIN}/lib/clang/*/include/{__wmmintrin_aes.h,__wmmintrin_pclmul.h,mm_malloc.h,cpuid.h}; do | |
| filename=$(basename "$h") | |
| if [ -e "/tools/host/include/${filename}" ]; then | |
| echo "${filename} already exists; don't need to copy!" | |
| exit 1 | |
| fi | |
| cp "$h" /tools/host/include/ | |
| done |
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.
Annoyingly, I didn't realize this was the problem until I was pretty far in. We might want to generalize this at some point, e.g., by fixing musl-clang or rolling our own.
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.
Yeah I've been wanting to do that, filed #684.
On a separate note - I assume zstd is smart enough to correctly detect availability of the intrinsics and we don't need to patch these out for x86-64-v1? I guess put another way, do we have tests that our x86-64-v1 builds don't include things that aren't supported on that microarch? If not we should add some (either with objdump or I think qemu can do it with machine options).
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.
I assume zstd is smart enough to correctly detect availability of the intrinsics and we don't need to patch these out for x86-64-v1?
Things looked properly gated from the poking around I did to get this working. I don't know of explicit test coverage.
geofft
left a comment
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.
LGTM ![]()
| pushd cpython-source-deps-zstd-${ZSTD_VERSION}/lib | ||
|
|
||
| if [ "${CC}" = "musl-clang" ]; then | ||
| # In order to build the library with SSE2, BMI, and AVX2 intrinstics, we need musl-clang to find |
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.
Yeah I've been wanting to do that, filed #684.
On a separate note - I assume zstd is smart enough to correctly detect availability of the intrinsics and we don't need to patch these out for x86-64-v1? I guess put another way, do we have tests that our x86-64-v1 builds don't include things that aren't supported on that microarch? If not we should add some (either with objdump or I think qemu can do it with machine options).
cpython-unix/build-zstd.sh
Outdated
| done | ||
| EXTRA_TARGET_CFLAGS="${EXTRA_TARGET_CFLAGS} -I${TOOLS_PATH}/host/include/" | ||
|
|
||
| # `qsort_r` is not available in musl and the zstd source provides a fallback implementation, but |
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.
It's in 1.2.3, but I guess we're on 1.2.2 for compatibility with older systems of some sort?
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.
That's funny. Yeah we're on the oldest version we can use for compatibility with old systems since we dynamically link now. We can probably upgrade at some point though.
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.
I'll update that comment.
|
I guess the trouble here is that musl (intentionally) does not expose any sort of version information in headers, and so if you want to figure out whether the target musl supports |
This task was deferred from the initial Python 3.14 support. There's already support on Windows.