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

Automatic features. #1787

Closed

Conversation

withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Nov 6, 2016

@withoutboats withoutboats added the T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. label Nov 6, 2016
@withoutboats
Copy link
Contributor Author

This proposal originates from the conversation in this thread, about how to make the orphan rules less of a bummer.

This specifically solves the problem of "I want to implement some traits for my type, but I don't want my users to have to add this dependency / have to fiddle with features" problem. It does not solve the "this type in a library I'm using should implement this trait, but the author didn't provide that implementation" problem, which is much trickier.

The idea behind this RFC was mine, and I think it is the best solution to a problem that is happening right now, but I don't feel super motivated to drive this problem to completion, so if someone else wanted to "co-sponsor" this RFC and be really active about it, I would be stoked about it.

cc @nikomatsakis @aturon @wycats @sgrif all of whom have talked about issues in this area.

@DanielKeep
Copy link

A concern I have with this are incidental dependencies.

For example, a crate I'm working on at the moment can optionally use regex for some parsing. However, using regex also means using lazy_static. Thus, I couldn't use this: Cargo doesn't allow one dependency to trigger the inclusion of another dependency.

A more general design might be to allow features to have an optional activate-if-available that lists dependencies that, if they're all already being used somewhere else in the dependency graph, cause the feature to be activated by default.

... of course, this same crate also wouldn't be able to use that because it'd be too easy to end up with multiple parsing implementations enabled. At least, not without having to explicitly control activation order everywhere in the code... I really wish Cargo would let us define mutually exclusive features...

I suppose my position is: "I'd like this feature, but I don't think this specific design would work in practice."

@withoutboats
Copy link
Contributor Author

withoutboats commented Nov 6, 2016

@DanielKeep This doesn't seem different from having multiple dependencies on the cfg: #[cfg(dependency=regex, lazy_static)]. That is a refinement that clearly should be thought through though.

```rust
/// lib.rs

#[cfg(dependency=foobar]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[cfg(dependency = "foobar")]

For example:

```rust
/// lib.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// is definitely not what you wanted.

```rust
/// foobar.rs

extern crate foobar;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is a departure from the very strong convention of only using extern crate in the crate root. It also has substantial potential to be confusing, because now for to import something from that crate you’d need use self::foobar::Bar; or use foobar::foobar::Bar; rather than use foobar::Bar;.

Unlike other forms of compilation, the `cargo doc` command will treat
conditional dependencies as present by default, in order to document the APIs
which exist only when conditional dependencies are present. The conditional
dependency may be provided somehow.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional dependency may be provided somehow.

I don’t know what you mean. The sentence doesn’t make sense. Do you mean documented, i.e. some kind of banner, like with the stability attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is supposed to be documented, yes, will update

@Nemo157
Copy link
Member

Nemo157 commented Nov 6, 2016

I like it, especially the part about cargo doc documenting all the optional dependencies. I've just written a crate with a few features and really wishing that cargo doc would just auto-generate something letting users know what different features activate. In fact I'm just about to add a serde feature to that crate for conditional deserialization support so if you get this implemented in the next couple of days I'll use it instead 😜


if a matching source for a conditional dependency (e.g. a matching version number) is already present in the graph

I think this could warrant some extra warning information added to cargo, e.g. assuming you have a dependency foo with a conditional implementation for serde 0.8.0 which you are currently using everything will work fine, if at some point in the future you update to serde 0.9.0 in the root crate it will no longer match and you'll start getting build errors like foo::Bar does not implement serde::Deserialize.

If cargo detects this it would be nice to have it suggest that the reason is the non-matching dependency versions. In this case it would probably be easy to detect why it happened, but if it's related to interaction between dependencies of dependencies it might be harder. I think this can only happen with breaking updates (assuming all crates are following semver around publicly exposed dependencies properly), but there might be some case I haven't thought of that would allow a non-breaking dependency update to break this.


Also, what about conditional implementations for multiple versions of a dependency? It might be nice to have something like

#[cfg(dependency = "foobar:1.0.0")]
mod foobar {
    extern crate foobar;
    impl foobar::Bar for MyType {
        ...
    }
}
#[cfg(dependency = "foobar:2.0.0")]
mod foobar {
    extern crate foobar;
    impl foobar::SuperBar for MyType {
        ...
    }
}

although I have no idea how you would specify that in the Cargo.toml 😕

@chris-morgan
Copy link
Member

@withoutboats #[cfg(dependency=regex, lazy_static)] would be #[cfg(all(dependency = "regex", dependency = "lazy_static"))]

matching version number) is already present in the graph, cargo will
automatically add that dependency.

## `#[cfg(dependency=)]`
Copy link
Member

@Manishearth Manishearth Nov 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we continue to use cfg(feature=) and specify conditional deps in Cargo as

[features.serialization]
crates = ["serde", "blah"]
magic = true #please bikeshed the name

instead? I don't really like creating a new system which works mostly like cargo features but is incompatible with them.

This also lets you specify features that should be brought in only when more than one crate is included, which the current proposal does not. It also means that there can be a straightforward transition -- folks can continue to specify optional dependencies via explicit features for crates which have switched to magic dependencies. It's all nice and interoperable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought a bit about this and there are two syntaxes which are equivalent that we can decide between, will write up more in a comment but it probably won't be today.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic route is what Cabal uses for its flags, basically.

Somewhat relatedly, I think target.cfg(....).foo syntax is really good---way better than optional syntax. I would love:

[targets.'cfg(and(or(feature = "foo", feature = "bar"), create = "baz"))'.asdf]
#...

will only be compiled when a certain dependency has been explicitly passed to
rustc using the --extern flag.

Because cargo automatically passes dependencies explicitly with this command,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think cargo build -p foo may need to be tweaked to support this case. Minor issue.

# Alternatives
[alternatives]: #alternatives

We considered options like allowing the orphan rules to be broken by certain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW the application of sibling crates is much more than just optional dependencies.

One example is Servo. Servo is nobody's dependency, and has a bunch of crates, split up for compile times. Between these crates it would be lovely if we could bypass the orphan rules -- the main reason for the orphan rules is so that downstream users shouldn't get conflict issues that they're powerless to fix (without editing their dependencies directly), but in this case we have no downstream users -- the downstream crates are all Servo crates, and if a change in layout forces me to make a change in style to avoid an ambiguity, so be it. All the Servo crates should really be considered as a single whole wrt orphan rules.

I recently was forced to merge three crates to get around orphan rule issues (and other things).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this isn't going to solve this problem. The solution to this problem is incremental compilation and then don't break up your projects into crates just to get coarse grained incremental compilation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a bit extreme though. We still want there to be crates for code organization and code reuse (for example now Firefox uses our style crate), but it would be nice if orphan rules could be broken amongst these crates.

(Yes, in this case that means we do have downstream users, but that's C++ code)

Copy link
Contributor Author

@withoutboats withoutboats Nov 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion any solution that means that rustc is no longer able to enforce the orphan rules without cargo is hard stop untenable. I'm open to any way to resolve the papercuts of the orphan rules that doesn't break that rule.

The other approach to this problem I've been taking is to push specialization's rules further so that we can define flexible blanket impls, so that hopefully you can get several trait impls for your type for free. Hopefully this would apply to Servo also.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to any way to resolve the papercuts of the orphan rules that doesn't break that rule.

Yeah, I think we can design this to work without cargo. But this is pretty off topic, just wanted to point out (like sgrif) that the orphan rules have a few more problems that this can't help with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we can design this to work without cargo.

Cool. :-) I'm always very excited to hear new ideas to make the orphan rules less of a burden. I'd love to continue this conversation off of this RFC thread.

@Ericson2314
Copy link
Contributor

So I'd really like to enforce that all uses of these conditional dependencies are propoerly guarded by a cfg attribute. As I argued in the internals thread for scenarios, features + a similar cfg-check probably cover scenarios, so now we have two uses for such a thing.

conditional = true
```

A single dependency objecy cannot contain both a `conditional` key and an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/objecy/object

@sgrif
Copy link
Contributor

sgrif commented Nov 6, 2016

This is a great proposal, and I like the overall idea. This can potentially let us sidestep a lot of issues with Cargo's version resolution if done properly. Having rustdoc include conditional dependencies by default is a nice touch as well. I strongly disagree with the notion that this solves the problems created by the orphan rule, as this still requires a crate to care about every other crate in the ecosystem, or for every other crate in the ecosystem to care about that crate. However, this does greatly improve ergonomics for the end user without having to address the orphan rule directly.

@withoutboats
Copy link
Contributor Author

So I'd really like to enforce that all uses of these conditional dependencies are propoerly guarded by a cfg attribute.

What we need are two compilation modes - one with the crate under compilation's conditional dependencies turned on, one with them turned off.

@withoutboats
Copy link
Contributor Author

I strongly disagree with the notion that this solves the problems created by the orphan rule

Definitely didn't mean to imply that this solves all of the negative consequences of the orphan rules.

@withoutboats
Copy link
Contributor Author

withoutboats commented Nov 7, 2016

So I think the actual issue that isn't solved by this scheme is not a situation where want to include some code if both (e.g.) regex and lazy_static are in the graph, but where you want to include some code if regex is in the graph, but that requires adding lazy_static.

How often is this desirable? If its something you usually need, it seems like we can do it, but it introduces other questions - if my dependency adds lazy_static while resolving a conditional dependency for regex, and another dependency has a conditional dependency on lazy_static, do we do another round of conditional dependency checking? If that second round adds some more new dependencies, do we continue doing this indefinitely? So on.

The syntax for doing that which is consistent with the syntax in this proposal seems to be accepting a string for the conditional member of a dependency, which indicates that it is conditional on another crate being in scope. As in:

[dependencies.regex]
version = "1.0.0"
conditional = true

[dependencies.lazy_static]
version = "1.0.0"
conditional = "regex"

@Ericson2314
Copy link
Contributor

Ericson2314 commented Nov 7, 2016

@withoutboats

What we need are two compilation modes - one with the crate under compilation's conditional dependencies turned on, one with them turned off.

Actually that wouldn't cut it. For example, it erroneously allows:

#[cfg(dependency=foo)]
extern bar;
#[cfg(dependency=bar)]
extern foo;

My original rambling post is https://internals.rust-lang.org/t/fleshing-out-libstd-scenarios/4206/9?u=ericson2314, the tl;dr is we need to do SAT (and when we take into account upstream restrictions, perhaps easier to think of as as SMT).

@Manishearth
Copy link
Member

The syntax for doing that which is consistent with the syntax in this proposal seems to be accepting a string for the conditional member of a dependency, which indicates that it is conditional on another crate being in scope.

Could we make this still tie-in with features (so that there is no distinction between cfg(feature) and cfg(conditional))? In your specific syntax I think just requiring people to define a regex feature (the way optional=true works right now) would be enough. However, I'd prefer marking the feature as magic/conditional rather than the dependencies.

Your new proposal still misses one use case, where you have two partially overlapping sets of conditional dependencies. Sounds pretty rare, but that and more complicated overlappings are cleanly handled by the magic feature proposal.

Regarding having to re-resolve indefinitely, it's not indefinite -- there is a finite number of dependencies and the conditional resolution algorithm is monotonic.

@withoutboats
Copy link
Contributor Author

withoutboats commented Nov 7, 2016

@Manishearth I imagine making this a 'property of features' would look something like this:

[features.use_serde]
dependencies = ["serde"]
automatic = true

And then the feature would be opted into automatically if the dependencies listed are already present. You could also imagine that automatic (or whatever) can take a list of dependencies separate from the dependencies listed as members of this feature, and the feature is opted into if the dependencies in automatic are listed.

These syntaxes seem equally capable to me. I'll list this as an alternative when I revise this RFC. I personally prefer the syntax in the RFC right now because it creates a distance between this and features. I don't see this as a system for making features easier to opt into, but as a system for ergonomically solving a problem that is being solved unergonomically with features right now. Maybe my view of it is limited though. I will admit I have a bias against feature systems in general, including ours. Because of their generality, they are always sort of frustrating to use in my experience. That's also why I'm a little wary about making this feature more and more flexible.

I'd also point out that you need to come up with a name for your feature if this has to be a feature, because it can't be the same name as a dependency. :-\

Your new proposal still misses one use case, where you have two partially overlapping sets of conditional dependencies.

I do not know what you mean by this.

@Manishearth
Copy link
Member

These syntaxes seem equally capable to me.

More or less yeah. The main difference is that this creates a second config system independent of features, whereas I'd prefer to work within the same framework. I really want to avoid a system where you have both cfg(feature=) and cfg(conditional=). If using features, you get interoperability with features, easy upgrades without breaking changes (folks can make their features automatic, and users won't be forced to change their cargo files to upgrade, nor do maintainers have to keep both a feature and a conditional alive in parallel till the next breaking change), and the same ability to pick and choose features in deps that we already have (instead of including conditional deps that match the ). This especially matters for conditional deps that need more than one crate to work (serde+serde_derive, for example). As a library I may not be using serde_derive, but I would like to trigger the serde conditional in a dependency of mine. I can't without including serde_derive (which means I have to go back to the dependency's cargo.toml and pick up all the conditional deps). However, if it has a "serialize" feature I can easily specify features = "serialize" for the dependency and it will work. Furthermore, the dependency can add more deps to the feature and I don't have to change anything.

I do not know what you mean by this.

I was wrong. Thought about it more.

@withoutboats
Copy link
Contributor Author

withoutboats commented Nov 7, 2016

If using features, you get ... easy upgrades without breaking changes (folks can make their features automatic, and users won't be forced to change their cargo files to upgrade, nor do maintainers have to keep both a feature and a conditional alive in parallel till the next breaking change)

This is pretty compelling!

@Ixrec
Copy link
Contributor

Ixrec commented Nov 8, 2016

Another reason I like the "automatic features" design is that I can imagine having features which some clients want to explicitly enable without caring exactly what set of (conditional) dependencies you use to implement those features.

If I use serde for everything, I'd like all my dependencies to automagically compile their serde impls. But if there's only one or two libraries I want to serialize types from, and I don't care whether Foo.serialize() uses serde or rustc-serialize or whatever, I'd prefer to turn on "the serialize feature" rather than pretend I depend directly on serde.

@nikomatsakis
Copy link
Contributor

FWIW, I prefer going through features for this.

@withoutboats
Copy link
Contributor Author

withoutboats commented Dec 1, 2016

Based on feedback, for the past three weeks I've intended to rewrite this from "conditional dependencies" to "automatic features," but I have trouble finding the time. If anyone wants to rewrite the proposal to work as @Manishearth suggested, I would happily accept the PR into this branch.

@Manishearth
Copy link
Member

I will ... try, but I'm not sure if I have time. (So if someone else can pick it up that would be great!)

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jan 5, 2017

@hvr, a Cabal developer, just told me that Cabal's "automatic flags" (basically this) are not intended to affect a package's interface. This means it is sound to use the global algorithm, which can discover optional edges the local one won't.

I think both are useful---for example I suppose if optional features don't affect the public API, they could transparently improve performance, in which case the more the merrier! [At least in release builds, more edges do reduce build parallelism.] The global algorithm's disadvantage---enabling more non-trivial edges---becomes and advantage in this case.

@Ericson2314
Copy link
Contributor

@sfackler Well, the other thing I've been thinking is "scenarios" need not be a whole new feature of Cargo, but rather a reformation of features/cfg. [Apologies for using the word "feature" in two different senses.]

@Manishearth
Copy link
Member

Crates don't, in fact, rely on the optional features.

Except for toplevel crates.

But this doesn't solve the original problem then. Let's say I have a crate which needs serialization to work. It wants to enable serde in all of its transitive dependencies. I want to be able to do this easily, so it just specifies a serde dep in the top level and the automatic features pull it in in its dependencies. If there is a version mismatch that's not a different situation than what we'd be in previously, and you have that kind of problem with non-optional dependencies anyway.

Not relying on optional features brings us back to the status quo.

Optional dependencies are only exposed

How would you gate that? Remember that an optional dependency is more than just an extra rlib; there will be cfgs in the parent crate that compile differently. You cannot control access to things affected by the cfgs.


Most of this is nicely solved by the fact that if you're relying on an optional dependency like serde you'll need to pull the crate in anyway to use it. It's not perfect, but basically we can declare that if a crate wish to forcibly trigger an optional dependency of its dependenc(ies) to be pulled in, you must specify its constituent crates/features in the crate that needs the dependency to be triggered. This can be a rule that is not enforced; I'm not sure if it can be enforced.

@Ericson2314
Copy link
Contributor

Except for toplevel crates.

I'm not sure what's special about them here? Moreover making the top ones of a DAG special is IMO a smell. Inductively, the leaf nodes are the base case and the roots are just more interior nodes.

Let's say I have a crate which needs serialization to work. It wants to enable serde in all of its transitive dependencies....

Good point. Heh, oh the joys of trying to find something both sound and useful! One possible solution (possibly sound, in that I just thought of it :)) is feeding in crates to all their siblings' descendants. In your example, since serde is a dependency of the root crate, everything gets built with serde-deps enabled, but only the root crate gets to see those optional features enabled by serde.

Maybe that's good enough, or maybe the intermediate crates needs to know their deps also use serde so they can write the relevant instances. In that case, they would need their own optional dependencies anyways for their serde usage. I forgot about routing optional sibling crates in my previous post, but that would allow the serde optional dep to be propagated without the change from the previous paragraph. [This removes the embarrassingly parallel aspect, but keeps the algorithm "weakly inductive" (I think that's the term) where you only consider a node and its immediate children rather than all descendants.]

Say there is a "gap" in the dag, with some crates using serde not reachable via the root crate following edges between serde-caring crates (which is using serde). Does it matter? I'm not sure. If it does, then we need both ideas---propagating optional features and propagating siblings to siblings' children.

If there is a version mismatch that's not a different situation than what we'd be in previously, and you have that kind of problem with non-optional dependencies anyway.

Ah! Let me add a detail I forgot in my previous paragraph (and which I am not adding there retroactively for the sake of information density). Each serde-aware crate should have have an optional automatic feature on serde, and that optional dependency should require (not merely allow to be enabled) the optional dependency of its immediate dependencies. Then, inductively, all the crates will be required to use serde together, and we, erm, somewhat side step your concern about whether versions should be resolved entirely before automatic features.

How would you gate that? Remember that an optional dependency is more than just an extra rlib; there will be cfgs in the parent crate that compile differently. You cannot control access to things affected by the cfgs.

This is the hard crux of what I think is already proposed as part of scenarios. We need to mask the interface of crates to match these hypothethetical other resolutions. The "Additivity" rule from #1841 is essential to make sure this is always upcasting and thus possible.

@Manishearth
Copy link
Member

Each serde-aware crate should have have an optional automatic feature on serde, and that optional dependency should require (not merely allow to be enabled) the optional dependency of its immediate dependencies.

No. That's my entire point. This is the status quo, this is exactly what you do when you want to use features. This has always been problematic, because you need to thread features through a million different Cargo.toml files. This is exactly what this RFC is trying to avoid.

One possible solution (possibly sound, in that I just thought of it :)) is feeding in crates to all their siblings' descendants.

Breaks down in the presence of generic types being pulled in from a different part of the dag.

@Ericson2314
Copy link
Contributor

No. That's my entire point...

Well then at least we need to require that no two automatic features are triggered by different version of the same crate. Still thinking about the rest.

@Manishearth
Copy link
Member

That restriction feels strange, but I don't see any actual issues with it.

@Ericson2314
Copy link
Contributor

Yeah its weird to me too. Very global, very not inductive

If there is a version mismatch that's not a different situation than what we'd be in previously, and you have that kind of problem with non-optional dependencies anyway.

Oh, do you mean the sketchy fact that we already allow multiple versions of the same crate in build plans?

@Manishearth
Copy link
Member

Oh, do you mean the sketchy fact that we already allow multiple versions of the same crate in build plans?

Yeah.

@Ericson2314
Copy link
Contributor

Breaks down in the presence of generic types being pulled in from a different part of the dag.

You mean generics + traits? I'd think generics alone would be fine.

@alexcrichton alexcrichton added T-cargo Relevant to the Cargo team, which will review and decide on the RFC. and removed T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. labels May 22, 2017
@aturon
Copy link
Member

aturon commented Sep 6, 2017

Based on discussion at the most recent Cargo team meeting, I'm going to close this as postponed.

While this still seems like a very promising idea, there are some more pressing revisions for the feature system (and in general bigger fish to fry with Cargo right now). Let's plan to revisit in 2018!

@aturon aturon closed this Sep 6, 2017
@aturon
Copy link
Member

aturon commented Sep 6, 2017

Sorry, that was a bit terse -- I'm trying to push on a bunch of RFCs and went too quickly. In more detail, Cargo folks are definitely fans of this idea. But:

  • There are some significant issues with the feature system right now, which we intend to revise as part of the build system integration work. We're generally holding off on extensions to the feature system until we've successfully rationalized what's there.

  • Our hands are going to be extremely full with some of the tasks on deck (the build system work alone could easily take the rest of the year).

Put together, it seemed wisest to delay finalizing the design here until the dust settled in other parts of Cargo.

@vi
Copy link

vi commented Aug 18, 2018

Has the dust already settled?

"Include tokio/async-specific features" seem to be a good example of an automatic feature. Otherwise either tokio/futures would suddenly appear unless default-features = false; or all async users need to write the curly brackets in the dependency line.

@kentfredric
Copy link

Having vendored perl packages which have been doing this since forever, I can only say that in practice its a terrible idea.

The last thing you want when trying to build a reliable system is having to think about the possibility you will incur a functional change as an incidental side effect of something simply having been present, where it previously was not.

Clarity in dependencies and explicitly requiring things when you need them is worth the extra tiny amount of developmental pain.

I'd hate to ever update one of my dependencies, which in turn, pulls in a new dependency, and have it turn on a feature in a different dependency which changes how my code behaves.

That's right in "spooky action at a distance" territory, and can be a nightmare to work out where the breaking change was introduced.

(Isn't that part of why we like use as opposed to a great big global share of everything like C has? ... 'automatically visible functions'. )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-cargo Relevant to the Cargo team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.