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: Give users control over feature unification #3692

Merged
merged 37 commits into from
Nov 2, 2024
Merged
Changes from 29 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
97d49c2
Skeleton
epage Sep 11, 2024
c3b17e8
Initial
epage Sep 11, 2024
a921e09
Set RFC number
epage Sep 11, 2024
7d6810d
Fix typo
epage Sep 12, 2024
5b13f93
Fix typo
epage Sep 12, 2024
750b5f1
Fix typo
epage Sep 12, 2024
4ebf0ff
Remove boilerplate
epage Sep 12, 2024
703a636
Remove copy/pasted header
epage Sep 12, 2024
e7a4f27
Move mutually exclusive features to a Drawback
epage Sep 12, 2024
f2bb64a
Cover no_std with std features being unified
epage Sep 12, 2024
c5bbeee
Fix typo
epage Sep 12, 2024
65e6988
Note cargo-hakari's build/host unification features
epage Sep 12, 2024
e752830
Add build/host unification qualification
epage Sep 12, 2024
0cebf5f
Fix typo
epage Sep 12, 2024
363be82
Add citation
epage Sep 12, 2024
6860e51
Note ecosystem importance of build/host unification
epage Sep 12, 2024
8b114b3
Fix typos
epage Sep 12, 2024
4bbdc51
fix: Clarify some points
epage Sep 17, 2024
8f848a4
Expand on future possibilities
epage Sep 17, 2024
c2a4ac1
Expand on dev/normal unification
epage Sep 17, 2024
d35c626
fix: Typo
epage Sep 19, 2024
760b618
fix: Clarify statement
epage Sep 19, 2024
cf07a12
fix: Whatever
epage Sep 19, 2024
51839e5
fix: Clarify statements
epage Sep 19, 2024
6318f47
fix: Clarify statement
epage Sep 19, 2024
cd2af9a
fix: Typi
epage Sep 19, 2024
1b2a3d2
fix: Clarify statement
epage Sep 19, 2024
eb8ffb2
fix: Typo
epage Sep 19, 2024
ff5e82a
fix: Link out to another related Issue
epage Sep 20, 2024
0aac9ab
fix: Remove issue link
epage Sep 30, 2024
c0158e7
fix: Disambiguate each use of 'target'
epage Oct 1, 2024
969cb03
Make cargo-install interaction explicit
epage Oct 2, 2024
a071fe4
alt: Build X as if its a dependency of Y
epage Oct 3, 2024
0e1fbc4
fix: Clarify summary
epage Oct 3, 2024
e3fa072
fix: Clarify this RFC offers no new concepts
epage Oct 3, 2024
f2075e1
fix(drawbacks): Put more emphasis on relationship with --workspace, m…
epage Oct 28, 2024
203e6fc
Add tracking issue
ehuss Nov 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
194 changes: 194 additions & 0 deletions text/3692-feature-unification.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
- Feature Name: `feature-unification`
- Start Date: 2024-09-11
- RFC PR: [rust-lang/rfcs#3692](https://github.com/rust-lang/rfcs/pull/3692)
- Rust Issue: [rust-lang/rust#3692](https://github.com/rust-lang/rust/issues/3692)
epage marked this conversation as resolved.
Show resolved Hide resolved

# Summary
[summary]: #summary

Allow users to control feature unification.

Related issues:
- [#5210: Resolve feature and optional dependencies for workspace as a whole](https://github.com/rust-lang/cargo/issues/5210)
- [#4463: Feature selection in workspace depends on the set of packages compiled](https://github.com/rust-lang/cargo/issues/4463)
- [#8157: --bin B resolves features differently than -p B in a workspace](https://github.com/rust-lang/cargo/issues/8157)
- [#13844: The cargo build --bins re-builds binaries again after cargo build --all-targets](https://github.com/rust-lang/cargo/issues/13844)

# Motivation
[motivation]: #motivation

Today, when Cargo is building, features in dependencies are enabled based on the set of packages selected to build.
This is an attempt to balance
- Build speed: we should reuse builds between packages within the same invocation
- Ability to verify features for a given package

This isn't always ideal.

If a user is building an application, they may be jumping around the application's components which are packages within the workspace.
The final artifact is the same but Cargo will select different features depending on which package they are currently building,
causing build churn for the same set of dependencies that, in the end, will only be used with the same set of features.
The "cargo-workspace-hack" is a pattern that has existed for years
(e.g. [`rustc-workspace-hack`](https://crates.io/crates/rustc-workspace-hack))
where users have all workspace members that depend on a generated package that depends on direct-dependencies in the workspace along with their features.
Tools like [`cargo-hakari`](https://crates.io/crates/cargo-hakari) automate this process.
To allow others to pull in a package depending on a workspace-hack package as a git dependency, you then need to publish the workspace-hack as an empty package with no dependencies
and then locally patch in the real instance of it.

This also makes testing of features more difficult because a user can't just run `cargo check --workspace` to verify that the correct set of features are enabled.
This has led to the rise of tools like [cargo-hack](https://crates.io/crates/cargo-hack) which de-unify packages.

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation


# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

We'll add two new modes to feature unification:

**Unify features across the workspace, independent of the selected packages**

This would be built-in support for "cargo-workspace-hack".

This would require effectively changing from
1. Resolve dependencies
2. Filter dependencies down for current target and selected packages
3. Resolve features

To
1. Resolve dependencies
2. Filter dependencies down for current target
3. Resolve features
4. Filter for selected packages

**Features will be evaluated for each package in isolation**
Copy link
Member

Choose a reason for hiding this comment

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

The main situation I encounter this with is a workspace that has a single package, but the different cargo targets have different dependencies and that affects feature resolution in a way that cargo build vs cargo test leads to rebuilds. Will this RFC help there? If yes, then the text should be clarified, because it seems to talk entirely about packages but not about targets within a package. If not -- what would it take to help in that situation?

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 calling this out! I had considered this but forgot it and didn't want to hold it up until I could remember.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sunshowers says cargo-hakari will unify normal and dev-dependencies. Platforms are not unified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sunshowers says that is also unifies the features that workspace member features enable

Copy link
Contributor Author

@epage epage Sep 12, 2024

Choose a reason for hiding this comment

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

Some cases with unifying dev-dependencies include

  • fail being used for dev-dependencies but not production
  • Tests enabling std but not wanting them for normal build
  • Some use case related to private keys where the impl Clone is only provided on debug builds and not release builds

There are likely other cases of this same sort of problem

Copy link
Member

@RalfJung RalfJung Sep 12, 2024

Choose a reason for hiding this comment

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

In Miri's case it's dev-dependencies enabling different features for libc or some other fundamental crate, which means that even after cargo test, doing cargo build does an almost complete re-build of Miri -- that's waiting time I'd rather avoid. :)

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've summarized this under "Future possibilities"


This will require building duplicate copies of build units when there are disjoint sets of features.

For example, this could be implemented as either
- Loop over the packages, resolving, and then run a build plan for that package
- Resolve for each package and generate everything into the same build plan

This is not prescriptive of the implementation but to illustrate what the feature does.
The initial implementation may be sub-optimal.
Likely, the implementation could be improved over time.

**Note:** these features do not need to be stabilized together.

##### `resolver.feature-unification`

*(update to [Configuration](https://doc.rust-lang.org/cargo/reference/config.html))*

* Type: string
* Default: "selected"
* Environment: `CARGO_RESOLVER_FEATURE_UNIFICATION`

Specify which packages participate in [feature unification](https://doc.rust-lang.org/cargo/reference/features.html#feature-unification).

* `selected`: merge dependency features from all package specified for the current build
* `workspace`: merge dependency features across all workspace members, regardless of which packages are specified for the current build
* `package`: dependency features are only considered on a package-by-package basis, preferring duplicate builds of dependencies when different sets of feature are activated by the packages.

# Drawbacks
[drawbacks]: #drawbacks

This increases entropy within Cargo and the universe at large.

If someone enables mutually exclusive features in different packages, then `workspace` unification will fail.
Officially, features are supposed to be additive, making mutually exclusive features officially unsupported.
Instead, effort should be put towards [official mutually exclusive globals](https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618).

Some features cannot be enabled in some packages, like a `no_std` package not wanting `std` features.
These workspaces will not be able to use `workspace` unification.
For now, unifying for the `"workspace"` is primarily targeted at single-application workspaces.
The other config fields can always be used instead.

# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives

This is done in the config instead of the manifest:
- As this can change from run to run, this covers more use cases.
- As this fits easily into the `resolver` table, there is less design work.

We could extend this with configuration to exclude packages for the various use cases mentioned.
Supporting excludes adds environment/project configuration complexity as well as implementation complexity.

# Prior art
[prior-art]: #prior-art

[`cargo-hakari`](https://crates.io/crates/cargo-hakari) is a "cargo-workspace-hack" generator that builds a graph off of `cargo metadata` and re-implements feature unification.

[cargo-hack](https://crates.io/crates/cargo-hack) can run each selected package in a separate `cargo` invocation to prevent unification.

# Unresolved questions
[unresolved-questions]: #unresolved-questions

- How to name the config field to not block the future possibilities

# Future possibilities
[future-possibilities]: #future-possibilities

### Support in manifests

Add a related field to manifests that the config can override.

### Dependency version unification
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshtriplett I've added dependency version unification as a future possibility


Unlike feature unification, dependency versions are always unified across the
entire workspace, making `Cargo.lock` the same regardless of which package you
select or how you build.

This can mask minimal-version bugs.
If a version-req is lower than it needs, `-Zminimal-versions` won't resolve down to that to show the problem if another version req in the workspace is higher.
We have `-Zdirect-minimal-versions` which will error if workspace members do not have the lowest version reqs of all of the workspace but that is brittle.

If you have a workspace with multiple MSRVs, you can't verify your MSRV if you
set a high-MSRV package's version req for a dependency that invalidates the
MSRV-requirements of a low-MSRV package.

We could offer an opt-in to per-package `Cargo.lock` files. For builds, this
could be implemented similar to `resolver.feature-unification = "package"`.

This could run into problems with
- `cargo update` being workspace-focused
- third-party updating tools

As for the MSRV-case, this would only help if you develop with the latest
versions locally and then have a job that resolves down to your MSRVs.

### Unify features in other settings

[`workspace.resolver = "2"`](https://doc.rust-lang.org/cargo/reference/resolver.html#features) removed unification from the following scenarios
- Cross-platform target unification
- `build-dependencies` / `dependencies` unification
- `dev-dependencies` / `dependencies` unification unless a dev target is enabled

Depending on how we design this, the solution might be good enough to
re-evaluate
[build-target features](https://github.com/rust-lang/rfcs/pull/3374) as we
could offer a way for users to opt-out of build-target unification.

Like with `resolver.incompatible-rust-version`, a solution for this would override the defaults of `workspace.resolver`.

`cargo hakari` gives control over `build-dependencies` / `dependencies` unification with
[`unify-target-host`](https://docs.rs/cargo-hakari/latest/cargo_hakari/config/index.html#unify-target-host):
- [`none`](https://docs.rs/hakari/0.17.4/hakari/enum.UnifyTargetHost.html#variant.None): Perform no unification across the target and host feature sets.
- The same as `resolver = "2"`
- [`unify-if-both`](https://docs.rs/hakari/0.17.4/hakari/enum.UnifyTargetHost.html#variant.UnifyIfBoth): Perform unification across target and host feature sets, but only if a dependency is built on both the target and the host.
- [`replicate-target-on-host`](https://docs.rs/hakari/0.17.4/hakari/enum.UnifyTargetHost.html#variant.ReplicateTargetOnHost): Perform unification across target and host feature sets, and also replicate all target-only lines to the host.
- [`auto`](https://docs.rs/hakari/0.17.4/hakari/enum.UnifyTargetHost.html#variant.Auto) (default): select `replicate-target-on-host` if a workspace member may be built for the host (used as a proc-macro or build-dependency)

`unify-target-host` might be somewhat related to [`-Ztarget-applies-to-host`](https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#target-applies-to-host)

For Oxide `unify-target-host` reduced build units from 1900 to 1500, dramatically improving compile times, see https://github.com/oxidecomputer/omicron/pull/4535
If integrated into cargo, there would no longer be a use case for the current maintainer of `cargo-hakari` to continue maintenance.

If we supported `dev-dependencies` / `dependencies` like `resolver = "1"`, it
could help with cases like `cargo miri` where through `dev-dependencies` a
`libc` feature is enabled. preventing reuse of builds between `cargo build` and
`cargo test` for local development.

In helping this case, we should make clear that this can also break people
- `fail` injects failures into your production code, only wanting it enabled for tests
- Tests generally enabled `std` on dependencies for `no_std` packages
- We were told of use cases around private keys where `Clone` is only provided when testing but not for production to help catch the leaking of secrets