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

Do not enable any features by default #2091

Merged
merged 4 commits into from
Aug 11, 2023

Conversation

smalis-msft
Copy link
Contributor

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.

@smalis-msft
Copy link
Contributor Author

It looks like in order for this to be doable a lot of cleanup is needed around putting usings and test code behind feature flags, and then updating CI to test with --all-features. Unfortunately I don't have the time to take that on.

@smalis-msft smalis-msft closed this Aug 9, 2023
@asomers
Copy link
Member

asomers commented Aug 9, 2023

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))]

@smalis-msft smalis-msft reopened this Aug 9, 2023
@smalis-msft
Copy link
Contributor Author

smalis-msft commented Aug 9, 2023

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.

@asomers
Copy link
Member

asomers commented Aug 11, 2023

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?

@asomers
Copy link
Member

asomers commented Aug 11, 2023

Actually, adding a new feature probably isn't good, because it would make the documentation longer and cargo hack would take even more time. Since this pseudofeature is only needed at one place in the code, I think it would be better just to put a very long #[cfg_attr(not(all(feature = "...", ...)), allow(unused_imports))]

@smalis-msft
Copy link
Contributor Author

I was afraid you'd say that lol.

@asomers asomers added this to the 0.27.0 milestone Aug 11, 2023
Copy link
Member

@asomers asomers left a 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 ?

@asomers asomers added this pull request to the merge queue Aug 11, 2023
Merged via the queue into nix-rust:master with commit 1fbc897 Aug 11, 2023
39 checks passed
@smalis-msft smalis-msft deleted the no-default-features branch August 11, 2023 17:17
rkuris added a commit to ava-labs/firewood that referenced this pull request Aug 28, 2023
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 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 PR for FD I/O safety)
rkuris added a commit to ava-labs/firewood that referenced this pull request Aug 28, 2023
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
jlebon added a commit to coreos/rpm-ostree that referenced this pull request Sep 18, 2023
A breaking change in the latest `nix` crate now requires specifying which
features to enable:

nix-rust/nix#2091
cgwalters pushed a commit to coreos/rpm-ostree that referenced this pull request Sep 18, 2023
A breaking change in the latest `nix` crate now requires specifying which
features to enable:

nix-rust/nix#2091
@ryoqun ryoqun mentioned this pull request Apr 5, 2024
3 tasks
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.

None yet

2 participants