Skip to content
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

Compilation error with flate2-ffi #442

Open
rob-p opened this issue Jan 10, 2025 · 9 comments
Open

Compilation error with flate2-ffi #442

rob-p opened this issue Jan 10, 2025 · 9 comments

Comments

@rob-p
Copy link

rob-p commented Jan 10, 2025

Hi,

I’m using flate2 in a project (several actually) and I recently started getting this error when compiling:

error[E0308]: mismatched types
  --> /home/rob/.cargo/registry/src/index.crates.io-6f17d22bba15001f/flate2-1.0.35/src/ffi/c.rs:65:25
   |
65 |                 zalloc: Some(allocator::zalloc),
   |                         ^^^^^^^^^^^^^^^^^^^^^^^ expected fn pointer, found `Option<extern "C" fn(..., ..., ...) -> ... {zalloc}>`
   |
   = note: expected fn pointer `unsafe extern "C" fn(*mut c_void, u32, u32) -> *mut c_void`
                    found enum `Option<extern "C" fn(*mut c_void, u32, u32) -> *mut c_void {zalloc}>`

error[E0308]: mismatched types
  --> /home/rob/.cargo/registry/src/index.crates.io-6f17d22bba15001f/flate2-1.0.35/src/ffi/c.rs:67:24
   |
67 |                 zfree: Some(allocator::zfree),
   |                        ^^^^^^^^^^^^^^^^^^^^^^ expected fn pointer, found `Option<extern "C" fn(..., ...) {zfree}>`
   |
   = note: expected fn pointer `unsafe extern "C" fn(*mut c_void, *mut c_void)`
                    found enum `Option<extern "C" fn(*mut c_void, *mut c_void) {zfree}>`

It looks like a change to the FFI that somehow isn’t being properly reflected during compilation. Any idea what may be going on?

@rob-p
Copy link
Author

rob-p commented Jan 10, 2025

Digging further it seems that this is specifically being triggered by the cloudflare-zlib backend. Alone, that package seems to compile fine, but as part of flate2 it breaks. It seems the sys package for cloudflare zlib was updated just 2 days ago, so, perhaps, that introduced a breaking change?

@Byron
Copy link
Member

Byron commented Jan 11, 2025

Thanks for reporting!

This is interesting, and assuming that the latest version of libz-sys (v1.1.21) is used here, the recent update could be related.

I also wonder if #441 would automatically fix this issue.

What's puzzling me is that the change seems to be breaking, an Option to a function pointer is provided, but no option is expected in the FFI - I have no idea how that can happen.

My suggestion is to open a PR with an empty commit even to see if CI runs into this issue as well. If so, that would facilitate a fix.

@oyvindln
Copy link
Contributor

The rust ffi in cloudflare-zlib hasn't changed in 2 years so either the upload changed something that's not in the git repo or maybe some feature combination that may or may not be fixed by #441 is making it compile against the wrong libz

@Byron
Copy link
Member

Byron commented Jan 11, 2025

Would you be able to submit a PR with empty commit here to trigger CI and see if it reproduces? If so, from there a fix might come more easily. Thanks for your consideration.

@oyvindln
Copy link
Contributor

Added one
#443

@rob-p
Copy link
Author

rob-p commented Jan 13, 2025

Hi @Byron & @oyvindln --- thanks for following up on this! Indeed it's interesting that the CI fails in a different way.

Regarding the issue I reported here. It turns out after some digging there is an issue including this with another crate. I created a minimal example that triggers the issue initially reported. Here is the required Cargo.toml:

[package]
name = "test_flate2"
version = "0.1.0"
edition = "2021"

[dependencies]
anndata = "0.6.1"
flate2 = { version = "1.0.35", features = ["cloudflare_zlib"] }

This is sufficient to trigger :

   Compiling flate2 v1.0.35
error[E0308]: mismatched types
  --> /home/rob/.cargo/registry/src/index.crates.io-6f17d22bba15001f/flate2-1.0.35/src/ffi/c.rs:65:25
   |
65 |                 zalloc: Some(allocator::zalloc),
   |                         ^^^^^^^^^^^^^^^^^^^^^^^ expected fn pointer, found `Option<extern "C" fn(..., ..., ...) -> ... {z
alloc}>`
   |
   = note: expected fn pointer `unsafe extern "C" fn(*mut c_void, u32, u32) -> *mut c_void`
                    found enum `Option<extern "C" fn(*mut c_void, u32, u32) -> *mut c_void {zalloc}>`

