Skip to content

GH-132983: Remove zstd version check in the header file #133502

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.

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

Merged
merged 2 commits into from
May 6, 2025

Conversation

AA-Turner
Copy link
Member

@AA-Turner AA-Turner commented May 6, 2025

We now have better version detection in autoconf, including detecting the most recent symbol we used. A hard version check in the header file can lead to spurious failures when using zstd versions between 1.1.3 (symbol added) and 1.4.5 (symbol stabilised).

xref:

A

@bedevere-bot

This comment was marked as resolved.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 6, 2025
@AA-Turner AA-Turner added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label May 6, 2025
@bedevere-bot

This comment was marked as resolved.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label May 6, 2025
@AA-Turner

This comment was marked as resolved.

@encukou
Copy link
Member

encukou commented May 6, 2025

Argh!
The header file puts it in an #ifdef ZDICT_STATIC_LINKING_ONLY block, saying it's experimental. But, configure looks for the exported function symbol.

@AA-Turner
Copy link
Member Author

cc @encukou @vstinner I'm confused by this error as the struct has existed with the members that we need since v0.7 (2016) of libzstd. Do you have any suggestions?

A

@AA-Turner

This comment was marked as duplicate.

@AA-Turner
Copy link
Member Author

Is it worth setting the ZDICT_STATIC_LINKING_ONLY define? The other option is to remove the fallback symbol detection and have autoconf just check for >=1.4.5.

@encukou
Copy link
Member

encukou commented May 6, 2025

IMO, the >=1.4.5 check is safer.

@AA-Turner AA-Turner added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label May 6, 2025
@bedevere-bot

This comment was marked as resolved.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label May 6, 2025
@AA-Turner
Copy link
Member Author

For reference, v1.4.4 and earlier guarded the symbol behind a define. It was stabilised (facebook/zstd#2111) in v1.4.5.

@AA-Turner

This comment was marked as outdated.

@bedevere-bot

This comment was marked as outdated.

@AA-Turner
Copy link
Member Author

free-threaded tsan failure is spurious: 1 test failed: test_asyncio.test_free_threading

@encukou
Copy link
Member

encukou commented May 6, 2025

test_datetime's [24, 24, 24] leaks are also spurious: this PR doesn't include #133498. I checked:

  • buildbot/AMD64 CentOS9 NoGIL Refleaks PR
  • buildbot/ARM64 MacOS M1 Refleaks NoGIL PR
  • buildbot/PPC64LE RHEL8 Refleaks PR

@AA-Turner
Copy link
Member Author

The buildbots don't seem to have scheduled

@AA-Turner

This comment was marked as resolved.

@bedevere-bot

This comment was marked as resolved.

@hugovk
Copy link
Member

hugovk commented May 6, 2025

The buildbots don't seem to have scheduled

Same thing happened at #133497 (comment). Trying again worked.

@AA-Turner
Copy link
Member Author

I'm still not seeing e.g. 'AMD64 RHEL8 LTO PR' in the pending checks list?

@AA-Turner
Copy link
Member Author

buildbot/PPC64LE RHEL8 Refleaks PR failures are the leaks:

test_datetime leaked [24, 24, 24] memory blocks, sum=72
FAIL: test_interrupt (test.test_multiprocessing_spawn.test_processes.WithProcessesTestProcess.test_interrupt)
Re-running test.test_multiprocessing_spawn.test_processes in verbose mode (matching: test_interrupt)
Re-running test_datetime in verbose mode
test_datetime leaked [24, 24, 24] memory blocks, sum=72

@encukou
Copy link
Member

encukou commented May 6, 2025

AMD64 RHEL8 FIPS No Builtin Hashes is unstable; failed in the usual way here.

@hugovk
Copy link
Member

hugovk commented May 6, 2025

Looking good so far, let's merge. Thanks both!

@hugovk hugovk merged commit f869190 into python:main May 6, 2025
56 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants