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

Allow specifying features of the implicit lib dependency #3020

Closed
wants to merge 1 commit into from
Closed

Allow specifying features of the implicit lib dependency #3020

wants to merge 1 commit into from

Conversation

pksunkara
Copy link

@pksunkara pksunkara commented Nov 15, 2020

This RFC proposes adding a way to specify features for the implicit library dependency for other non-library targets (bin/example/bench/test). This is intended to reduce the friction between developers and users.

Previous discussion
Previous related RFC

Rendered

@coolreader18
Copy link

How about if you could add default-features = false to the lib map, so you could have easy default functionality for a library while not bloating a binary target that doesn't need all the features?

@pksunkara
Copy link
Author

Yeah, I already described that in the RFC on this line.

@coolreader18
Copy link

Ah, gotcha, I missed that

@djc
Copy link
Contributor

djc commented Nov 15, 2020

The mechanics of this sound good to me, much more coherent than the current required-features design, and I prefer this design over the one proposed in #2887.

Bikeshedding: the lib name sticks out to me as not that obvious. Maybe expand to lib-features? I suppose anything including features would introduce stuttering with something like lib-features = { default-features = false, features = ["foo"] }, but just lib feels like it could mean any number of things in this context -- a bit too terse for my taste.

@kornelski
Copy link
Contributor

Are they really lib (only) features?

If you have lib = { features = ["foo"] }, does #[cfg(feature = "foo")] not work in the bin/test targets?

Currently an enabled feature is enabled for all build products. I'd expect this to also enable features "globally" for the whole package. In that case the name of the property could be more generic than lib.

@kornelski
Copy link
Contributor

kornelski commented Nov 15, 2020

I get that this RFC tries to be consistent with how Cargo selects features in dependencies, but I'm not a fan of Cargo's current behavior around implied default-features. It's too easy to accidentally enable default features.

If there's:

[[bin]]
lib = { features = ["yaml"] } # implied default-features=true

does that mean that

cargo run --no-default-features

has default features enabled, as implied by the bin target?

What's the use-case for disabling default features? If there isn't a strong case for them, maybe this problem could be dodged by making target-specific features purely additive, e.g.

[[bin]]
enable-features = ["yaml"]

or

[[bin]]
required-features = ["yaml"]
enable-required-features = true

@pksunkara
Copy link
Author

pksunkara commented Nov 15, 2020

Regarding the key name not being lib, I am open to suggestions.


Regarding the need for default-features, as commented by another person:

so you could have easy default functionality for a library while not bloating a binary target that doesn't need all the features

I had this need in several example, test and bench targets too. For example, if I am building a library for mostly normal users but also want to add no_std support, I would add a feature called std and add it to the default feature list.

Now, without the default-features support here, to run the test/bench for this no_std support, I would need to run several hand written cargo commands.


Regarding the cargo run --no-default-features with default-features = true, We could do it two ways:

  1. We can say lib value always takes precedence and thus the default features will get activated
  2. We can say command line args for run takes precedence and thus the default features wont be activated

I would prefer 1 because the second case doesn't feel as ergonomic to me as a library author.

@kornelski
Copy link
Contributor

kornelski commented Nov 15, 2020

Maybe this could be approached from another angle, by letting each target replace the default features list? This replacement would work for adding as well as removing features.

[[bin]]
features = { default = ["cli", "yaml"] } # or
default-features = ["cli", "yaml"] # or
replace-default-features = ["cli", "yaml"] 

@pksunkara
Copy link
Author

pksunkara commented Nov 15, 2020

IMHO what you are suggesting is adding complexity to the ergonomics while still not solving the original problem you posed regarding the default features.

It deviates away from the syntax cargo users are already used to while defining the features for dependencies.

One of the other reasons for this RFC is to also provide users with a small impression and reminder that every target depends on the library as an implicit dependency.

@Nemo157
Copy link
Member

Nemo157 commented Nov 15, 2020

Replacing doesn't really feel right when building multiple targets, to pull from a real usecase I have:

[[test]]
name = "brotli"
default-features = ["brotli"]

[[test]]
name = "bzip2"
default-features = ["bzip2"]
> cargo test --test bzip2 --test brotli --features tokio

This would need the full set of brotli, bzip2, tokio active to test properly.

@Nemo157
Copy link
Member

Nemo157 commented Nov 15, 2020

Personally, to resolve any default features issues I would just say that these sort of "internal dependencies" do not activate default features implicitly the way an external dependency does. If a binary wants the default features active it can just add default (or the actual features behind it) as one of the features it forces active. That would allow simplifying the syntax to not need a nested table, without losing any power/familiarity for users.

@Aloso
Copy link

Aloso commented Nov 20, 2020

I upvoted because I agree that the current situation is suboptimal. However, I'm not convinced that the proposed solution is ideal.

One question the RFC doesn't answer: How does the lib key interact with features provided manually? For example:

[features]
default = ["a", "b"]
c = []
d = []

[[bin]]
# default binary
lib = { features = ["c"] }

Execution:

cargo run --features a,d --no-default-features

I assume that in this case the features a, c and d are activated: c cannot be disabled, but d can be optionally enabled.

I agree with @kornelski that the syntax could be simpler. My proposal:

[[bin]]
# this adds a feature that cannot be disabled
active-features = ["foo"]

# this disables or overrides the default features
# (equivalent to the `--no-default-features` cargo option)
default-features = []

Note that you can extend the default features with

default-features = ["default", "another", "feature"]

@ehuss ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Dec 4, 2020
@pksunkara
Copy link
Author

pksunkara commented Apr 4, 2021

Thanks to everyone for discussing the proposal and shining a light on the rough edges. I have updated the RFC now to resolve these edge cases satisfactorily.

I went with the modification suggested in #3020 (comment) which seems to handle all the concerns raised in the above discussions.

Would appreciate you guys reviewing again 😄


I am open to bikeshedding on the lib-features name. What about enable-features?

@fogti
Copy link

fogti commented Apr 4, 2021

I currently do not really understand why there is a need to introduce a whole new table key... Wouldn't it be easier to (alternative 1) just introduce an edition change which changes the behavoir of required-features from "default is implicit" to "default must be specified in required-features is needed"?
or (alternative 2) introduce a key default-features at the same level of the proposed lib key, which, with value false, deactivates the implicit default feature, similiar to external dependencies.

Both alternatives could also be introduced simultaneously (e.g. (2) for older editions, (1) for newer editions).

@pksunkara
Copy link
Author

This line discusses the reasons against that.

@fogti
Copy link

fogti commented Apr 4, 2021

I would like to propose just changing the behavoir of required-features from "fail if feature isn't enabled, but target is requested" to "enable necessary features automatically"

@pksunkara
Copy link
Author

Again, repeating this from the RFC, required-features current behaviour should not be changed for 2 reasons:

  1. Backward compatibility
  2. There are legitimate use cases where it is useful for skipping targets.

@fogti
Copy link

fogti commented Apr 4, 2021

for 2.: are there use cases which require both the ability to skip targets and to automatically enable necessary features when the target is requested? this could be resolved by introducing a command line flag. if that is not required, that switch could be specified per-target in Cargo.toml. Both could also be applied simultaneously, with the command line flag taking precedence over Cargo.toml.

Thus, proposal: introduce enable-features (open for bike-shedding) at the same level as lib from above. If enable-features = false, retain the current behavoir. If enable-features = true, automatically enable necessary features to compile the target (e.g. not skipping it). The flag should be overridable via the cargo command line, with both true and false, e.g. --enable-features=true, --enable-features=false. This would be backward compatible and IMO less invasive.

@pksunkara
Copy link
Author

pksunkara commented Apr 4, 2021

That was my initial idea actually as described in this line. The issue with that would be the need to make required-features not have default feature implicitly set by default which would in turn make this whole thing backward incompatible.

Which is why I proposed lib-features and then we can later on remove the need for required-features by adding the flag as you described (but for reverse) and then deprecate required-features.

@fogti
Copy link

fogti commented Apr 4, 2021

I would try to encapsulate that change (e.g. not have default feature implicitly set) as edition change...

@pksunkara
Copy link
Author

We would want it to be a new key to allow for testing. If it is widely liked and used, we can make it opt out in the next edition that rolls around.

This has been how cargo is being developed right now (ex: resolver=2 change). We don't want to break things just because everyone agrees on an RFC.

@pksunkara
Copy link
Author

Are you reading an old RFC? The updated one doesn't contain the subtable anymore.

@fogti
Copy link

fogti commented Apr 5, 2021

ah ok, the rendered link above is outdated.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks for working on the RFC!

I think renaming the key to enable-features sounds good to me, and might help avoid some confusion. There is some concern that the difference between enable-features and required-features might be confusing, similar to how systemd's requires vs wants can be confusing. However, I don't think there is much of an alternative, and we should just try to do our best to pick a name that is as clear as we can make it (without being to verbose).

Assuming I understand this as automatically enabling the features of the package, this might cause some complications with the interaction of required-features. That might require an iterative algorithm since enabling some features might unlock a target's required-features, which could in turn enable more features. Might be good to point out or explain that interaction.

The implication of essentially disabling default features by default is interesting, but I think potentially confusing. Like, if a project has default features, and they add a [[bin]] with enable-features, then suddenly the default features don't work? Or maybe I'm misunderstanding how that is intended to work. Can you explain more how that would work with default target selection (like a bare cargo test) and in general what it means?

Sorry about the long delays, I'll try to respond to this RFC more quickly.

# Summary
[summary]: #summary

Allow specifying features of the implicit lib dependency that need to be enabled by default on non-lib targets (`[[bin]]`, `[[example]]`, etc) in a single crate.
Copy link
Contributor

Choose a reason for hiding this comment

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

This statement is a little confusing to me. This isn't enabling features for the lib exactly, is it? For example, if you have a clap feature, it needs to also be enabled for the binary, too, right?

In general, I would probably like to see this framed as just changing the set of features enabled on a package. Cargo's features work at the resolution of packages, and changing that might be quite difficult.

Or maybe I am not understanding what the RFC is proposing. I am assuming that the features for the package would still be the union of the features enabled by the selected targets. I'm not sure I understand how this intends target selection to interact with the enable set of features.

Copy link
Author

Choose a reason for hiding this comment

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

I am assuming that the features for the package would still be the union of the features enabled by the selected targets.

Yes.

But the statement talks about enabling features on non-lib targets. I am not sure what exactly caused the confusion here. Do you think you can explain a bit more on what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my confusion boils down to, I don't understand why this RFC is talking about the implicit "lib" dependency at all.

The phrase "specifying features of the implicit lib dependency" implies to me that this is specifying the features of lib.rs. But it seems to be the opposite of that; that it is specifying the features of the binary/example targets? Or is it specifying the features for the whole package?

For example, let's say there is a package with 3 binaries and no lib.rs, what would specifying lib-features do? Why does it have the word "lib" in it, if a package doesn't even have a library?

The only thing that I see that is special about "lib" is that it is required in all circumstances, so setting features on the [lib] table doesn't make sense (which is also the case with required-features).

@pksunkara
Copy link
Author

pksunkara commented Jun 16, 2021

That might require an iterative algorithm since enabling some features might unlock a target's required-features, which could in turn enable more features

That would not enable more features, because required-features doesn't automatically enable features. It makes sure the target is skipped if all the features specified in there aren't activated.

and they add a [[bin]] with enable-features, then suddenly the default features don't work

The default features won't work if and only if that binary target is being compiled.

@ehuss
Copy link
Contributor

ehuss commented Jun 16, 2021

I think the interaction with required-features could use some clarification in this scenario:

[[bin]]
name = "foo"
lib-features = ["a"]

[[bin]]
name = "bar"
required-features = ["a"]
lib-features = ["b"]

[[bin]]
name = "baz"
required-features = ["b"]

Here, building with no feature flags, I would assume the resolver would see foo enables a. Once a is enabled, that means bar's required-features are now satisfied, so it can be built. That in turn enables b which in turn means baz's required-features are now satisfied, so it can be built. That is what I mean by an iterative approach.

If it doesn't work like that, then there is some alternative that needs to be clarified. I'd be concerned if features get enabled, but they don't allow required-features to be satisfied, since that could be confusing. Although it is not really clear if required-features will be used much, so it might not be too important, it is good to clarify how this works.


The default features won't work if and only if that binary target is being compiled.

Right, and I'm saying that could be confusing. For example, I have a project with a library, and it has default-features, and I've been happily using it for a while. Then, I decide I want a binary that enables clap when it is built, so I add:

[[bin]]
name = "my-bin"
lib-features = ["clap"]

Now, suddenly, whenever I type cargo build or cargo test, none of the default features are enabled, and everything breaks. I think that would be a confusing and frustrating situation.

@ehuss
Copy link
Contributor

ehuss commented Oct 8, 2021

@pksunkara I just wanted to check in to see if you had any questions on this, and if you're interested in continuing with it.

@pksunkara
Copy link
Author

Definitely interested in continuing. I might pop up in zulip and ask you a few questions when I start focusing on this PR.

@pksunkara
Copy link
Author

Superseded by #3374

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.

8 participants