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

Managing the propagation of features across a large workspace is infeasible without tooling support #9094

Open
brson opened this issue Jan 22, 2021 · 7 comments
Labels
A-features Area: features — conditional compilation C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage.

Comments

@brson
Copy link
Contributor

brson commented Jan 22, 2021

Describe the problem you are trying to solve

TiKV is a big workspace.

It has several features that are propagated throughout most of the workspace.

The "pattern" for maintaining these features in each crate is identical,
but humans are unable to maintain the pattern consistently across the workspace.

As a result, the build for any individual crate, under any particular combination of features,
has a good chance of being broken.

This is much exacerbated in the v2 resolver,
which TiKV uses,
because cargo now does feature resolution only based on the crates passed to -p,
not based on the default binary (or whatever it did before - I don't know exactly).
So the build of multiple crates is usually broken under various combinations of features
even though the main binary or --all may build correctly under those feature combinations.

Maintaining a working build for each crate under its various feature combinations
is basically impossible without some kind of custom test scripting.
The script I have put together to maintain correct builds for TiKV's crates takes hours to run,
so are infeasible to put under CI.

Here is an example of the pattern we are using to maintain two mutually-exclusive protobuf backends:

[features]
default = ["protobuf-codec", ...]
protobuf-codec = [
  "batch-system/protobuf-codec",
  ...
]
prost-codec = [
  "batch-system/prost-codec",
  ...
]

[dependencies]
batch-system = { path = "../batch-system", default-features = false }

Every crate has a default protobuf backend so that the crate compiles by default with -p under the v2 resolver.
Each of those crates default features must be turned off by the crates that depend on them,
and re-activated by that crate's default features.
The list of crates that have protobuf features is huge, and each crate needs to follow this same pattern perfectly -
any mistakes and some crate in the workspace will stop compiling under some feature combination.

Describe the solution you'd like

Not sure!

My first thought was a static analysis that could understand this pattern across the workspace and tell me I've made a mistake. After reading https://github.com/rust-lang/rfcs/blob/master/text/2906-cargo-workspace-deduplicate.md via #6921 though I'm also wondering if deduplicating feature metadata in some way could help.

Notes

Here's the script I'm using to brute-force test every crate/feature combo:

https://github.com/tikv/tikv/blob/master/scripts/check-build-opts.py

cc @nrc

@brson brson added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Jan 22, 2021
@ehuss
Copy link
Contributor

ehuss commented Feb 13, 2021

Hi! I understand that it can be difficult to manage features in a large workspace. However, this issue is a little unfocused, so it may be difficult for it to have any traction. Perhaps a discussion on one of the forums might be more well suited?

feature resolution only based on the crates passed to -p

Unless I'm misunderstanding what you are saying, this is how Cargo has always worked. The features for a workspace are resolved for a particular execution of Cargo, and ignores packages not selected. The primary issue for tracking this is #4463. One workaround is something like rustc-workspace-hack, which I assume you and Nick are familiar with.

Mutually exclusive features are tracked in #2980. They are an inherently tricky problem, and I usually recommend avoiding them if at all possible. (I realize that is not always possible, but finding creative solutions to avoiding that can help.)

The only tooling I'm aware of for analyzing Cargo workspaces (and features) is cargo guppy which Facebook uses for things like Diem. As part of the v2 work, I also made a bunch of changes to cargo tree for inspecting feature usage. If you want to create more tooling, I think that would be great, though probably best developed as a separate tool for now.

Otherwise, if you have more specific ideas for something not already in the issue tracker here, I'd suggest trying to keep the feature request focused and with more concrete problem statements and suggestions.

@apopiak
Copy link

apopiak commented Sep 14, 2021

We have similar issues in Substrate(-based projects) and I have a particular idea that might fit here (without knowing the design of cargo well):
Features that propagate by default. So instead of having to redefine and pass e.g. protobuf-codec through all the dependencies cargo will just build all dependencies of a crate where a "propagating feature" is selected.

@epage
Copy link
Contributor

epage commented Apr 8, 2022

What if we exposed dependency features for conditional compilation regardless of which crate enabled the feature, like #[cfg(feature = "serde/std")]? It seems like that could reduce some aspects of feature propagation. You could then also take advantage of this to have global features by having a global_features crate to centrally define all features. All crates could then depend on that and do #[cfg(feature = "global_features/std")].

See also

@ia0
Copy link

ia0 commented Nov 15, 2022

It seems to me that we don't want to go with something automatic:

On the contrary, we want something explicit at the application level:

Here's an imperfect (see drawbacks below) attempt at this:

  • Introduce a notion of global features.
  • Contrary to (regular) features which are scoped at the crate level, global features are scoped at cargo invocation level.
  • A consequence of scoping is that global features live in a different namespace than features. In particular, you can have a serde global feature (bound at the cargo invocation) as well as (possibly many) serde (local) features (bound to each crate in the dependency graph that defines that feature).
  • Global features can only be set at cargo invocation, either on the command line with --global-features=std,serde or in .cargo/config.toml1. They are disabled by default and can only be enabled.
  • Global features could be encoded with --cfg 'global-feature="std"' --cfg 'global-feature="serde"' as for regular features. But that's probably not needed, since they only impact cargo feature resolution.
  • Global features can enable regular features (and only regular features since global features can't be set outside of the cargo invocation) in Cargo.toml in the [global-features] section, similarly to regular features.

Here's how a library crate could look like:

[dependencies]
serde = { version = "1.0.147", default-features = false, optional = true }

[features]
std = ["serde?/std"]
serde = ["dep:serde", "serde/derive"]

[global-features]
std = ["std"]
serde = ["serde"]

If something like #4956 ends up being merged, then we could have a shortcut to map a regular feature to the homonymous global feature and avoid the [global-features] section.

[features]
std = { dependencies = ["serde?/std"], global = true }
serde = { dependencies = ["dep:serde", "serde/derive"], global = true }

And if the feature only enables an optional dependency, then we could also rely on the implicit feature of optional dependencies and avoid the [features] section:

[dependencies]
# Note that it doesn't make sense to have `global = true` without `optional = true`.
# So maybe the latter could be implicit and omitted.
serde = { version = "1.0.147", features = ["derive"], optional = true, global = true }

This solution has some drawbacks though:

  • As with anything global, how are naming conflicts supposed to be handled? Should we only allow global-features to be named according to some crate name in the dependency graph?
  • How to deal with defaults? Most users probably want to have --global-features=std.
  • How do global features interact with default features? (e.g. std) It seems that we always want default-features = false and selectively add features (either locally or globally).2
  • If cargo is ever able to understand arbitrary (i.e. not just target-related) --cfg to control features (i.e. not only dependencies), then this would subsume the notion of global features. However, global features would enforce some naming convention over --cfg usage by using the global-feature="std" format.

Footnotes

  1. I think using Cargo.toml would be confusing to library authors. They might think they can force global features at crate level.

  2. I was never really convinced by default features. They seem to add more complication (there's this special feature that is enabled by default) than simplification (avoid users having to explicitly add it). The default feature could be a feature like any other and most users would just enable it (cargo add could select it by default so it's not even hurting velocity).

@epage epage added the A-features Area: features — conditional compilation label May 24, 2023
@epage
Copy link
Contributor

epage commented Sep 25, 2023

I've created a Pre-RFC for mutually-exclusive, global features which might be pertinent to the discussion.

@kriswuollett
Copy link

I'd agree with discussion notes above about handling global propagating features would be difficult. I'd never expect that to be ever solved in a reasonable timeframe.

But the issue title, currently, is propagation across a large workspace. Could the primary use case be solved by specifying the propagation logic in the workspace file, i.e., more or less a syntactic sugar? My use case is propagating a feature like with-prost to all my workspace members. Since I own all the Cargo.toml files in my workspace I should be able to remove the with-prost propagation in the following package:

[features]
fizzbuzz = ["dep:fizzbuzz"]
with-prost = [
    "my-api/with-prost",
    "fizzbuzz?/with-prost",
    "dep:prost",
    "dep:prost-types",
]

[dependencies]
fizzbuzz = { workspace = true, optional = true }
my-api = { workspace = true }
prost = { workspace = true }
prost-types = { workspace = true }

with:

[features]
fizzbuzz = ["dep:fizzbuzz"]
with-prost = [
    "dep:prost",
    "dep:prost-types",
]

[dependencies]
fizzbuzz = { workspace = true, optional = true}
my-api = { workspace = true }
prost = { workspace = true }
prost-types = { workspace = true }

given my workspace file has something like:

feature-aspects = [
    "with-prost",
]

The above just means flip on a feature named with-prost, and define it if necessary, in any workspace member, and set that feature on any workspace member dependencies if the dependent package has it enabled.

@dflemstr
Copy link

I created a cargo plugin prototype that offers this functionality: https://github.com/dflemstr/cargo-feature-aspect

I'm not super happy with the implementation or the command-line API/UI so any feedback is much appreciated!

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 C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

No branches or pull requests

7 participants