-
Notifications
You must be signed in to change notification settings - Fork 185
test zlib API for all compatible backends #525
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
Conversation
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.
Pull request overview
This PR adds comprehensive capability tests for zlib-specific API functions across all compatible backends (any_zlib and zlib-rs), excluding miniz_oxide which doesn't support these features. The PR builds on #524 which added set_dictionary and set_level support to the zlib-rs backend.
Key Changes:
- New test file tests zlib-specific APIs (window_bits configuration, dictionary support, compression level changes) for both any_zlib and zlib-rs backends
- Extended feature gates from
any_zlibonly toany(feature = "any_zlib", feature = "zlib-rs")for several functions - Implemented set_dictionary and set_level methods for the zlib-rs backend with proper error handling
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/capabilities.rs | New comprehensive test suite for zlib-specific functionality (window_bits, gzip, dictionaries, compression levels) |
| src/mem.rs | Updated feature gates to include zlib-rs, added set_dictionary and set_level method implementations for zlib-rs, removed redundant tests moved to capabilities.rs |
| src/ffi/zlib_rs.rs | Implemented set_dictionary and set_level for zlib-rs backend, added custom total_in/total_out tracking to exclude dictionary bytes |
| Cargo.toml | Bumped zlib-rs dependency from 0.5.3 to 0.5.5 for new API support |
| .github/workflows/main.yml | Enabled zlib-rs tests on mingw platform |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
After all, now it doesn't rely on a C compiler anymore.
…lib-rs` This spells out what was implied before. And it's something that has to be kept in sync with `zlib-rs`, until maybe one day it can return something completely unambiguous.
Some of these tests were previously gated and placed in such a way that `zlib-rs` was able to switch from its C-bindings to using its Rust API without implementing all zlib-implementation specific methods. The exstiing tests were unable to see this, and this commit adds a new capabiltiies test that will test these methods in all implementations but the `miniz_oxide`, which truly doesn't support them.
This release will finally make `zlib-rs` without C bindings public.
There has been a drift in implementation capabilities that makes the current internal feature toggles less suitable than they initially were. We fix this by introducing `any_c_zlib` to capture all zlib backends that need C bindings. Then there is `any_zlib` which is all backends that are fully zlib compatible.
|
CI is green, great! And I hope nothing more subtle broke. Please do review very carefully, maybe you can find something @joshtriplett . |
|
Wrote a few comments; with those addressed, r=me. |
|
Thanks a lot for the review @joshtriplett, I think it makes it much better. |
Because we no longer depend on it since bc2f339 (GitoxideLabs#2370). For context on the associated TODO this completes and removes, see: - rust-lang/flate2-rs#520 (superseded) - rust-lang/flate2-rs#524 - rust-lang/flate2-rs#525
Add capabilities test for functions reserved to zlib-implementations.
Some of these tests were previously gated and placed in such a way that
zlib-rswas able to switch from its C-bindings to using its Rust API without implementing all zlib-implementation specific methods.The exstiing tests were unable to see this, and this commit adds a new capabiltiies test that will test these methods in all implementations but the
miniz_oxide, which truly doesn't support them.