GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-132983: Support finding libzstd without pkg-config by emmatyping · Pull Request #133550 · python/cpython

Conversation

@emmatyping

@emmatyping emmatyping commented May 7, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

GH-133479 removed the logic to check for libzstd outside of pkg-config. This commit adds that logic back with a check for the version so that users can provide their own libzstd. This is to bring parity with lzma, bz2, and zlib detection.

The issues previously seen on RHEL 8 were due to how the checks are ordered. After #133479, if a library version was too old, the PKG_CHECK_MODULES macro would take the "not found" path. This would then search for and find zstd.h and a libzstd with the symbol ZDICT_finalizeDictionary, but then compilation would fail when the header check occured. #133502 removed this path solving the issue on RHEL 8. However, this also make it impossible for a user to set a custom build of libzstd as the dependency (as they can do for liblzma, libbz2, etc.).

In this PR we add this path back, but add an additional check for the version of the libraries. If a user/third party packager would like to point to their own libzstd, they can do so. We avoid the issue #133479 was meaning to solve by ensuring that the version check only runs if we already have the zstd.h header and libzstd library found.

  • Issue: Implement PEP 784 - Adding Zstandard to the Python standard library #132983

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@emmatyping
Support finding libzstd not in pkg-config
f3dd7f7 
pythonGH-133479 removed the logic to check for libzstd outside of pkg-config.
This commit adds that logic back with a check for the version so that
users can provide their own libzstd. This is to bring parity with lzma,
bz2, and zlib detection.
@emmatyping
emmatyping requested review from AA-Turner and gpshead May 7, 2025 03:07
@emmatyping
emmatyping requested review from corona10 and erlend-aasland as code owners May 7, 2025 03:07
@emmatyping emmatyping added the skip news label May 7, 2025
@bedevere-app bedevere-app Bot added the awaiting review label May 7, 2025
@emmatyping emmatyping changed the title Support finding libzstd not in pkg-config GH-132983: Support finding libzstd not in pkg-config May 7, 2025
@bedevere-app bedevere-app Bot mentioned this pull request May 7, 2025
Implement PEP 784 - Adding Zstandard to the Python standard library #132983
Closed
13 tasks done
@emmatyping emmatyping changed the title GH-132983: Support finding libzstd not in pkg-config GH-132983: Support finding libzstd without pkg-config May 7, 2025
@emmatyping

emmatyping commented May 7, 2025

Copy link
Copy Markdown
Member Author

!buildbot RHEL8
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented May 7, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @emmatyping for commit f3dd7f7 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133550%2Fmerge

The command will test the builders whose names match following regular expression: RHEL8

The builders matched are:

  • PPC64LE RHEL8 LTO PR
  • PPC64LE RHEL8 Refleaks PR
  • AMD64 RHEL8 LTO + PGO PR
  • aarch64 RHEL8 PR
  • aarch64 RHEL8 Refleaks PR
  • AMD64 RHEL8 FIPS Only Blake2 Builtin Hash PR
  • AMD64 RHEL8 PR
  • aarch64 RHEL8 LTO + PGO PR
  • aarch64 RHEL8 LTO PR
  • s390x RHEL8 Refleaks PR
  • PPC64LE RHEL8 LTO + PGO PR
  • s390x RHEL8 PR
  • s390x RHEL8 LTO PR
  • s390x RHEL8 LTO + PGO PR
  • AMD64 RHEL8 FIPS No Builtin Hashes PR
  • AMD64 RHEL8 Refleaks PR
  • AMD64 RHEL8 LTO PR
  • PPC64LE RHEL8 PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@emmatyping

emmatyping commented May 7, 2025

Copy link
Copy Markdown
Member Author

Scheduled RHEL 8 but I also want to double check the ghcr.io/cirruslabs/macos-runner:sonoma job.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@emmatyping

emmatyping commented May 7, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Okay this is looking good.
RHEL 8:

...
checking for libzstd >= 1.4.5... no
checking for zstd.h... no
checking for zdict.h... no
...
checking for stdlib extension module _zstd... missing
...

macOS Sonoma:

...
checking for libzstd >= 1.4.5... yes
...
checking for stdlib extension module _zstd... yes
...

The AMD64 RHEL8 FIPS No Builtin Hashes PR buildbot failure looks unrelated.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AA-Turner AA-Turner mentioned this pull request May 7, 2025
GH-132983: Restore libzstd fallback detection #133565
Merged
@AA-Turner

AA-Turner commented May 7, 2025

Copy link
Copy Markdown
Member

I've opened #133565 as an alternative with slightly different (and I think also faster) autoconf syntax, what do you think?

A
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@emmatyping @AA-Turner
Use AC_COMPILE_IFELSE and checking message when looking for zstd
235fe69 
Co-authored-by: Adam Turner <[email protected]>
@emmatyping

emmatyping commented May 7, 2025

Copy link
Copy Markdown
Member Author

!buildbot RHEL8
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented May 7, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @emmatyping for commit 235fe69 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133550%2Fmerge

The command will test the builders whose names match following regular expression: RHEL8

The builders matched are:

  • PPC64LE RHEL8 LTO PR
  • PPC64LE RHEL8 Refleaks PR
  • AMD64 RHEL8 LTO + PGO PR
  • aarch64 RHEL8 PR
  • aarch64 RHEL8 Refleaks PR
  • AMD64 RHEL8 FIPS Only Blake2 Builtin Hash PR
  • AMD64 RHEL8 PR
  • aarch64 RHEL8 LTO + PGO PR
  • aarch64 RHEL8 LTO PR
  • s390x RHEL8 Refleaks PR
  • PPC64LE RHEL8 LTO + PGO PR
  • s390x RHEL8 PR
  • s390x RHEL8 LTO PR
  • s390x RHEL8 LTO + PGO PR
  • AMD64 RHEL8 FIPS No Builtin Hashes PR
  • AMD64 RHEL8 Refleaks PR
  • AMD64 RHEL8 LTO PR
  • PPC64LE RHEL8 PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@emmatyping
Merge branch 'main' into 3.14-zstd-configure-improvements
e26de5e
erlend-aasland
erlend-aasland approved these changes May 7, 2025
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.

Thanks, this seems correct to me! It would be unfortunate if we had a different set of configure rules for libzstd than for other third-party dependencies.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels May 7, 2025
@emmatyping
Merge branch 'main' into 3.14-zstd-configure-improvements
d4e6ccb
@emmatyping

emmatyping commented May 8, 2025

Copy link
Copy Markdown
Member Author

Closing in favor of #133565
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@emmatyping emmatyping closed this May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland approved these changes
@gpshead gpshead Awaiting requested review from gpshead
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner
@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting merge skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@emmatyping @bedevere-bot @AA-Turner @erlend-aasland
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.