-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Automatic features. #1787
Closed
withoutboats
wants to merge
4
commits into
rust-lang:master
from
withoutboats:conditional_dependencies
Closed
Automatic features. #1787
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
- Feature Name: `automatic_features` | ||
- Start Date: 2016-11-05 | ||
- RFC PR: (leave this empty) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
Support "automatic" features, which will be used only if the dependencies of | ||
that feature are already present. Users will be able to write code to integrate | ||
with another library (for example, serializations for their type) without | ||
introducing a *necessary* dependency on that library for users who don't need | ||
it. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
Currently, a common pattern exists for libraries to have feature flags which | ||
turn on a dependency. For example, if I have a library which defines a new | ||
type, I might want users to be able to serialize it with `serde`, but I don't | ||
want users who don't care about this functionality to have to add `serde` to | ||
their dependency graph. So I add a `serialization` feature to my Cargo.toml, | ||
and only if you turn on this feature do you get the feature for serialization. | ||
|
||
Ultimately, the source of this problem is the orphan rules - because only *I* | ||
can define the impls for my type, I have to add this dependency. For every | ||
commonly used trait defining library, I have to add a new feature, and all of | ||
my clients have to turn on that feature. | ||
|
||
cargo features have many uses, but this particular pattern ends up being a lot | ||
of fiddling for a very common use case. Instead of requiring users go through | ||
the feature flag rigmarole, we can provide first class support for this | ||
pattern through "conditional dependencies" - dependencies that are turned on | ||
automatically if they were already present. | ||
|
||
# Detailed design | ||
[design]: #detailed-design | ||
|
||
## Automatic features in `Cargo.toml` | ||
|
||
When declaring a feature in a Cargo.toml file, if that feature is an | ||
object, it can contain the `automatic` member. For example: | ||
|
||
```toml | ||
[features] | ||
foobar = { dependencies = ["foo"], automatic = true } | ||
``` | ||
|
||
Or: | ||
|
||
```toml | ||
[features.foobar] | ||
dependencies = ["foo"] | ||
automatic = true | ||
``` | ||
|
||
When generating the dependency graph, cargo will at first not include features | ||
tagged automatic (unless asked to do so specifically via `--features` or if the | ||
feature is a default feature and `--no-default-features` has not been past). It | ||
will then look for features (in all crates in the graph) that only need | ||
dependencies which are already part of the graph. These features will be | ||
enabled. This will add more edges to the dependency graph, but not more nodes. | ||
|
||
Note that features can depend on crates as well as _other features_, so this | ||
might be implemented as a multi-pass process. | ||
|
||
For example, I can have the `foo` crate with an automatic feature `serialize` | ||
which enables a dependency on serde. I can also have the `bar` crate (which | ||
depends on `foo`) with an automatic feature of the same name which enables both | ||
a dependency on serde and a dependency on `foo/serialize`, i.e. it enables the | ||
`serialize` feature of its dependency `foo`. If serde was included, then in the | ||
first pass, the automatic feature `serialize` on `foo` will be enabled, but not | ||
on `bar`. In the second pass, because `foo/serialize` now exists, | ||
`bar/serialize` is automatically enabled as well. | ||
|
||
The process terminates because it is monotonic -- it only ever adds edges to the | ||
graph, never removing them. | ||
|
||
## Convention for using automatic features | ||
|
||
If you want to add a new automatic feature, you should add a top level | ||
submodule (that is, mounted directly under lib, main, etc) with the same name | ||
as the automatic feature. This module will be tagged with the cfg, and the | ||
extern crate declaration and all code that uses symbols from that crate will | ||
go inside of that module. | ||
|
||
For example: | ||
|
||
```rust | ||
/// lib.rs | ||
|
||
#[cfg(feature=foobar)] | ||
mod foobar; | ||
``` | ||
|
||
```rust | ||
/// foobar.rs | ||
|
||
extern crate foobar; | ||
|
||
impl foobar::Bar for MyType { | ||
... | ||
} | ||
``` | ||
|
||
## `cargo doc` compiles with all dependencies | ||
|
||
Unlike other forms of compilation, the `cargo doc` command will treat | ||
automatic features as present by default, in order to document the APIs | ||
which exist only when automatic features are present. | ||
|
||
# Alternatives | ||
[alternatives]: #alternatives | ||
|
||
We considered options like allowing the orphan rules to be broken by certain | ||
crates that were recognized as "sibling" crates by a parent, as a way to get | ||
around the orphan rule issue. However, this alternative would create a | ||
soundness hole in the orphan rules and violate good layering, by making them | ||
only fully enforced by cargo and not by rustc. Ultimately, conditionally | ||
altering the shape of code compiled by the flags passed by cargo seemed like | ||
a better solution to the problem. | ||
|
||
A previous version of this RFC proposed "conditional dependencies", which let | ||
you specify individual dependencies as "conditional", which would be turned on | ||
if the dependency already existed in the graph. However, this introduces a | ||
parallel way of specifying optional dependencies which doesn't integrate with | ||
features. Automatic features provide a smooth transition and also allow | ||
dependencies to be grouped together. | ||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
The Cargo.toml key names ("dependencies" and "automatic") could be bikeshedded. | ||
|
||
Should we allow automatic features which depend on other features? This seems | ||
like a natural thing to do, but it complicates the resolution process. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
All dependencies define their own features, so I'm assuming
automatic
can be applied dependency declarations as well?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.
We could let that be the case. I'd prefer if it went by the route where you must create a feature of the same name and use that, but no strong reason (mostly just to avoid it getting confusing) for this.
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 would prefer that approach as well, but that seems like a whole other RFC!
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'm not sure why it needs to be a different RfC. This RfC covers it already -- you can already create feature names that shadow crate dependencies.
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 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 fact that (right now) you need to have a different name for the feature from the dependency is a big part of why I was in favor of the conditional dependencies proposal in the first place. Saying "automatic" could be applied to features is just returning to that proposal, but changing the key from "conditional" to "automatic" (which doesn't make sense to me).
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.
So the reason that features can't have the same names as dependencies is that all dependencies are also features. This works, for example:
The dependency doesn't even need to be optional.
Allowing these implicit features to be configured would be fantastic though if you, for example, need to pull in another crate for the functionality to work. I used to use implicit featuers in rust-postgres but had to move from e.g.
--features openssl
to--features with-openssl
because of that issue.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.
@withoutboats Well, the difference is that the new proposal is still applying to features (and thus not requiring any changes to rustc), and still integrates with the rest of the feature system (and still works for multi-dependency features). I would prefer to push for allowing dependency features to be shadowed, which fixes this problem for us.
@sfackler yeah, I was aware, I just thought that you could shadow that.
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.
Thanks, I knew that dependencies were features but the implications of that hadn't been obvious to me before.
So it seems like the normal way to do this is going to be something like this:
I don't love having to attach two booleans to the dependency object, but I'm not sure some sort of third key that is the same as
optional && automatic
is a good idea.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.
FWIW I was talking with @wycats and it seems like the lack of feature shadowing is mostly an oversight. We should just include that in this rfc.