Skip to content

Use zstd-1.5.7 #255

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
Closed
bmwiedemann wants to merge 4 commits into from
Closed

Use zstd-1.5.7 #255

bmwiedemann wants to merge 4 commits into from

Conversation

@bmwiedemann
Copy link
Contributor

@bmwiedemann bmwiedemann commented Feb 21, 2025

this changed compression results, so we have to adapt tests

@bmwiedemann
Use zstd-1.5.7
b4a5869 
this changed compression results, so we have to adapt tests
@mgorny
Copy link
Contributor

mgorny commented Feb 24, 2025

Didn't you mean to update vendored zstd as well?

That said, CFFI needs some updates as well:

building 'zstandard._cffi' extension
x86_64-pc-linux-gnu-gcc -fno-strict-overflow -Wsign-compare -fPIC -I/tmp/python-zstandard/zstd -I/tmp/python-zstandard/.venv/include -I/usr/include/python3.13 -c build/zstandard/_cffi.c -o build/temp.linux-x86_64-cpython-313/build/zstandard/_cffi.o
build/zstandard/_cffi.c: In function ‘_cffi_const_ZSTD_frameHeader’:
build/zstandard/_cffi.c:12692:30: error: expected expression before ‘<=’ token
12692 |   int n = (ZSTD_frameHeader) <= 0;
      |                              ^~
build/zstandard/_cffi.c:12693:48: error: expected expression before ‘|’ token
12693 |   *o = (unsigned long long)((ZSTD_frameHeader) | 0);  /* check that ZSTD_frameHeader is an integer */
      |                                                ^
build/zstandard/_cffi.c: In function ‘_cffi_const_ZSTD_frameType_e’:
build/zstandard/_cffi.c:12699:30: error: expected expression before ‘<=’ token
12699 |   int n = (ZSTD_frameType_e) <= 0;
      |                              ^~
build/zstandard/_cffi.c:12700:48: error: expected expression before ‘|’ token
12700 |   *o = (unsigned long long)((ZSTD_frameType_e) | 0);  /* check that ZSTD_frameType_e is an integer */
      |                                                ^
build/zstandard/_cffi.c: In function ‘_cffi_const_ZSTD_paramSwitch_e’:
build/zstandard/_cffi.c:12706:32: error: expected expression before ‘<=’ token
12706 |   int n = (ZSTD_paramSwitch_e) <= 0;
      |                                ^~
build/zstandard/_cffi.c:12707:50: error: expected expression before ‘|’ token
12707 |   *o = (unsigned long long)((ZSTD_paramSwitch_e) | 0);  /* check that ZSTD_paramSwitch_e is an integer */
      |                                                  ^
build/zstandard/_cffi.c: In function ‘_cffi_const_ZSTD_sequenceFormat_e’:
build/zstandard/_cffi.c:12713:35: error: expected expression before ‘<=’ token
12713 |   int n = (ZSTD_sequenceFormat_e) <= 0;
      |                                   ^~
build/zstandard/_cffi.c:12714:53: error: expected expression before ‘|’ token
12714 |   *o = (unsigned long long)((ZSTD_sequenceFormat_e) | 0);  /* check that ZSTD_sequenceFormat_e is an integer */
      |                                                     ^
error: command '/usr/bin/x86_64-pc-linux-gnu-gcc' failed with exit code 1

FWICS upstream has renamed types, e.g. from ZSTD_frameHeader to ZSTD_FrameHeader, and then added backwards compatibiltity macros — and CFFI seems to be misinterpreting these macros as constants.

@bmwiedemann
Copy link
Contributor Author

bmwiedemann commented Feb 24, 2025

For some reason I did not see these issues with our (openSUSE Tumbleweed's) cffi-1.17.1 - how do you get these errors?

@mgorny
Copy link
Contributor

mgorny commented Feb 24, 2025

For some reason I did not see these issues with our (openSUSE Tumbleweed's) cffi-1.17.1 - how do you get these errors?

I actually did upgrade vendored zstd in zstd directory.

@mgorny
Copy link
Contributor

mgorny commented Feb 24, 2025

Anyway, I'll work on it, since it's blocking us for a while now.

@mgorny
Copy link
Contributor

mgorny commented Feb 24, 2025

@bmwiedemann, I've just filed bmwiedemann#1 against your branch that fixes the CFFI backend and updates the vendored library. Could you merge it, so we have a single PR with all the necessary changes?

@hmaarrfk hmaarrfk mentioned this pull request Mar 2, 2025
Closed
4 tasks
@h-vetinari h-vetinari mentioned this pull request Mar 26, 2025
Closed
1 task
indygreg
indygreg approved these changes Aug 16, 2025
View reviewed changes
Copy link
Owner

@indygreg indygreg left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution!

I've cherry picked the commits to my local repo and will be pushing them up, possibly with some light modifications. The PR will be marked as closed when I do but it will logically be merged.

I'll retain your author attribution on the commits to reflect the work you performed.

Thanks again.

@indygreg indygreg closed this in 41f6dfa Aug 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@indygreg indygreg indygreg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bmwiedemann @mgorny @indygreg