Skip to content

Conversation

@folkertdev
Copy link
Contributor

Uses unreleased zlib-rs code, so we'll have to wait with that.

But before I make a release: does this look right? Are there any other missing things?

@folkertdev
Copy link
Contributor Author

So, should zlib-rs enable any_zlib? I don't think it should, although the feature names get really confusing that way.

Copy link

Copilot AI left a 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 support for zlib-rs features, including dictionary support and dynamic compression level adjustment. The changes integrate unreleased zlib-rs functionality and improve feature flag documentation.

Key Changes

  • Added set_dictionary() methods for both compression and decompression with zlib-rs backend
  • Added set_level() method support for zlib-rs backend with proper feature gating
  • Enhanced feature flag documentation in Cargo.toml with comprehensive descriptions of each backend option
  • Improved total_in/total_out byte tracking to exclude dictionary bytes in zlib-rs backend

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/mem.rs Added zlib-rs-specific set_dictionary and refactored set_level with feature-gated implementations for both zlib-rs and C backends
src/lib.rs Updated backend documentation to mention feature flags and added feature selection priority order documentation
src/ffi/zlib_rs.rs Implemented custom total_in/total_out tracking to exclude dictionary bytes, added dictionary and level setting methods with proper error handling
Cargo.toml Switched to unreleased zlib-rs git dependency, added document-features dependency, and comprehensively documented all feature flags with usage guidance

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Byron
Copy link
Member

Byron commented Dec 10, 2025

Thanks so much @folkertdev for getting this going!

I can't really tell if it's good like this, but can say 'it looks good'. Thus, I think we'd want some more validation.
First of all, could you rebase onto #520 (which I am trying to get merged as well), and squash your commits into one so it's easier to see what you changed?

My idea here would be to adjust the zlib-rs-capabilities test to extend to the newly added functions. That way we'd know that they are actually available with whatever feature configuration we chose. From there, we can probably find a way to adjust the documentation of the internal feature toggles, or change them entirely as they are non-public.

I hope that makes sense, if not, let's make adjustments until it does :).

@Byron Byron force-pushed the copilot/add-tests-for-zlib-rs-functionality branch from d8a67e7 to 816bd26 Compare December 10, 2025 04:13
@folkertdev folkertdev force-pushed the zlib-rs-round-2 branch 4 times, most recently from a637889 to fc06575 Compare December 10, 2025 11:46
@folkertdev
Copy link
Contributor Author

I've made the zlib-rs release now, so from that perspective this is ready. The cfg rules are a bit unfortunate to satisfy cargo clippy --all-features, but I don't see a good alternative there.

At this point, what does that separate zlib-rs test file add?

@Byron
Copy link
Member

Byron commented Dec 11, 2025

That's great, thank you!

Do you want to want to set the PR to be non-draft, or is something missing (or are there open questions?).

Regarding the zlib-rs test, I think what it should really test is what's implied by these meta-features any_zlib/any_impl. And ideally, it would do that for all backends, so we don't lose functionality next time we refactor them. It's fine to handle this in the respective PR though, please ignore my request to rebase.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@folkertdev folkertdev marked this pull request as ready for review December 11, 2025 10:44
@folkertdev
Copy link
Contributor Author

No I'm happy to go with this if it looks good.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks!

I looked through it carefully and everything seems OK. When merged, I will continue trying to figure out the tests for these features on integration level via #520.

@Byron Byron merged commit acc6d2c into rust-lang:copilot/add-tests-for-zlib-rs-functionality Dec 12, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants