GH-132983: Build _zstd on Windows - #133366
Conversation
Sorry, something went wrong.
|
Oh this is great! I had a branch with similar changes that I hadn't gotten to making a PR, but this looks good! Thank you for proposing it.
I don't think the current code currently or will use it, so I think it is fine to exclude. |
Sorry, something went wrong.
emmatyping
left a comment
There was a problem hiding this comment.
Looks good!
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
cc @zooba -- please could I ask for a quick once-over of the build changes in this PR? I'm fairly confident in them, but would be good to have a review. A |
Sorry, something went wrong.
zooba
left a comment
There was a problem hiding this comment.
Looks fine to me, though I'm not sure we need a separate project for zstd? We should be able to reference all the files directly through _zstd (we couldn't for zlib-ng because their build process is way too complex to integrate into pythoncore, but this one looks pretty simple and already has its own extension module that's separate).
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
sethmlarson
left a comment
There was a problem hiding this comment.
SBOM changes LGTM, thank you!
Sorry, something went wrong.
MSB8027: Two or more files with the name of zdict.c will produce outputs to the same location. This can lead to an incorrect build result. The files involved are ..\Modules\_zstd\zdict.c, \externals\zstd-1.5.7\lib\dictBuilder\zdict.c.
|
🤖 New build scheduled with the buildbot fleet by @AA-Turner for commit 1ae3a76 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133366%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
Sorry, something went wrong.
zooba
left a comment
There was a problem hiding this comment.
LGTM. Merge whenever you're ready
Sorry, something went wrong.
|
Note for others: because we now build zstd as one project, we can't have name collisions, so we've renamed Modules\zdict.c to Modules\zstddict.c. |
Sorry, something went wrong.
- python/cpython#133027 - python/cpython#133366 - python/cpython#133284 - python/cpython#133398 - python/cpython#131298 - python/cpython#132438 - python/cpython#133012 --------- Co-authored-by: Wingy <git@wingysam.xyz> Co-authored-by: Geoffrey Thomas <geofft@ldpreload.com>
Per the documentation in zlib/lib, we build lib/common, lib/compress, lib/decompress, and lib/dictBuilder.
Building lib/legacy is noted as optional to support decompression only of archives created with Zstd 0.8 (2016) or earlier. That release is 10 years old, so for simplicity I chose not to build it, but we could add it in if thought useful.
A