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

remove libc from default features #290

Closed
wants to merge 4 commits into from

Conversation

Craig-Macomber
Copy link

@Craig-Macomber Craig-Macomber commented Jan 18, 2022

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
Copy link
Author

Craig-Macomber commented Jan 18, 2022

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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 rust-lang/cargo#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
Copy link
Author

I created #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.

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