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

RFC: Remove the implicit features associated with dependencies #1286

Closed
sfackler opened this issue Feb 10, 2015 · 32 comments
Closed

RFC: Remove the implicit features associated with dependencies #1286

sfackler opened this issue Feb 10, 2015 · 32 comments
Labels
A-features Area: features — conditional compilation

Comments

@sfackler
Copy link
Member

Background

Features in a Cargo crate setup can currently be created in two ways. Features can be manually added:

[features]
foo = []
bar = ["a/b", "c"]

Manually added features can depend on other features. bar above depends on feature c from its own crate, and feature b from crate a.

In addition, all dependencies have an implicit associated feature with the same name. However, these features cannot depend on other features.

The Problem

The current setup misses some important use cases. For example, rust-postgres has a uuid feature which adds support for the Postgres UUID type by linking to the uuid crate and generating trait implementations for the uuid::Uuid type. A separate crate, rust-postgres-array adds support for array types. Here, we would like to have the same setup - a uuid feature that adds support for the Postgres UUID[] type by linking to the uuid crate and adding the appropriate trait implementations. The uuid feature of rust-postgres-array needs to depend on functionality provided by the uuid feature of rust-postgres, but this isn't currently possible since uuid is a feature implicitly created by the uuid dependency.

There exists a kind of workaround here, where you define a feature with a different name that depends on uuid:

[features]
uuid_support = ["uuid", "postgres/uuid"]

That's a bit unfortunate, though. Having the feature name match the crate name avoids a bit of verbosity and makes it explicit that it's using that crate specifically (for example, support for the Postgres JSON type is provided via rustc-serialize::Json, and naming the feature rustc-serialize makes this point more clear than naming it json would, as well as keeping room for support via some other crate as well in the future).

I was initially going to change Cargo to add support for an optional dependency depending on features of other dependencies like so:

[dependencies]
postgres = "0.6"

[dependencies.uuid]
optional = true
version = "0.1"
features = ["postgres/uuid"]

After some more thought, I came to the conclusion that this was the wrong way to go about it, and deeper changes to Cargo's feature API would solve both this issue and some other issues:

Backwards compatibility hazards

The fact that all dependencies are also features poses backwards compatibility hazards for a crate author when making changes that would not otherwise be user facing. Imagine Alice makes a crate foo:

[package]
name = "foo"
version = "0.1.0"
authors = []

[dependencies]
bar = "0.1"

And Bob makes a crate baz that depends on foo:

[package]
name = "baz"
version = "0.0.1"
authors = []

[dependencies.foo]
version = "0.1"
features = ["bar"]

Note that he's enabled the bar feature on the foo crate. This is basically a no-op (it defines the feature = "bar" cfg when compiling foo, but let's assume that foo doesn't use that anywhere).

Now imagine Alice does some internal refactoring that allows her to remove the dependency on bar. She publishes version 0.1.1, but suddenly a bug report is filed on foo. The new release broke Bob's crate, since the bar feature no longer exists! Bob should never have enabled that "feature" in the first place, but that doesn't make the situation any better for the people that depend on the baz crate.

So why not forbid the use of features named after non-optional dependencies?

Features matching the name of a non-optional dependency are another important use case! rust-postgres currently depends on time for internal use that doesn't touch the public API. It also implements some traits for time::Timespec to enable support for the Postgres TIMESTAMP type. However, I want to make the TIMESTAMP support opt-in via a feature to enable me to drop the dependency on time if future work on rust-postgres removes its internal usage. If the user facing part (the trait implementations) is behind a feature, I can make the time dependency optional without breaking backwards compatibility.

Proposal

As discussed above, the current feature implementation is both too restrictive to enable some uses and too eager to enable dependency flexibility. Both of these problems boil down to Cargo's intertwining of features and dependencies. It seems like the solution here is to remove that interconnectedness.

Dependencies, mandatory or optional, will no longer automatically have associated features. Features can be defined with two forms. The short form matches visually with current feature syntax, though the meaning will be adjusted:

[features]
bar = ["a", "b/c"]

[dependencies.a]
optional = true

[dependencies.b]
optional = true

This defines a feature bar. While in current Cargo, bar would also activate the a and b/c features, it would now activate the optional dependency on crate a, as well as crate b, activating crate b's feature c. Note that the behavior here is actually identical between current Cargo and Cargo after the proposed changes. It will make a difference in a case like this:

[features]
bar = ["a"]
a = []

In current Cargo, this is valid, and activating feature bar causes feature a to be activated as well. In this proposal, this would be an error, since a is a feature and not a dependency.

If a feature needs to activate another feature, a more verbose method is allowed:

[features]
a = []

[features.b]
features = ["a"]
dependencies = ["foo"]

[dependencies.foo]
optional = true

Having this extended form could also be useful in the future to allow the addition of things like descriptions which could be displayed on crates.io:

[features.foo]
description = "Adds support for the foo thingy"
@sfackler
Copy link
Member Author

cc @alexcrichton @wycats this is what I mentioned on IRC earlier today. Thoughts?

@wycats
Copy link
Contributor

wycats commented Feb 10, 2015

@sfackler I'm glad you decided to flesh out the feature system rather than go down the path of a separate optional dependency system.

I originally designed it this way because I think that optional dependencies are just a degenerate form of features. I perhaps overzealously tried to make the simple optional dependency case ergonomic, but at the cost of making things features that shouldn't have been.

I think I buy these arguments, and imagine that the impact would be relatively low because features are probably used relatively little at the moment.

@sfackler do you agree with my assessment of the impact?

@sfackler
Copy link
Member Author

Yep, I think that's probably the case. We could also build in a bit of a deprecation period for the old style of features without too much pain I think.

@alexcrichton
Copy link
Member

This all sounds like a great idea to me, thanks for writing this up @sfackler!

@alexcrichton alexcrichton added the A-features Area: features — conditional compilation label Feb 11, 2015
@sfackler
Copy link
Member Author

Cool, I'll start on the implementation. One thing to clarify before I do: the short form of the feature syntax is mildly inconsistent because it contains dependencies from this crate, and features from other crates. That is, these features are equivalent:

[features]
a = ["a", "b/c"]

[features.a]
dependencies = ["a"]
features = ["b/c"]

It works this way because that seems to be what you want most of the time, i.e. it's relatively uncommon for one feature to depend on another in the same crate. Does that seem reasonable?

@sfackler
Copy link
Member Author

As @alexcrichton mentioned on IRC, b/c actually makes sense to think about as a dependency as well, so the short form array just contains dependencies, and the features array is only for features in the same crate.

sfackler added a commit to sfackler/cargo that referenced this issue Feb 12, 2015
See the associated issue for more details.

Closes rust-lang#1286.
@wycats
Copy link
Contributor

wycats commented Feb 12, 2015

@sfackler I agree with @alexcrichton

@bluss
Copy link
Member

bluss commented Mar 11, 2016

Feature groups aren't that uncommon. Feature documentation would be great. I want to be able to communicate what features require, in particular two aspects: (1) does this feature require nightly or stable rust (2) Is this feature a stable part of the crate's api or not.

@jethrogb
Copy link
Contributor

jethrogb commented Jul 17, 2016

Another issue of mixing features and dependencies, which I'm not sure is covered by the redesign. Say you have a feature F and a dependency D that also has a feature F. I want to be able to encode the following cases:

  1. no features, no dependencies
  2. feature F, but no dependencies
  3. dependency D without any features, no features
  4. feature F and dependency D with feature F

The problem is that the current implementation has no way to write option number 2 and 4. If you write F = [], option 2 works, but trying to use option 4 doesn't activate the feature on D. If you write F = ["D/F"], option 4 works, but trying to use option 2 will activate also dependency D.

@alexcrichton
Copy link
Member

cc @djc , we were talking about this during the impl days at rustfest

@djc
Copy link
Contributor

djc commented Oct 5, 2017

So the proposal we came up with is that features and dependencies will live in different namespaces. The idea we had is that the dependency activation of dependencies by features will have to be explicit, with syntax something like this:

