-
Notifications
You must be signed in to change notification settings - Fork 682
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
Do not enable any features by default #2091
Conversation
It looks like in order for this to be doable a lot of cleanup is needed around putting |
I agree. I think it's been long enough that we can switch to no default features. As for your second comment, we do indeed need to turn on --all-features in CI. But that's just a few lines. As for unused import lines, trying to correctly conditionalize those on enabled features would be fiendishly difficult. So we take a shortcut. lib.rs has this line, which means we only need to worry about unused_imports when all features are enabled: #![cfg_attr(not(feature = "default"), allow(unused_imports))] |
Clever work around, but it's not quite enough anymore, as default doesn't contain features now. Thoughts on how to fix it? I'll go add all features to the testing. |
What about adding a feature named "full" which enables all of the others? |
Actually, adding a new feature probably isn't good, because it would make the documentation longer and |
I was afraid you'd say that lol. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also please mention this in the CHANGELOG ?
Two major changes: - features must be included; we only need two (this will speed up compiles!) - FD safety; we now need an OwnedFd, so attempting to use the fd after the object is dropped is no longer valid. Additionally, we don't need to implement Drop, as OwnedFd already has a drop which calls close References: * [/~https://github.com/nix-rust/nix/pull/2091](nix PR for features drop) * [https://doc.rust-lang.org/stable/src/std/os/fd/owned.rs.html#170-182](OwnedFd docs) * [/~https://github.com/nix-rust/nix/pull/1906](nix PR for FD I/O safety)
Two major changes: - features must be included; we only need two (this will speed up compiles!) - FD safety; we now need an OwnedFd, so attempting to use the fd after the object is dropped is no longer valid. Additionally, we don't need to implement Drop, as OwnedFd already has a drop which calls close References: * nix-rust/nix#2091 - nix PR for features drop * https://doc.rust-lang.org/stable/src/std/os/fd/owned.rs.html#170-182 - OwnedFd docs * nix-rust/nix#1906 - nix PR for FD I/O safety
A breaking change in the latest `nix` crate now requires specifying which features to enable: nix-rust/nix#2091
A breaking change in the latest `nix` crate now requires specifying which features to enable: nix-rust/nix#2091
The original intention behind #1498 was to allow repositories to slim down what amount of nix they consume, saving on compile time. However, with all these features enabled by default, it only takes one crate in your entire dependency tree that doesn't set
default-features = false
and suddenly you're back to square one. The better solution IMHO would be to always require crates to opt in to the functionality they need, ensuring that this footgun can't fire.This is a breaking change, and would need a new version number before release.