GH-132983: Build _zstd on Windows#133366
Conversation
AA-Turner
requested review from
a team,
corona10,
erlend-aasland and
sethmlarson
as code owners
|
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. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
AA-Turner
force-pushed
the
zstandard-pc
branch
3 times, most recently
from
955650f to
df19f59
Compare
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.
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 |
zooba
left a comment
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).
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
sethmlarson
left a comment
sethmlarson
left a comment
There was a problem hiding this comment.
SBOM changes LGTM, thank you!
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.
AA-Turner
added
the
🔨 test-with-buildbots
|
🤖 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. |
bedevere-bot
removed
the
🔨 test-with-buildbots
zooba
left a comment
zooba
left a comment
There was a problem hiding this comment.
LGTM. Merge whenever you're ready
|
Note for others: because we now build |
- 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>
5 participants
Per the documentation in
zlib/lib, we buildlib/common,lib/compress,lib/decompress, andlib/dictBuilder.Building
lib/legacyis 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