Skip to content

GH-132983: Build _zstd on Windows - #133366

Merged
AA-Turner merged 7 commits into
python:mainfrom
AA-Turner:zstandard-pc
May 5, 2025
Merged

GH-132983: Build _zstd on Windows#133366
AA-Turner merged 7 commits into
python:mainfrom
AA-Turner:zstandard-pc

Conversation

@AA-Turner

@AA-TurnerAA-Turner commented May 4, 2025•
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

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

@bedevere-appbedevere-appBot mentioned this pull request May 4, 2025
Closed
13 tasks
@emmatyping

emmatyping commented May 4, 2025•
edited
Loading

Copy link
Copy Markdown
Member

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.

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.

I don't think the current code currently or will use it, so I think it is fine to exclude.

emmatyping
emmatyping approved these changes May 4, 2025
View reviewed changes

@emmatypingemmatyping left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good!

@AA-Turner

This comment was marked as resolved.

Sign in to view
@bedevere-bot

This comment was marked as resolved.

Sign in to view
emmatyping
emmatyping reviewed May 4, 2025
View reviewed changes
Comment threadPCbuild/pcbuild.sln Outdated
@AA-TurnerAA-Turner mentioned this pull request May 4, 2025
Merged
@AA-Turner
AA-Turnerforce-pushed the zstandard-pc branch 3 times, most recently from 955650f to df19f59CompareMay 4, 2025 20:27
@AA-Turner

This comment was marked as resolved.

Sign in to view
@bedevere-bot

This comment was marked as resolved.

Sign in to view
@AA-Turner

This comment was marked as resolved.

Sign in to view
@AA-Turner

AA-Turner commented May 5, 2025

Copy link
Copy Markdown
MemberAuthor

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
zooba reviewed May 5, 2025
View reviewed changes

@zoobazooba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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).

Comment threadPCbuild/pcbuild.sln Outdated
@emmatyping

This comment was marked as resolved.

Sign in to view
@zooba

This comment was marked as resolved.

Sign in to view
sethmlarson
sethmlarson approved these changes May 5, 2025
View reviewed changes

@sethmlarsonsethmlarson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SBOM changes LGTM, thank you!

zooba
zooba reviewed May 5, 2025
View reviewed changes
Comment threadPCbuild/python.props Outdated
@AA-Turner
Combine zstd.vcxproj into _zstd.vcxproj
755919f
@AA-Turner
Rename Modules\zdict.c to zstddict.c
17149df
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
Point $(zstdDir) to the root
1ae3a76
@AA-TurnerAA-Turner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 5, 2025
@bedevere-bot

bedevere-bot commented May 5, 2025

Copy link
Copy Markdown

🤖 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-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 5, 2025
zooba
zooba approved these changes May 5, 2025
View reviewed changes

@zoobazooba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Merge whenever you're ready

@AA-Turner

AA-Turner commented May 5, 2025

Copy link
Copy Markdown
MemberAuthor

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.

@AA-Turner
AA-Turner merged commit e6f8e0a into python:mainMay 5, 2025
@AA-Turner
AA-Turner deleted the zstandard-pc branch May 5, 2025 23:58
@zaniebzanieb mentioned this pull request May 17, 2025
Merged
zanieb added a commit to astral-sh/python-build-standalone that referenced this pull request Jun 4, 2025
@zanieb@Wingysam@geofft
Update to 3.14.0b1 (#610)
d2030cb
- 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>
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@AA-Turner@Pranjal095
pythonGH-132983: Build_zstdon Windows (python#133366)
e190590
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emmatypingemmatypingemmatyping approved these changes
@zoobazoobazooba approved these changes
@sethmlarsonsethmlarsonsethmlarson approved these changes
@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aaslanderlend-aasland is a code owner
@corona10corona10Awaiting requested review from corona10corona10 is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@AA-Turner@emmatyping@bedevere-bot@zooba@sethmlarson