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: Allow packages to specify a set of supported targets #3759

Open
wants to merge 65 commits into
base: master
Choose a base branch
from

Conversation

carloskiki
Copy link

@carloskiki carloskiki commented Jan 8, 2025

Summary

The addition of supported-targets to Cargo.toml. This field is an array of target-triple/cfg specifications that restricts the set of targets which a package supports. Packages must meet the supported-targets of their dependencies, and they can only be built for targets that satisfy their supported-targets.

Rendered

Somtimes use "crate" instead of "cargo-target" for better readability

added section on handling cfgs

miscellaneous fixes

testing

fix dead links
fix links and typos

fix example

fix note

remove note

more fixes

fixes

minor fixes

Third draft

add note to cargo-target level

remove todo
- fix GH ui bug (indented codeblocks)
@Lokathor
Copy link
Contributor

Lokathor commented Jan 8, 2025

I think that there should be a little more explanation about how docs generation works with this. Specifically: Can I build docs for a target that's unsupported, such as if my host machine isn't supported, can i cargo doc to still just see the documentation?

@ehuss ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Jan 8, 2025
@opeik
Copy link

opeik commented Jan 8, 2025

This would be a godsend for crates that link against third party libraries, thus limiting supported targets.

@carloskiki
Copy link
Author

I think that there should be a little more explanation about how docs generation works with this. Specifically: Can I build docs for a target that's unsupported, such as if my host machine isn't supported, can i cargo doc to still just see the documentation?

Indeed, and this is especially important since docs.rs needs to be able to generate the docs for all crates. I added it here.

@ahicks92

This comment was marked as off-topic.

@Lokathor
Copy link
Contributor

Lokathor commented Jan 9, 2025

Crates assuming that they're running on one of several targets could even end up making unsafe code decisions based on that fact. Forcing the code to "just build anyway" would naturally lead to problems.

And crates can already force themselves to only build only on a specific target, this would not be a new ability, but instead it's only a way to better organize that information.

@workingjubilee
Copy link
Member

The author already mentions in the Prior art that the following Rust can be written:

