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

Cargo target features #3374

Closed
wants to merge 2 commits into from
Closed

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jan 20, 2023

This adds a new enable-features field to Cargo Targets which forces a feature to be enabled if the target is being built.

Rendered

@ehuss ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Jan 20, 2023
Comment on lines +150 to +155
* Developers can organize their project in a [Cargo Workspace](https://doc.rust-lang.org/cargo/reference/workspaces.html) instead of using multiple targets within a single package.
This allows customizing dependencies and other settings within each package.
Workspaces can add some more overhead for managing multiple packages, but offer a lot more flexibility.
Instead of implementing this RFC, we could put more work into making workspaces easier to use, which could benefit a larger audience.
However, it is not clear if it is feasible to make workspaces work as effortlessly compared to targets.
Also, there is likely more work involved to get workspaces on the same level.

Choose a reason for hiding this comment

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

If I am not mistaken, this works for binaries, but not for examples.
If you extract an example out of a package and turn it into its own stand-alone package within the same workspace you "lose" the fact that it's an example for that library/binary.

This gap is indeed an issue of its own. It leads projects with complex examples (especially ones where the Cargo.toml itself is an important part of the example) to move away from the example feature, so to speak.
If this gap were to be filled, I'd strongly argue that this alternative workflow would be my preference over adding more complexity to Cargo.toml.
It would make it easier to direct people towards workspaces or standalone packages based on their needs: start with a standalone package, if you need X you must upgrade to a workspace, which is easily done using magical cargo command to convert into a workspace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you say more about what you feel like you lose when the distinction of an "example" is lost? I understand that instead of running cargo run --example foo you have to do cargo run -p foo, but are there other differences? Is that mostly a perception issue?

Choose a reason for hiding this comment

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

There is definitely a perception element.
The other difference is when they get compiled - a cargo test invocation will also build examples, which is quite convenient to make sure they don't rot.

Copy link

Choose a reason for hiding this comment

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

Also - as far as I'm aware - installing a crate with multiple [[bin]]s installs all of those bins, whereas examples are not built nor installed, and thus are purely for development/documentation/testing purposes.

Comment on lines +137 to +138
* Instead of adding a separate field that lists features, a `force-enable-features = true` field could be added to change the behavior of `required-features` to have the behavior explained in this RFC.
That might be less confusing, but would prevent the ability to have both behaviors at the same time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you speak to when it is desirable to have both enable-features and required-features at the same time? (I'm assuming force-enable-features could be set per-target).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would expect it to be rare. A possible example would be:

[[example]]
name = "db"
enable-features = ["dep:clap"]
required-features = ["sql"]

Here the example would only be built if -F sql were specified. This could be done if you don't want a heavy dependency (like a database dependency) to always be built when running cargo test. The presence of clap isn't the primary focus of the example, so it is a hidden aspect of the example, and will get enabled automatically if the example is built.

I think a con for force-enable-features = true I forgot to list is that it would require entering two lines to get the behavior that I think most users will want.

Copy link

@Qix- Qix- Aug 7, 2023

Choose a reason for hiding this comment

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

I quite like the distinction between "enable this target if X features are enabled" in addition to "if this target is enabled, build it with Y additional features". If I'm understanding correctly, force-enable-features turns that into an either/or, which I think is more work for less utility overall. 👎 on force-enable-features if that's the case.

In the embedded world, this would be very useful to enable building variant targets of e.g. firmware based on chip class/manufacturer.

[[bin]]
name = "stm32f4"
enable-features = ["stm32f4"]
required-features = ["stm32"]

[[bin]]
name = "stm32f7"
enable-features = ["stm32f7"]
required-features = ["stm32"]

Which would build both the stm32f4 and stm32f7 variants of firmware just by using cargo build --features stm32, for example.

Copy link

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

Sorry for dropping the ball on #3020. Thanks for picking this up.

text/3374-cargo-target-features.md Outdated Show resolved Hide resolved
These additional features will be combined with any features selected on the command-line before calling the resolver.

Unfortunately this selection of targets is quite complex.
This may require some potentially invasive refactoring, as Cargo currently chooses the targets to select after resolution is done (including a separate phase for implicit binaries used for integration tests).

Choose a reason for hiding this comment

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

Quoting your question from #3020 (comment):

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.

How does the feature resolver solve this problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for bringing that up! I had forgotten about it, and have added it to the RFC. It should likely iterate until no new requirements can be satisfied.


[[bin]]
name = "myproject"
enable-features = ["dep:clap"]
Copy link

@pksunkara pksunkara Jan 21, 2023

Choose a reason for hiding this comment

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

In #3020, I have the following line:

For a target, specifying lib-features does not implicity activate the default features of the library dependency. If needed, the target can specify default in the list of values for lib-features.

And there were some concerns about that as described in #3020 (comment)

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.

The original motivation for having that line is to allow the library and binary to have separate dependencies. There are use cases where the user use the same cool name for a library and wants to have a cli tool with the same cool name but with different dependencies. (ex: cli tool only does project generation to use the library). In this scenario, when the end user runs cargo install cool, the unneeded dependencies present in default are also built.

To solve this issue, I would propose what I had been thinking last on #3020.

[[bin]]
name = "foo"
enable = { features = ["dep:clap"], no-default-features = true }

[[bin]]
name = "bar"
enable = ["dep:clap"] # shorthand

[dependencies]
clap = { version = "4.0.26", optional = true }
serde = { version = "1.0.152", optional = true }

[features]
default = ["dep:serde"]

If I were to run cargo install cool --bin foo, default features will not be activated:

 Compiling clap v4.0.26
Installing /bin/foo

If I were to run cargo install cool (installing both binaries), the default features will be activated because of bar target:

 Compiling clap v4.0.26
 Compiling serde v1.0.152
Installing /bin/foo
Installing /bin/bar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I have added it as a possible extension to consider.

Comment on lines +146 to +147
* Instead of using `enable-features`, developers can be diligent in passing the appropriate `--features` options on the command-line when building their projects, possibly using `required-features` to ensure they only get built in the correct scenarios.
The intent of this RFC is to make that process easier and more seamless.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the UX should be called out for this alternative.

atm commands like cargo check will report the needed features:

$ cargo run --example git-derive
error: target `git-derive` in package `clap` requires the features: `derive`
Consider enabling them by passing, e.g., `--features="derive"`

The gap is with cargo install which doesn't;

$ cargo install pulldown-cmark --no-default-features
    Updating crates.io index
  Installing pulldown-cmark v0.9.2
   Compiling version_check v0.9.4
   Compiling memchr v2.5.0
   Compiling pulldown-cmark v0.9.2
   Compiling bitflags v1.3.2
   Compiling unicase v2.6.0
    Finished release [optimized] target(s) in 1.86s
warning: none of the package's binaries are available for install using the selected features

I've created rust-lang/cargo#11617 for us to track this

Copy link

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

Everything else looks good.


* [cargo#1982](https://github.com/rust-lang/cargo/issues/1982) is the primary issue requesting the ability to set per-target dependencies, and contains some discussion of the desired use cases.
* Other names may be considered for the field `enable-features`, such as `forced-features`, `force-enable-features`, etc.
* A flag could be added to the target definition to indicate that default features should be disabled when the target is being built.

Choose a reason for hiding this comment

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

This is not exactly an alternative but an extension on top of the RFC.

I think this should be actually be inside this RFC itself, but if not I will create a quick follow up RFC since IMHO, there are good use cases for this.

The flag name could be:

disable-default-features = true

Comment on lines +51 to +53
When using `myproject` as a library dependency, by default the `networking` feature and `clap` will not be enabled.
When building the binary as in `cargo install myproject` or `cargo build`, the `clap` dependency will automatically be enabled allowing the binary to be built correctly.
Similarly, `cargo build --example fetch-http` will enable the `networking` feature so that the example can be built.
Copy link
Contributor

Choose a reason for hiding this comment

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

automatically enabled whenever the target is being built
...
When building the binary as in cargo install myproject or cargo build, the clap dependency will automatically be enabled allowing the binary to be built correctly.
...
Cargo will need to determine which features are being force-enabled by the targets selected on the command-line.
Unfortunately this selection of targets is quite complex. This may require some potentially invasive refactoring, as Cargo currently chooses the targets to select after resolution is done (including a separate phase for implicit binaries used for integration tests).

I feel like an important part of this RFC is non-obvious unless a person is very familiar with these terms and fully thinks through the implications of those terms.

Going into this RFC, my care about was

  • cargo run --example foo would activate the needed features so I could run it

However, this RFC also introduces

  • cargo build will build all default targets, activating their enable-features
  • cargo test will build and test all default targets, activating their enable-features
  • cargo check --all-targets will build and test all targets, activating their enable-features

This is in contrast to required-features which would have skipped targets.

My primary concern with this is it will make it impossible to check and test a project without something like cargo-hack. In nearly all of my projects, CI runs

  • cargo test --workspace --no-default features
  • cargo test --workspace
  • cargo test --workspace --all-features

The assumption is this covers enough feature combinations to feel confident that it will build. There are combinations of features that might break but the probability is low enough to not justify extra CI time.

With enable-features, these three test runs could become equivalent. The only way to verify in CI what I had originally intended would be to select the exact targets for the desired feature set which would be unmanageable without external tooling.

From watching cargo's issue tracker and other forums, I feel like feature unification is one of the biggest hurdles to features in cargo today and I feel that this design compounds that problem to being unmanageable.

Under the alternatives is listed:

Only the situation where required-features generates an error could be changed to implicitly enable the missing features. This would likely make required-features less annoying to work with, but doesn't help for use cases like running cargo test where you have specific tests or examples that you want to be automatically included (where required-features simply makes them silently excluded).

As I said, this is what I expected going into this RFC.

For cargo test or cargo build --examples, having it skips incompatible targets seems fine. When I want to indiscriminately enable all, that is what --all-features is for. Granted, it can be annoying when you have debug features. Maybe another alternative is a --required-features flag that activates all required features for the selected targets?

The one use case I can think of that wouldn't be served by this is cargo install pulldown-cmark (manifest). Since no target is selected, required-features wouldn't activate but then there wouldn't be anything to install. I suspect these are minor enough of cases that with improved error messages (#3374 (comment)), enabling of features instead of error, and --required-features, things would be sufficiently improved into a "good enough" state.

Choose a reason for hiding this comment

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

The one use case I can think of that wouldn't be served by this is cargo install pulldown-cmark (manifest). Since no target is selected, required-features wouldn't activate but then there wouldn't be anything to install.

What's stopping us from default enabling --required-features on install command?

Copy link
Contributor

Choose a reason for hiding this comment

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

I had considered including that but vaguely remember a conversation with concerns for a similar idea ("just make cargo install do the right thing").

One problem is there are two different workflows

  • Bin-only packages where required-features would only be used in truly optional bins
  • Primarily lib packages with optional bins at which point we'd need required-features for the most basic of bins.

This also makes cargo install behave differently than nearly any cargo command (cargo metadata is an exception) which makes it less predictable. In particular, this furthers the split cargo build and cargo installs behavior (handling of lockfiles being a point of contention).

That isn't to say I'm against it but that we'd need to do weigh it carefully and maybe find ways to help with these problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are very valid and strong concerns. Part of the drawbacks mentions --no-default-features, but I hadn't considered that just testing a library without other binaries enabling features you don't want could be problematic. My original thinking was that if that's what you want, cargo test --lib --no-default-features would be the command to run, but I understand how that's probably not realistic for most people to run.

--no-default-features could disable all enable-features clauses, though that seems worryingly complicated to explain, and fragile for testing.

For cargo test or cargo build --examples, having it skips incompatible targets seems fine.

IIUC, this is suggesting to use required-features instead? That doesn't seem to address the use cases that I have in mind. For example, if a package has a library and a binary, and the binary has command-line processing dependencies, and the integration tests need to test the binary. If those binaries used required-features, then the tests would fail (or possibly just not run). That's not the expectation I have for a bare cargo test which by default should test the majority of things in the package.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, this is suggesting to use required-features instead? That doesn't seem to address the use cases that I have in mind. For example, if a package has a library and a binary, and the binary has command-line processing dependencies, and the integration tests need to test the binary. If those binaries used required-features, then the tests would fail (or possibly just not run). That's not the expectation I have for a bare cargo test which by default should test the majority of things in the package.

This doesn't seem prohibitive. The tests could have cfgs for the relevant features needed for them to run, just like tests calling into parts of an API that are feature dependent. Alternatively, you could declare specific test crates to have required-features that match the bin. In place of individual features, you can define a cli feature (imo this is what should be done or else you are having people rely on implementation details). I don't think this is enough to justify this feature.

Copy link
Member

@Nemo157 Nemo157 Jun 22, 2023

Choose a reason for hiding this comment

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

That's not the expectation I have for a bare cargo test which by default should test the majority of things in the package.

I have not found this to be true up till now in feature-heavy crates. I've taken two approaches in the past: feature-gate tests appropriately and note in the readme that you need to use --features <stuff you touched>, or add an internal feature and compile_error!("please test with --all-features"); the tests if it's not active.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps my comment wasn't clear, but for me required-features isn't a solution. For example, if I ran cargo test in cargo's repo, and that didn't run the testsuite (because the binary requires clap), I would be very frustrated.

I have not found this to be true up till now in feature-heavy crates.

This isn't intended for just feature-heavy situations. The primary use case and motivation for this is target-specific dependencies, where you might not have any features.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And FWIW, it may be the solution is to instead lean more heavily on workspaces, and making them easier to use. That is what I heavily lean towards as an alternative to this, particularly since the amount of complexity ended up being far greater than I was expecting. I'm not sure what might be the solution there, but it would at least be helpful for all workspace users if workspaces were easier to use.

Copy link
Contributor

@epage epage Jun 22, 2023

Choose a reason for hiding this comment

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

This isn't intended for just feature-heavy situations. The primary use case and motivation for this is target-specific dependencies, where you might not have any features.

I think this is one of the problems with the RFC. It is taking a very specific use case / workflow (bin/lib crates existing in the same package) and making a general feature for it. For example, test, and bench targets will more likely run into this problem due to being in feature-heavy crates (like clap) for which this solution is a major step back yet is being advertised as solving that problem.

And FWIW, it may be the solution is to instead lean more heavily on workspaces, and making them easier to use. T

imo potential steps forward are:

Copy link
Contributor

@epage epage Sep 11, 2023

Choose a reason for hiding this comment

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

For the cargo-install use case, another solution is "bin dependencies" or dependencies that activate for bins (#2887).

@ehuss
Copy link
Contributor Author

ehuss commented Sep 11, 2023

I'm going to close this RFC for a few reasons. I think some of the behaviors around feature unification are going to result in some hazards that will make it difficult to use and cause problems that people would then need to figure out how to work around. The cargo test workflow in particular would be unable to test without the features enabled. I also agree that the indirect nature of using this mechanism to specify per-target dependencies is not obvious, so I worry about making things too confusing, along with being unable to isolate those dependencies between targets. It also doesn't help to have two similarly named fields, which can be confusing. I've also been hesitant to move forward with this since I started writing it due to the level of complexity in the implementation that appeared (I only started it because I thought it would have been relatively easy).

The Cargo team is still very interested in smoothing over the workflows and use cases here, but I think we'll need to evaluate different solutions. The "Alternatives" section of this RFC mentions a few, and #3374 (comment) includes several others, some of which are easier to do and potentially more useful to a wider audience.

@ehuss ehuss closed this Sep 11, 2023
@Qix-
Copy link

Qix- commented Sep 11, 2023

Is there an alternative RFC issue we could be tracking (subscribed to) now, by chance?

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
No open projects
Status: Postponed
Development

Successfully merging this pull request may close these issues.

7 participants