search

rust-lang / flate2-rs

DEFLATE, gzip, and zlib bindings for Rust
https://docs.rs/flate2
Apache License 2.0
900 stars 162 forks source link

remove libc from default features #290

Closed Craig-Macomber closed 2 years ago

Craig-Macomber commented 2 years ago

This also required adding an alias for the miniz-sys feature which includes libc, and this is a breaking change for users of the miniz-sys feature: they need to use miniz instead or add the libc feature.

I am unaware of a way to make this a non-breaking change, and still remove libc from the default.

Another option would be to include libc in the default feature, but not in an explicit rust backend. That would avoid breaking users of miniz-sys that include the default feature, but it would still break users who use it without the default.

This addresses issue #274

Craig-Macomber commented 2 years ago

General disclaimer: I'm very new to optional features, so I may have missed something. If you do want to accept this change, despite its breaking nature, don't assume I did it correctly. I think I ran the tests correctly with the default rust back-end and miniz, but thats all I tried. Also I'm not really sure what the proper approach for testing combinations of features is, or what is acceptable breaking change wise (ex: does this change need to be delayed until we want to do do a new major version?)

Craig-Macomber commented 2 years ago

Looking a little closer, miniz-sys is published within this repo, so renaming it might not be too hard. That would allow using the feature name miniz-sys like we used to to avoid the change being breaking. If this approach would be preferred, recommend a name and I'll do the change.

alexcrichton commented 2 years ago

I believe cargo's very recently stabilized namespaced features feature will enable solving this, but it would probably be best to wait for that to avoid any breakage

Craig-Macomber commented 2 years ago

I believe cargo's very recently stabilized namespaced features feature will enable solving this, but it would probably be best to wait for that to avoid any breakage

I think you are referring to: https://doc.rust-lang.org/cargo/reference/unstable.html#namespaced-features Which looks like it was stabilized in https://github.com/rust-lang/cargo/pull/10269

I'll update this PR to use it, then you can merge it whenever you think its been long enough that its ok to depend on that cargo feature.

Looks like this will be waiting for us to be ok to require Cargo 1.60, which isn't even scheduled for release until 2022-04-07 (https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-160-2022-04-07) so it will be a while.

When the ecosystem is ready to use that, it will be a great time to clean up lots of package's dependencies :)

Craig-Macomber commented 2 years ago

I created https://github.com/rust-lang/flate2-rs/pull/291 with the new approach (it requires much less changes than on this branch, so I made it separately), which I left as a draft since it should not be merged for several months waiting for a new cargo release at least.