Skip to content

GH-132983: Build _zstd on Windows #133366

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 5, 2025
Merged

GH-132983: Build _zstd on Windows #133366

merged 7 commits into from
May 5, 2025
+454 −9

Conversation

AA-Turner
Copy link
Member

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

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-app bedevere-app bot mentioned this pull request May 4, 2025
Open
11 tasks
@emmatyping
Copy link
Member

emmatyping commented May 4, 2025 •
edited
Loading

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
Copy link
Member

@emmatyping emmatyping left a comment

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
@AA-Turner AA-Turner mentioned this pull request May 4, 2025
Merged
@AA-Turner AA-Turner force-pushed the zstandard-pc branch 3 times, most recently from 955650f to df19f59 Compare May 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
Copy link
Member Author

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
Copy link
Member

@zooba zooba left a comment

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

@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
Copy link
Contributor

@sethmlarson sethmlarson left a comment

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
AA-Turner added 3 commits May 5, 2025 23:30
@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-Turner AA-Turner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 5, 2025
@bedevere-bot
Copy link

🤖 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 bedevere-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
Copy link
Member

@zooba zooba left a comment

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
Copy link
Member Author

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:main May 5, 2025
67 of 75 checks passed
@AA-Turner AA-Turner deleted the zstandard-pc branch May 5, 2025 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmatyping emmatyping emmatyping approved these changes

@zooba zooba zooba approved these changes

@sethmlarson sethmlarson sethmlarson approved these changes

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

@corona10 corona10 Awaiting requested review from corona10 corona10 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