[features]
foo = ["bar", "crate:baz", "baz/spock"]

(Since the slash notation has no application for features, the part before the slash can safely be interpreted as a dependency; also note that baz/spock does not mean that the baz dependency is activated, just that the spock feature for it will be enabled if the dependency is activated.)

In our plan, this would be turned on by a configuration boolean in the package section, which I've tentatively called namespaced-features. This means that the old mode will be default until we decide otherwise in a future epoch (that is, the feature switch will become part of an epoch value.)

I'm still wondering if we can have a shortcut by which an optional dependency would result in implicitly adding a feature of the same name as long as a feature of that name does not exist. Because for the simple case of a feature that simply switches on a single dependency seems like annoying boilerplate:

[features]
foo = ["crate:foo"]

Not sure if we have good reason to disable that shortcut? Remember, as soon as you explicitly define the foo feature, the dependency has to also be activated explicitly by that feature, so the namespaces are still conceptually separate.

I am currently working on a bunch of refactoring to clarify the code in the resolver, in a way that should make it possible to abstract over the differences between the old and new models, I hope I can post an initial draft in the next few days.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Oct 5, 2017

I wish I could do:

[features.foo.dependencies]
foo = { ... }
bar = { ... }
# ...

skipping the indirection of the separately-declared optional dependency. That would be a fine replacement for concision lost if we get rid of the shortcut @djc mentions.

Even better, allow features boolean logic like https://github.com/tbu-/rust-rfcs/blob/master/text/1361-cargo-cfg-dependencies.md if a dependency is needed by multiple features.

@alexcrichton
Copy link
Member

@djc it's an interesting idea yeah of maybe not requiring an explicit opt-in here, being able to "shadow" the name of an optional dependency with the name of a feature sounds dangerously plausible to me, so long as we require that the feature doing the shadowing actually does enable the optional dependency!

@briansmith
Copy link

briansmith commented Oct 5, 2017

I like that idea. e.g. An "openssl" feature must enable the "openssl" dependency and MAY also enable other dependencies.

Consider the case where the crate a is relying on the implicit feature (e.g. openssl) to enable the optional dependency. Then the crate changes to add a dependency on another crate b that has an openssl feature. Then a needs to change its implicit openssl feature to an explicit 'openssl' feature that enables ["openssl", "b/openssl"]. I literally just experienced this when trying to refactor Trust-DNS; we will probably have to break backward compatibility since this isn't possible today.

@alexcrichton
Copy link
Member

@briansmith yeah, that's a great use case to fix as well! I think @djc is working on implementing a fix for this issue, so we should hopefully have this fixed in the near future!

@djc
Copy link
Contributor

djc commented Oct 6, 2017

Right, so: if a feature a does not exist but the crate has an optional dependency a, a feature will be defined implicitly as a = ["crate:a"]. But also, for any feature x where a dependency x also exists, (a) x must be an optional dependency, and (b) the feature x must include crate:x.

bors added a commit that referenced this issue Oct 31, 2017
Introduce Requirements struct to clarify code

`cargo::core::resolver::build_requirements()` previously, in this order:

* Defined local variables to track state
* Called a function to mutate the local variables
* Defined the aforementioned function
* Returned two out of three local variables as a tuple

This PR changes the code so that this is a recast as a struct (appropriately called `Requirements`), which is mutated in a more fine-grained way by its methods and acts as the return type for `build_requirements()`. To me, this seems a lot easier to understand.

This work was done in the context of #1286, but I figured it was easier to start landing some of the refactoring to avoid bitrot and get early (well, earlier) feedback.
bors added a commit that referenced this issue Mar 29, 2018
Split resolver module to make it more manageable

The `core::resolver` module is currently the largest in Cargo, at some 2000 lines. Here's an attempt at splitting it into more reasonable parts and reordering the code in it for better comprehensibility.

Splitting is done in three major steps:

* Move the `Resolve` type and its dependencies into a separate module (~220 lines)
* Move utility data types into a separate module (~400 lines)
* Move the `Context` type and its dependencies into a separate module (~400 lines)

