-
Notifications
You must be signed in to change notification settings - Fork 654
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
Conditionally enable functionality with feature flags #1498
Conversation
f76c43a
to
d17c09c
Compare
Using features reduces build time and size for consumer crates. By default all features are enabled.
@rtzoeller @vdagonneau would either of you like to review this PR? |
@asomers I should be able to look at this PR this weekend! |
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.
Running cargo doc --no-default-features
revealed some items which were potentially missed when adding things to features.
nix::fcntl::SealFlag
, for instance, seems like it should be restricted to the fs
feature.
CHANGELOG.md
Outdated
@@ -6,6 +6,9 @@ This project adheres to [Semantic Versioning](https://semver.org/). | |||
## [0.23.0] - 2021-09-28 | |||
### Added | |||
|
|||
- Added fine-grained features flags. Most Nix functionality can now be | |||
conditionally enabled. By default, all features are enabled. | |||
(#[TODO](https://github.com/nix-rust/nix/pull/TODO)) |
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.
Don't forget to update this link now that the pull request is enabled. It looks like this is also in the 0.23.0 release section, and needs to be moved.
Cargo.toml
Outdated
"acct", "aio", "dir", "env", "event", "features", "fs", | ||
"hostname", | ||
"inotify", "ioctl", "kmod", "mman", "mount", "mqueue", | ||
"net", "personality", "process", "poll", "pthread", "ptrace", "quota", |
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.
The p's and q's aren't quite alphabetical, either in the default list or the list of feature dependencies below.
//! * `net` - Networking-related functionality | ||
//! * `personality` - Set the process execution domain | ||
//! * `process` - Stuff relating to running processes | ||
//! * `poll` - APIs like `poll` and `select` |
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.
Not quite alphabetical.
src/fcntl.rs
Outdated
@@ -165,6 +172,9 @@ libc_bitflags!( | |||
} | |||
); | |||
|
|||
feature!{ |
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.
I know nix doesn't use rustfmt (but maybe someday that can change!), but I think it's worth pointing out that rustfmt wants there to be a space between the !
and {
in all of these feature macros. Might be worth a trivial bulk find and replace, since we're already adding noise to all these files' diffs.
feature!{ | |
feature! { |
@@ -20,6 +20,8 @@ build: &BUILD | |||
- $TOOL +$TOOLCHAIN $BUILD $ZFLAGS --target $TARGET --all-targets | |||
- $TOOL +$TOOLCHAIN doc $ZFLAGS --no-deps --target $TARGET | |||
- $TOOL +$TOOLCHAIN clippy $ZFLAGS --target $TARGET -- $CLIPPYFLAGS | |||
- if [ -z "$NOHACK" ]; then $TOOL install cargo-hack; fi | |||
- if [ -z "$NOHACK" ]; then $TOOL hack $ZFLAGS check --target $TARGET --each-feature; fi |
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.
There are enough features that it's not feasible to run cargo hack check --feature-powerset
, but it may be useful to run it with limited depth.
$ cargo hack check --feature-powerset --depth 2
I found one issue (and possibly more, but I can't seem to get cargo hack check
to not fail fast) by setting the depth to 2.
The following command fails:
$ cargo check --no-default-features --features mount,socket
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.
Good catch. This became an error only after PR #1546, though it was probably wrong anyway. I'm not sure why I put that part about uio in there.
Running
cargo doc --no-default-features
revealed some items which were potentially missed when adding things to features.
nix::fcntl::SealFlag
, for instance, seems like it should be restricted to thefs
feature.
Good catch. I only checked the no-features docs on FreeBSD, where SealFlag doesn't exist. What about unistd::pipe
? It doesn't seem essential, like read
and write
, but it also doesn't seem related to any other feature and I didn't want to create a new feature just for it.
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.
Agreed it doesn't make sense to have a dedicated feature for it.
Given the options of sticking it in a feature now and potentially moving it to a different feature later (because we aren't happy with the location), or leaving it unconditionally present for now and potentially moving it to a feature later, I prefer the second option. Both would be breaking changes, but the second feels less confusing to me.
I think you should leave it as essential for now.
I think I addressed all of your comments. HOWEVER, rustdoc's behavior just changed, which will require revisiting every part of this PR. I'm working on that now. |
One other note: if you've considered updating the codebase to use |
So I played with this some more, and I've come to the conclusion that the new rustdoc feature isn't good enough to be useful. While it's a step in the right direction, it just isn't good enough, so we'll basically have to code as if it weren't there. Worse, we'll have to carefully workaround the new feature so the docs don't become confusing. The problems are that:
|
This should be merged in a separate PR
The reasons not to use it are: * Items that aren't built on the platform for which the docs were built won't show up in the docs. For example, if you build the docs on FreeBSD, you won't see that there's a kmod module only available for Linux. * Some items are built with different signatures on different OSes. A "target_os" annotation won't tell the reader that. * Using #[cfg(any(docsrs, target_os=...))] is not a solution, because too many items will simply throw errors when generating the documentation that way. For example, every enum value in a libc_enum! invocation and every const in a libc_bitflags! invocation. * Annotating the docs with a bunch of OS notes gives readers a false sense of security. For the reasons noted above, those annotations will be incomplete, which might deter users from checking the docs built for their platform. So I think we should carefully remove all of those annotations from the docs. However, I'm leaving in place annotations about target_env and target_arch, because we don't build docs separately for all of those combinations.
@asomers I think we should consider whether or not we expect tests with just some features enabled to work. E.g. should I'm also fine with saying "ideally yes, but not in this PR". |
It would be nice. OTOH it would clutter up the test files with a lot of |
This PR satisfies a long-running request: to slim down the Nix crate by conditionally enabling certain functionality. By default everything is enabled, but consumers can reduce that by selecting a subset of features.