#[cfg(target_arch = "lol"))]
compile_error!("experience bij)";

Please do not make comments on the RFC which do not engage with the RFC's content.


User experience is enhanced by raising an error that fails compilation when the supported targets
of a package are not satisfied by the selected target. A package's `supported-targets` must be a subset
of its dependencies' `supported-targets`, otherwise the build also fails.
Copy link
Member

@joshtriplett joshtriplett Jan 9, 2025

Choose a reason for hiding this comment

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

This seems helpful, but at the same time, it may prove annoying to have to copy these across, if a dependency has a very specific list. And it may be non-trivial to enforce.

I think we should downgrade this to a lint, and say that it's best-effort, not mandatory.

Copy link
Member

Choose a reason for hiding this comment

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

This also seems complicated by the fact that I might have a dependency that only supports (say) wasm, but I'm using it solely as a dev-dep (e.g., in tests). I don't think that case merits also setting supported targets on the containing package as a whole, since downstream consumers might not care about that limitation for running on tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's just a lint then unsafe coders can't depend on this, and they will still need to just use a compile_error! or something if they're really trying to avoid unsoundness.

Copy link
Contributor

@epage epage Jan 9, 2025

Choose a reason for hiding this comment

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

This is effectively saying that if you have a dependency that can't build on a target you claim to support, the build will fail and you should instead move it to a target.*.dependencies table.

imo that seems like something that should be a hard error to me.

I could see loosening the restriction on

When supported-targets is not specified, any target is accepted, so all dependencies must support all targets.

To me "we don't check", like package.rust-version, requiring supported-targets = ["cfg(true)"] to get the checking.

Copy link
Author

Choose a reason for hiding this comment

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

I totally agree with @epage here.

Using the same logic as package.rust-version is also something I have not thought of but is a great idea.

Copy link
Author

Choose a reason for hiding this comment

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

A problem I could see however is that if a package foo does not use supported-targets but one of its dependencies does, then when depending on foo errors of incompatible targets can be hard to solve since they come from transitive dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not quiet sure the problem. When building foo, you are building for a specific target and the error would be for that target. With package.rust-version, we check what all packages aren't compatible with the current toolchain and provide a single error message, see https://github.com/rust-lang/cargo/blob/9589831f61a8259919e64c6d68c1a36efc6efd20/src/cargo/ops/cargo_compile/mod.rs#L494-L543

Copy link
Member

Choose a reason for hiding this comment

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

If it's just a lint then unsafe coders can't depend on this, and they will still need to just use a compile_error! or something if they're really trying to avoid unsoundness.

supported-targets would still be enforced (modulo some kind of force option); I'm talking about softening the enforcement that a crate's supported-targets is a subset of its dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this has moved to a Future Possibility

@joshtriplett
Copy link
Member

This looks excellent!

@joshtriplett
Copy link
Member

Let's go ahead and start the process of asynchronously checking for consensus.

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 9, 2025

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Jan 9, 2025
@joshtriplett
Copy link
Member

@rfcbot concern should-crates-have-to-set-supported-targets-to-match-their-dependencies

Comment on lines 353 to 355
- `required-targets`. Pro: it matches with the naming of `required-features`. Con: `required-features` is a list of features
that must _all_ be enabled (conjunction), whereas `supported-targets` is a list of targets
where _any_ is allowed (disjunction).
Copy link
Contributor

Choose a reason for hiding this comment

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

For me the important precedence is that the field means "skip if the qualification is not met" and for that reason I favor using this name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently, this is different than required-features as this errors, rather than skips.

I raised this at https://github.com/rust-lang/rfcs/pull/3759/files#r1909208702

Copy link
Contributor

Choose a reason for hiding this comment

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

I also feel like using the word "support" carries a lot of unnecessary connotations that complicate the conversation. For example, with MSRV, the Cargo team has been leaning in the direction that "support" is an active process that gets tested. However, applying that here would lead to people over-constraining their targets.

Copy link
Author

Choose a reason for hiding this comment

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

The difference with this and required-features is that supported-targets is at the package level, not at the cargo-target level.

So if I run cargo build in a package with a binary foo which does not have its required-features met, then cargo can still possibly "do work" if there is another cargo-target which has its required-features met (for instance a library cannot have any required-features).

However if the supported-targets are not met for a package, then there is no chance of cargo doing compilation work for that package. That is why the packages are skipped when in a workspace, but an error is raised in a single package. This is just how if I ran cargo build --bin foo in the previous example, then cargo errors instead of skipping.

Copy link
Author

Choose a reason for hiding this comment

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

I do not have a preference for the name, I kept it as is to not confuse people who had read the Pre-RFC. I would not be against changing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this being at the package or build-target level makes much of a difference. The RFC is starting at the package level and has build-target as a future possibility. In that case, we can treat this as the package is providing the default for all build-targets like package.edition

Copy link
Contributor

@epage epage Jan 28, 2025

Choose a reason for hiding this comment

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

@epage
Copy link
Contributor

epage commented Jan 9, 2025

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

- Should we strip the `cfg` prefix from the field e.g., `supported-targets = 'target_os = "linux"'`?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should. This makes the form of the syntax explicit, follows the pattern of how cfg syntax is used everywhere else, and gives us room to evolve this field

Comment on lines 87 to 88
The `supported-targets` field is an optional key that tells cargo which targets the package can be
built for.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about build scripts? Host targets can be very different than deployment targets.

Or when we do dependency filtering, do we only filter normal/dev dependencies and not build scripts?

Copy link
Author

Choose a reason for hiding this comment

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

In theory any host-tuple should be able to run the build script, so build dependencies should support all targets. Enforcing this is very impractical, so we resort to failing the build if one of the [build-dependencies] does not support the host. See here.

I also added some details in bc7d86b, specifically that we do not filter build-dependencies.

`supported-targets`, like any other dependency.


## Eliminating unused dependencies from `Cargo.lock`
Copy link
Contributor

Choose a reason for hiding this comment

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

The main focus of this is limiting inclusion in cargo vendor so that should be clearly called out here

`supported-targets`, like any other dependency.


## Eliminating unused dependencies from `Cargo.lock`
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking, this feature is not blocked on this RFC. We could do set logic between target.*.dependencies and target.*.dependencies. When a package itself is only supported on some platforms, they could workaround the lack of supported-targets by putting all dependencies in a target.*.dependencies.

Copy link
Author

Choose a reason for hiding this comment

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

When a package itself is only supported on some platforms, they could workaround the lack of supported-targets by putting all dependencies in a target.*.dependencies.

Indeed. supported-targets only adds an implied target.* bound.
Similarly people can already restrain their crate to specific targets with compile_error!().

In the end supported-targets is only about the ergonomics of doing so and running commands like cargo check --workspace.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add this to alternatives that the resolver changes could be made without manifest changes, just with less ergonomics?

Comment on lines +428 to +430
Formally, dependencies (and transitive dependencies) under `[target.**.dependencies]` tables are
eliminated from the dependency tree of a package if the `supported-targets` of the package is
mutually exclusive with the target preconditions of the dependency.
Copy link
Contributor

Choose a reason for hiding this comment

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

The way this is written, it sounds like all target.*.dependencies tables are compared only to supported-targets

You could have a situation like

[package]
name = "foo"
supported-targets = 'cfg(target_os = "linux")'

[dependencies]
bar = "1.0.0"
[package]
name = "bar"

[target.'cfg(target_os="windows")'.dependencies]
baz = "1.0.0"
[package]
name = "baz"

[target.'cfg(target_os="linux")'.dependencies]
bob = "1.0.0"
[package]
name = "bob"

As specified, bob will be included in Cargo.lock / cargo vendor but it doesn't need to be.

However, implementing this properly in the resolver is complex and we may not want to do it for an MVP solution.

Copy link
Author

Choose a reason for hiding this comment

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

If baz is filtered out then bob is also filtered out because its parent is removed from the tree. Maybe this need clarification.

Copy link
Contributor

Choose a reason for hiding this comment

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

The potential for both approaches should be called out so we don't forget when we get to this and because part of this is being evaluated based on these future possibilities.

Comment on lines +432 to +438
### Comparing `supported-targets`

To prune the dependency tree, and to ensure proper use of dependencies, it becomes necessary to
compare `supported-targets`. When comparing two sets of `supported-targets`, it is necessary to
know if one is a _subset_ of the other, or if both are _mutually exclusive_. To proceed, both
are flattened to the same representation, and they are then compared. This process is done
internally, and does not affect the `Cargo.toml` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically we'll only compare supported-targets to target.*.dependencies for Cargo.lock / cargo vendor.

Copy link
Author

Choose a reason for hiding this comment

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

We could also want to lint/warn for dependencies that do not support all of supported-targets. In which case we need to compare the supported-targets of the dependencies with the supported-targets of the package.

Comment on lines +432 to +438
### Comparing `supported-targets`

To prune the dependency tree, and to ensure proper use of dependencies, it becomes necessary to
compare `supported-targets`. When comparing two sets of `supported-targets`, it is necessary to
know if one is a _subset_ of the other, or if both are _mutually exclusive_. To proceed, both
are flattened to the same representation, and they are then compared. This process is done
internally, and does not affect the `Cargo.toml` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

A general rule in Cargo development is cargo +<version> check should not make the lockfile dirty in git.

This means we'd be locked-in to the implementation once stabilized. We could rev workspace.resolver or the lockfile version for new implementations but we'd need to keep supporting every variant.

This can crop up with

  • Changes to the set logic, including
    • Bug fixes
    • Any changes we make related to not handling
    • Changes in heuristics between related cfgs
  • Changing from target.*.dependencies only being compared to supported-targets to also compared to all target.*.dependencies that lead to it

Comment on lines +468 to +478
#### The subset relation

To determine if the `supported-targets` set "A" is a subset of another such set "B", the standard
mathematical definition of subset is used. That is, "A" is a subset of "B" if and only if each
element of "A" is contained in "B".

So each element of the union forming "A" is compared against each element of the union forming "B".
A `cfg(all(A, B, ...))` is a subset of a `cfg(all(C, D ...))`, if the list `C, D, ...` is a subset
of the list `A, B, ...`.

_Note_: `cfg(A) == cfg(all(A))`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do nots need to be called out in relation to subset handling? In some cases, it seems like they'll need to evaluate to true and that should probably be called out

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand the claim here. Is it that if A and B are mutually exclusive, then B ⊆ not(A)?

`supported-targets`, like any other dependency.


## Eliminating unused dependencies from `Cargo.lock`
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should not have cargo vendor filtering use supported-targets but instead a new resolver.targets .cargo/config.toml field.

Most of the rest of the RFC works equally for application and library developers. This section is almost exclusively for application developers as they are the ones most likely to use cargo vendor. These vendored dependencies are meant for upstream pre-built binaries. They will be dealing with specific --target <platform>. Any downstream-built binaries that might be beyond this set will likely also exclude the vendored dependencies.

So if we provide a different mechanism for this feature focused on target tuples

  • Application developers are already coupled to target tuple specifics so this does not make their use of volatile target tuples any worse
  • By being able to use target tuples, this removes the largest piece of volatility / complication
  • Being further out of the way and in an already a volatile situation (target tuples) might mean its ok for any remaining volatility in the set relation logic?
  • However, Cargo.lock would become dependent on transient / environment configuration. We may want to record the targets used for resolving in Cargo.lock so they show up in any diffs
  • However, cargo publish would include a Cargo.lock that is not intended for all platforms and cargo install --locked may fail. Maybe cargo publish could resolve the published Cargo.lock for all platforms (unless --locked is specified for which it would error) and it would be ok?

So the question then is if this RFC is justifiable enough on its own without the expectation of this future possibility building on it. I think so because the workarounds for cargo check --workspace with platform-specific packages is quite annoying, it could be useful for documentation purposes, and the future possibility of the lints could help people better manage their dependencies.

@taladar
Copy link

taladar commented Feb 28, 2025

I haven't seen it mentioned here yet in the discussion so I would just like to add that the information on supported targets might also be useful for tools checking for security issues to allow them to not mention issues in platform-specific dependencies only affecting targets not supported by the application (e.g. a lot of Linux only applications do not need to worry about Windows-specific security holes in indirect dependencies).

I would also suggest that epage brings up an important point when he mentions host and deployment targets. This should probably be split to avoid later painful changes for cross compilation, especially since that is becoming more common with WASM and embedded targets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. T-cargo Relevant to the Cargo team, which will review and decide on the RFC.
Projects
Status: FCP blocked
Development

Successfully merging this pull request may close these issues.