This halves the size of the root module, which is then reordered to make it more readable.

(This is a yak-shaving expedition of sorts, in preparation for #1286.)
bors added a commit that referenced this issue Mar 30, 2018
Split resolver module to make it more manageable

The `core::resolver` module is currently the largest in Cargo, at some 2000 lines. Here's an attempt at splitting it into more reasonable parts and reordering the code in it for better comprehensibility.

Splitting is done in three major steps:

* Move the `Resolve` type and its dependencies into a separate module (~220 lines)
* Move utility data types into a separate module (~400 lines)
* Move the `Context` type and its dependencies into a separate module (~400 lines)

This halves the size of the root module, which is then reordered to make it more readable.

(This is a yak-shaving expedition of sorts, in preparation for #1286.)
bors added a commit that referenced this issue Apr 4, 2018
Introduce FeatureValue type to represent features table values

This is the next step towards #1286 (after #5258). The goal here is to have a central place in the code where feature strings are interpreted as (a) a feature, (b) a dependency or (c) a dependency/feature combo, and anchor that interpretation in the type system as an enum.

I've spent quite a bit of effort avoiding extra string allocations, complicating the code a bit; notice in particular the use of `Cow<str>` in `FeatureValue` variants, and the slight workaround in `Context::resolve_features()` and `build_requirements()`. I hope this is all okay.

cc @Eh2406
bors added a commit that referenced this issue Apr 30, 2018
Introduction of namespaced features (see #1286)

I think this basically covers all of the plans from #1286, although it still needs a bunch of tests and documentation updates. Submitting this PR to get some early feedback on the direction.
@bors bors closed this as completed in cb533ae Apr 30, 2018
@djc
Copy link
Contributor

djc commented Jun 27, 2018

This is the tracking issue for stabilizing this feature:

#5565

@ExpHP
Copy link
Contributor

ExpHP commented Aug 10, 2018

It took me a while to figure out how this proposal actually solves the problem it's trying to address.

Here's what I gather that the final TOML for the uuid example should look like. It does indeed allow you to have a --feature uuid which transitively toggles the dependency on postgres.

(fomat in proposal in OP)

[dependencies]
postgres = "0.6"
uuid = { version = "0.1", optional = true }

# (now that implicit features are gone, you can explicitly define a "uuid" feature)
[features.uuid]
features = ["postgres/uuid"]
dependencies = ["uuid"]

(fomat in more recent discussion; I suppose this is what was implemented?)

[dependencies]
postgres = "0.6"
uuid = { version = "0.1", optional = true }

# (now that implicit features are gone, you can explicitly define a "uuid" feature)
[features]
uuid = ["crate:uuid", "postgres/uuid"]

@djc
Copy link
Contributor

djc commented Aug 11, 2018

You're right on the money about what was implemented. Is there something I should add to the namespaced-features documentation in the Cargo book that would make this more accessible?

@ExpHP
Copy link
Contributor

ExpHP commented Aug 11, 2018

Exemplary use cases (mainly TOML snippets) of things not possible without the feature, like:

  • Transitively enabling optional dependencies (the uuid problem). This is the big one!
  • An [[example]] or [[test]] which e.g. happens to need an optional dependency for reasons entirely unrelated to the feature. (e.g. it needs to generate random numbers, but it does not need the impls/methods added by the crate's rand feature) Such an entry could have required-features = ["crate:rand"].
    • (that said, this is easily approximated with required-features = ["rand"] so this example is probably not worth the real-estate)
  • Is there a real-world use-case for a feature foo named after an optional dependency foo, but which does not actually activate "crate:foo"? If so I'd really like to see it!

Lencerf added a commit to Lencerf/lumi that referenced this issue Apr 3, 2021
[C-SERDE](https://rust-lang.github.io/api-guidelines/interoperability.html#c-serde)

However, crate `serde` is still compiled by cargo even if we compile lumi
without enabling feature `serde`. This is similar to
[#1286](rust-lang/cargo#1286). Let us wait
until `namespaced-features` is stable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-features Area: features — conditional compilation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants