-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Open
epage
wants to merge
32
commits into
rust-lang:master
Choose a base branch
from
epage:feature-unification
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+195
−0
Open
Changes from all commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
97d49c2
Skeleton
epage c3b17e8
Initial
epage a921e09
Set RFC number
epage 7d6810d
Fix typo
epage 5b13f93
Fix typo
epage 750b5f1
Fix typo
epage 4ebf0ff
Remove boilerplate
epage 703a636
Remove copy/pasted header
epage e7a4f27
Move mutually exclusive features to a Drawback
epage f2bb64a
Cover no_std with std features being unified
epage c5bbeee
Fix typo
epage 65e6988
Note cargo-hakari's build/host unification features
epage e752830
Add build/host unification qualification
epage 0cebf5f
Fix typo
epage 363be82
Add citation
epage 6860e51
Note ecosystem importance of build/host unification
epage 8b114b3
Fix typos
epage 4bbdc51
fix: Clarify some points
epage 8f848a4
Expand on future possibilities
epage c2a4ac1
Expand on dev/normal unification
epage d35c626
fix: Typo
epage 760b618
fix: Clarify statement
epage cf07a12
fix: Whatever
epage 51839e5
fix: Clarify statements
epage 6318f47
fix: Clarify statement
epage cd2af9a
fix: Typi
epage 1b2a3d2
fix: Clarify statement
epage eb8ffb2
fix: Typo
epage ff5e82a
fix: Link out to another related Issue
epage 0aac9ab
fix: Remove issue link
epage c0158e7
fix: Disambiguate each use of 'target'
epage 969cb03
Make cargo-install interaction explicit
epage File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,195 @@ | ||
- Feature Name: `feature-unification` | ||
- Start Date: 2024-09-11 | ||
- RFC PR: [rust-lang/rfcs#3692](https://github.com/rust-lang/rfcs/pull/3692) | ||
|
||
# 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 build-target and selected packages | ||
3. Resolve features | ||
|
||
To | ||
1. Resolve dependencies | ||
2. Filter dependencies down for current build-target | ||
3. Resolve features | ||
4. Filter for selected packages | ||
|
||
**Features will be evaluated for each package in isolation** | ||
|
||
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. | ||
|
||
This field will not apply to `cargo install` to match the behavior of `resolver.incompatible-rust-versions`. | ||
|
||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 build-target unification | ||
- `build-dependencies` / `dependencies` unification | ||
- `dev-dependencies` / `dependencies` unification unless a dev build-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 platform-target and the host. | ||
- [`replicate-target-on-host`](https://docs.rs/hakari/0.17.4/hakari/enum.UnifyTargetHost.html#variant.ReplicateTargetOnHost): Perform unification across platform-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 |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
vscargo 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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 productionstd
but not wanting them for normal buildimpl Clone
is only provided on debug builds and not release buildsThere are likely other cases of this same sort of problem
There was a problem hiding this comment.
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 aftercargo test
, doingcargo build
does an almost complete re-build of Miri -- that's waiting time I'd rather avoid. :)There was a problem hiding this comment.
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"