error[E0308]: mismatched types
  --> /home/rob/.cargo/registry/src/index.crates.io-6f17d22bba15001f/flate2-1.0.35/src/ffi/c.rs:67:24
   |
67 |                 zfree: Some(allocator::zfree),
   |                        ^^^^^^^^^^^^^^^^^^^^^^ expected fn pointer, found `Option<extern "C" fn(..., ...) {zfree}>`
   |
   = note: expected fn pointer `unsafe extern "C" fn(*mut c_void, *mut c_void)`
                    found enum `Option<extern "C" fn(*mut c_void, *mut c_void) {zfree}>`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `flate2` (lib) due to 2 previous errors

However, it's not clear to me at all why. I'm opening an issue in the anndata repo and will mention this issue there as well.

@Byron
Copy link
Member

Byron commented Jan 13, 2025

You are welcome!

I took a look at the anndata manifest and it seems it uses flate2 without any feature specification, and thus with default features. This means it compiles with the features rust_backend and cloudflare_zlib. When trying this locally here in flate2, everything still works though, at least on MacOS.

diff --git a/Cargo.toml b/Cargo.toml
index 6d64821..96170b7 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -35,7 +35,7 @@ rand = "0.8"
 quickcheck = { version = "1.0", default-features = false }
 
 [features]
-default = ["rust_backend"]
+default = ["rust_backend", "cloudflare_zlib"]
 any_zlib = ["any_impl"] # note: this is not a real user-facing feature
 any_impl = [] # note: this is not a real user-facing feature
 zlib = ["any_zlib", "libz-sys"]

I think what's really happening here is that flate2 thinks that cloudflare_zlib is used, when in fact zlib-ng or another non-cloudflare implementation is used.

This means the mz_stream struct is the wrong one. It's probably the logic here:

flate2-rs/src/ffi/c.rs

Lines 423 to 439 in 8d1467d

#[cfg(feature = "zlib-ng")]
use libz_ng_sys as libz;
#[cfg(all(feature = "zlib-rs", not(feature = "zlib-ng")))]
use libz_rs_sys as libz;
#[cfg(
// cloudflare-zlib
all(feature = "cloudflare_zlib", not(feature = "zlib-rs"), not(feature = "zlib-ng")),
)]
use cloudflare_zlib_sys as libz;
#[cfg(
// libz-sys
all(not(feature = "cloudflare_zlib"), not(feature = "zlib-ng"), not(feature = "zlib-rs")),
)]
use libz_sys as libz;

The provided test-manifest fails on MacOS too, and cargo tree -i flate2 -e features reveals the following features are active:

├── flate2 feature "cloudflare-zlib-sys"
│   └── flate2 feature "cloudflare_zlib" (*)
├── flate2 feature "cloudflare_zlib" (*)
├── flate2 feature "default" (*)
├── flate2 feature "libz-ng-sys"
│   └── flate2 feature "zlib-ng" (*)
├── flate2 feature "miniz_oxide"
│   └── flate2 feature "rust_backend" (*)
├── flate2 feature "rust_backend" (*)
└── flate2 feature "zlib-ng" (*)

This basically means the zlib-ng snuck in there, and it seems to be set by polars-io.

To fix this, one would have to figure out the correct logic for both the mz_stream initialisation and its import to match up in the same way, right now they are out of sync. The mz_stream is the one from zlib-ng but the initialization is the one for cloudflare.

Since cloudflare_zlib isn't really kicking in, it's probbaly easiest to remove it in the application manifest so zlib-ng can come through properly.

Ideally, flate2 would be able to select only valid combinations according to its own 'feature-ladder'.

@oyvindln
Copy link
Contributor

does using the git version of flate2 with #441 instead of release change this? I thought that was supposed to fix just this

@Byron
Copy link
Member

Byron commented Jan 14, 2025

🤦‍♂️The #441 PR in my memory was in libz-sys so I kept thinking it's related to the recent release. But this was completely wrong, and you are right that when using the latest flate2 release, the issue does not reproduce anymore.

Now we just have the broken roundtrip test left to fix, and unfortunately I have no idea what it could be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants