Make cloudflare-zlib dependency optional #369
Conversation
|
Why opt-in if it's only broken on one OS with one config? |
|
Because AFAIK there is no nice way to tell Cargo to enable that feature by default for the aarch64 and x64 architectures except for the MinGW Windows toolchain. Moreover, in case the cloudflare-zlib-sys crate gets updated to support more platforms, end users will inmediately be able to use that improvement once OxiPNG updates its dependencies, with minimal maintenance needed by OxiPNG for that. And, overall, I think that being able to compile this crate on a not-so-exotic platform outweighs it being a little bit slower only if the end user doesn't bother enabling the feature again for supported platforms. |
|
Good news: a nice way to implement platform-specific features got stabilized recently, and is underway to the next stable Rust version. See https://doc.rust-lang.org/nightly/cargo/reference/features.html#feature-resolver-version-2 and rust-lang/cargo#8997. This means that it should be possible to improve this PR soon in a way that it automatically excludes that feature for problematic platforms, but is this actually in scope for OxiPNG? To be honest, it seems like I'm really the only person that cares about the mess that is combining OxiPNG with other random C libraries that do not play nicely with the MSVC toolchain on Windows. |
|
As it is, I don't like that this PR disables cloudflare-zlib by default. However, now that Rust 1.50 is out, I'd like to see what we can achieve with that new approach. Or is that feature not out until 1.51? |
|
It'll land on 1.51, as seen here. Let's wait until 1.51 is released for this PR to continue. |
Although the Cloudflare fork of zlib may introduce performance improvements, there are issues building it with MinGW toolchains on Windows, as evidenced by this StackOverflow question: https://stackoverflow.com/questions/61170244/gcc-exe-fail-when-building-cloudflare-zlib-sys-rs However, it is not always possible to switch to the MSVC toolchain on Windows, because that may break other native C libraries which have trouble with MSVC instead, and just compiling to a 32-bit target can get pretty messy too. I've opened an issue to get this error fixed upstream, in the cloudflare-zlib-sys crate, but it was not fixed there yet. Therefore, make the Cloudflare zlib dependency optional, so users will have to explicitly opt in for it. As a side bonus, these changes also remove the restriction of Cloudflare zlib only being available on x64 and Aarch64 platforms.
|
I've been looking into this lately and, instead of working around cloudflare-zlib portability problems by adding features to the crate or bumping MSRV and using platform-specific hacks, maybe it's a better idea to drop it altogether in favor of libz-sys with zlib-ng. zlib-ng is yet another fork of zlib that integrates both Cloudflare and Intel patches to zlib. Performance wise, it trades blows with the Cloudflare fork, and even wins depending on the benchmark, but it has the advantage that is more portable. I've recently contributed zlib-ng support for the spng crate and it provided a 1,85x speedup for image decoding compared to stock zlib, and it's allegedly faster than miniz_oxide anyway. Would investigating the replacement of cloudflare-zlib with zlib-ng be a good thing for OxiPNG? I think it looks promising. |
|
Days ago, version 0.3.0 of |
Although the Cloudflare fork of zlib may introduce performance improvements, there are issues building it with MinGW toolchains on Windows, as evidenced by this StackOverflow question.
However, it is not always possible to switch to the MSVC toolchain on Windows, because that may break other native C libraries which have trouble with MSVC instead, and just compiling to a 32-bit target can get pretty messy too.
I've opened an issue to get this error fixed upstream, in the cloudflare-zlib-sys crate, but several months have passed with no answer.
Therefore, make the Cloudflare zlib dependency optional, so users will have to explicitly opt in for it.