Skip to content

Conversation

NobodyXu
Copy link
Contributor

@NobodyXu NobodyXu commented Sep 27, 2025

Having a safe API for MaybeUninit<u8> would be necessary for async-compression to wrap and reduce the performance penalty of initializing the buffer for all tokio::io::* traits implementation.

Also update (de)compress_vec API to use the new API instead of passing a &mut [u8] which technically violates rust's semantics.

NOTE that for miniz_oxide implementation, it still zero-initialize the output, since probably no one use it in production and now libz-rs is the go-to high-performance pure-rust implementation I don't see any point in optimizing it.

Related PR: trifectatechfoundation/bzip2-rs#147

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Sep 27, 2025

macOS msrv CI failure is due to cloudflare-zlib-sys, seems unrelated to this PR

https://github.com/rust-lang/flate2-rs/actions/runs/18062890552/job/51401644728?pr=502#step:10:283

looks like due to macos-latest is updated with new env, previously it probably does not import stdio.h but now some other (system) headers might import it

This is clearly not something that can be fixed in this repository, ignoreing it might makes more sense

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 a lot for contributing! To me it seems very valuable to clean up output memory handling to maybe make it more robust thanks to the (to me very magical) MaybeUninit.

The biggest problem I am having is the MSRV failure, as I don't know why it happens now and what should be done about it. As any cloudflare-zlib or MSRV change should have triggered this back in the day, we may probably assume that the macOS image changed.
Since cloudflare-zlib is on the way out, it might be acceptable to let it rot and disable this particular test for it. But I am not certain the easy way is the good way here.

@Byron Byron requested a review from jongiddy September 28, 2025 05:34
@NobodyXu NobodyXu requested a review from Byron September 28, 2025 06:07
@jongiddy
Copy link
Contributor

Yes, it may just be simpler to remove cloudflare-zlib as an option at this point. Pinging @kornelski for any comment.

CI on MacOS is giving the error:

In file included from /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/cloudflare-zlib-sys-0.3.5/zutil.c:10:
In file included from /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/cloudflare-zlib-sys-0.3.5/gzguts.h:20:
In file included from /Applications/Xcode_16.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/stdio.h:61:
/Applications/Xcode_16.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/_stdio.h:318:7: error: expected identifier or '('
  318 | FILE    *fdopen(int, const char *) __DARWIN_ALIAS_STARTING(__MAC_10_6, __IPHONE_2_0, __DARWIN_ALIAS(fdopen));
      |          ^
/Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/cloudflare-zlib-sys-0.3.5/zutil.h:137:33: note: expanded from macro 'fdopen'
  137 | #        define fdopen(fd,mode) NULL /* No fdopen() */

@kornelski
Copy link
Contributor

You're using cloudflare-zlib-sys 0.3.5, and the issue has been fixed in 0.3.6 released in March.

The incompatibility with new macOS headers was caused by an obsolete workaround meant for a very ancient MacOS, carried over from old zlib.h.

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 a lot for everyone's help!

I take it that this PR can be merged as is then, even though I have a few more coment.

Could you also rebase and squash it down a bit?

@NobodyXu
Copy link
Contributor Author

CI is green now 🎉

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.

Thank a lot!

Then… let's do it 🙏.

(I am always a bit afraid to merge anything here with my possibly dangerous mix of naiveté and ignorance 😅)

@Byron Byron merged commit 3cae7da into rust-lang:main Sep 29, 2025
15 checks passed
@NobodyXu NobodyXu deleted the patch-1 branch September 30, 2025 00:18
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Oct 1, 2025

do you have any plan of cutting new release?

Not in a rush though

@Byron
Copy link
Member

Byron commented Oct 2, 2025

I had no plans, actually. Maybe we can wait a little longer

2025-09-29 18:28 +0200 Sebastian Thiel            M─┐ [main] {origin/main} {origin/HEAD} Merge pull request #502 from NobodyXu/patch-1
2025-09-29 10:52 +0000 Jiahao XU                  │ o Add `(de)compress_uninit` impl for uninit buffer
2025-09-29 04:37 +0200 Sebastian Thiel            M─┤ Merge pull request #503 from jongiddy/update-cloudflare-zlib
2025-09-28 21:21 +0100 Jonathan Giddy             │ o Update cloudflare-zlib-sys crate
2025-07-20 09:20 +0200 Sebastian Thiel            M─┤ Merge pull request #497 from Shnatsel/undo-misleading-docs
2025-07-20 01:09 +0100 Sergey "Shnatsel" Davidoff │ o Undo introducing straight up incorrect documentation
2025-07-19 14:43 +0200 Sebastian Thiel            M─┤ Merge pull request #496 from fintelia/flush-kinds
2025-07-19 00:43 -0700 Jonathan Behrens           │ o Use partial flushes with miniz_oxide backend
2025-07-19 10:22 +0200 Sebastian Thiel            M─│─┐ Merge pull request #495 from fintelia/patch-1
2025-07-19 00:18 -0700 Jonathan Behrens           │ │ o Correct documentation typo
2025-07-03 07:37 +0200 Sebastian Thiel            M─┼─┘ Merge pull request #492 from Shnatsel/simd-adler32
2025-07-03 07:30 +0200 Sebastian Thiel            │ o [simd-adler32] [refs/patches/simd-adler32/thanks-clippy] thanks clippy
2025-07-02 14:08 +0100 Sergey "Shnatsel" Davidoff │ o Switch from adler2 to simd-adler32 crate when using miniz_oxide backend
2025-06-14 14:24 +0200 Sebastian Thiel            M─┤ Merge pull request #491 from folkertdev/zlib-version
2025-06-07 12:48 +0200 Folkert de Vries           │ o use `zlibVersion()` instead of a `const` for the version
2025-06-07 05:21 +0200 Sebastian Thiel            M─┤ <1.1.2> Merge pull request #490 from folkertdev/update-zlib-rs-0.5.1

There doesn't seem to be too much either.
What do you recommend me doing?

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Oct 2, 2025

Looks like it has some fixes (use zlibVersion() instead), and I think it's actually good to have too much commits accumulating?

cc cuts a new release every week as long as there're changes, not saying flate2 should be as frequent as that, but I think letting commits sit for months without release is actually bad as end users end up waiting really long for improvements in flate2.

plus, having less commits per release is good for finding bugs, users who experienced breakage after upgrading can find the culprit commit easier than having a lot.

With that says it's my two cents, and I'm not rushed to have this PR released, just my own opinion on release cadence

@Byron
Copy link
Member

Byron commented Oct 4, 2025

Thanks for sharing! Let me try to get better at making releases then :).

Oh, and I remember now, I can't approve my own PRs so releasing is a hassle unless it's planned and a version bump is made by someone else. I don't actually know who is associated with this project and hope that everyone who is suggested as reviewer also is allowed to approve PRs here.
Please see #504 .

@Byron Byron mentioned this pull request Oct 4, 2025
2 tasks
@Byron
Copy link
Member

Byron commented Oct 5, 2025

Great - here is the new release: https://github.com/rust-lang/flate2-rs/releases/tag/1.1.3 .

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Oct 5, 2025

Thank you!

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Oct 5, 2025

The docs.rs build fails brcause doc_auto_cfg has been removed in nightly

https://docs.rs/crate/flate2/1.1.3/builds/2560338

@Byron
Copy link
Member

Byron commented Oct 5, 2025

Thanks for reporting! If it was an issue I'd add the"help wanted" label just because I can't merge my own fixes 😅

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Oct 5, 2025

@Byron opened #505, though I didn't run the docs.rs locally

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.

4 participants