-
Notifications
You must be signed in to change notification settings - Fork 1.6k
build-std: explicit dependencies #3875
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
base: master
Are you sure you want to change the base?
build-std: explicit dependencies #3875
Conversation
| ..is equivalent to the following explicit dependency on `std`: | ||
|
|
||
| ```toml | ||
| [package] | ||
| name = "hello_world" | ||
| version = "0.1.0" | ||
| edition = "2024" | ||
|
|
||
| [dependencies] | ||
| std = { builtin = true, public = true } | ||
| ``` |
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.
While I think this is a sensible default, I think there should be a warn-by-default lint if this ever occurs for a crate annotated with #![no_std], since we should be pushing people toward adding an alloc or core dependency instead.
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.
To clarify, a lint in the compiler if we end up passing --extern std=... to rustc but with a #![no_std] crate? That makes sense to me but the part of the document you've commented on is about public/private dependencies which seems unrelated, so wanted to double check.
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.
Basically, if:
Cargo.tomldoesn't have any std/alloc/core dependencies- The crate is
no_std
It may require explicit support from Cargo to do this.
| If there is an optional dependency on the standard library then Cargo will | ||
| validate that there is at least one non-optional dependency on the standard | ||
| library (e.g. an optional `std` and non-optional `core` or `alloc`, or an | ||
| optional `alloc` and non-optional `core`). `core` cannot be optional. For | ||
| example, the following example will error as it could result in a build without | ||
| `core` (if the `std` feature were disabled): |
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.
While it is an unstable feature, I think it's worth commenting on how this interacts with the no_core feature: will crates using this still have to specify the core dependency, or will it be allowed to be omitted in those cases? Additionally, would this require an additional cargo feature or would just the feature on the crate be enough?
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 haven't considered no_core because it's perma-unstable. I don't see us ever stabilising it unless we're comfortable with alternative core crates having no stability guarantees (otherwise we can't add new required language items, which is very useful). Given that there is no path to or interest in stabilising this feature, I don't think it is worth mentioning in the RFC - a proposal to stabilise no_core can mention interactions with build-std rather than this RFC anticipating it.
|
|
||
| [dependencies] | ||
| std = { builtin = true, optional = true } | ||
| # error: must have a non-optional dependency on core |
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 think that it might be worth mentioning the concept of lints here to make this less annoying of a requirement.
Right now, we have the concept of cargo lints, but we don't have any lints that let you autofix Cargo.toml, which I think would be a much-appreciated addition here. I feel like it makes a substantial difference toward this being a reasonable requirement if, instead of a hard error, this were a warn-by-default lint (implicitly adding the core dependency anyway) and that crates like this simply could not be published to crates.io.
In particular, it would be quite annoying if this issue completely prevented rust-analyzer from operating on code until it were fixed.
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'm not convinced this is that annoying a requirement. Users will only hit this after adding an explicit optional dependency on the alloc or std and immediately notice the error and need to fix it. I don't see the value in having a lint that would allow you to delay fixing this but would force you to fix it eventually when you want to publish the crate.
| ## `dev-dependencies` and `build-dependencies` | ||
| [dev-dependencies-and-build-dependencies]: #dev-dependencies-and-build-dependencies | ||
|
|
||
| Explicit dependencies on the standard library can be specified in | ||
| `dev-dependencies` in the same way as regular `dependencies`. Any explicit | ||
| `builtin` dependency present in `dev-dependencies` table will disable the | ||
| implicit dependencies. It is possible for `dev-dependencies` to have additional | ||
| `builtin` dependencies that the `dependencies` section does not have (e.g. | ||
| requiring `std` when the regular dependencies only require `core`). |
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 suspect this will be something people don't fully understand the implications of until they are testing the feature, so we should make sure we note this here and then in the tracking issue to call this out for real world feedback.
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 added a link to this comment from the PR description
| [`cargo add`][cargo-add]'s heuristics will include adding `std`, `alloc` or | ||
| `core` as builtin dependencies if these crate names are provided. `cargo add` | ||
| will additionally have a `--builtin` flag to allow for adding crates with a | ||
| `builtin` source explicitly: |
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.
What happens if I do:
$ cargo new foo
$ cd foo
$ cargo add std --optionalShould we
- error upfront
- wait for the natural parse error
- act as if they also did
cargo add coreorcargo add alloc(next lowest library)
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'd expect we'd wait for the natural parse error but I don't feel strongly, if Cargo has different norms around how to deal with cases like this then we can follow those norms.
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.
Today, we don't do much upfront work which leads to situations like rust-lang/cargo#16101
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'd lean towards following existing precedent and if issues like rust-lang/cargo#16101 are something that t-cargo would like to resolve then a case like this can be included in that effort.
| > When producing a registry index entry for a package Cargo will not serialise | ||
| > any `builtin` dependencies it inferred. This allows the set of inferred | ||
| > packages to change in the future if needed. Similarly, the published | ||
| > `Cargo.toml` will not explicitly declare any inferred dependencies. |
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.
This is missing a very important motivation: if we put implicit builtins into Cargo.toml, then publishing with a new Cargo would raise your MSRV
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.
Added this motivation in 87c3a95
| [`cargo add`][cargo-add]'s heuristics will include adding `std`, `alloc` or | ||
| `core` as builtin dependencies if these crate names are provided. `cargo add` | ||
| will additionally have a `--builtin` flag to allow for adding crates with a | ||
| `builtin` source explicitly: |
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.
Should cargo add automatically add public = true for builtins?
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.
Hmm, but what about before its stabilized?
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 added a note that public= true would be added after public/private deps are stabilised in c214948
| [dependencies] | ||
| std = { builtin = true } |
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'm assuming this is more an implementation detail but it could have design ramifications: how do std dependencies refer to each other? builtin, path, or something else? Both first-party and vendored.
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 think they would continue to use path, and then we can simplify how their dependencies refer to them as in Remove rustc_dep_of_std.
| [`cargo metadata`][cargo-metadata] will emit `std`, `alloc` and `core` | ||
| dependencies to the metadata emitted by `cargo metadata` (when those crates are | ||
| explicit dependencies). None of the standard library's dependencies will be | ||
| included. `source` would be set to `builtin` and the remaining fields would be | ||
| set like any other dependency. |
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.
This feels a bit odd to me. I can understand why we would want to skip this if they are implied, as thats the current state of things and it would technically bloat the output unnecessarily. But if explicit, why would we omit the dependencies of the standard libraries? Maybe I am misunderstanding but this sounds like it would make it difficult for tools like rust-analyzer to use the output to construct the full crate graph with sources and all correctly?
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 think I excluded dependencies of the standard library only as we've tried to hide those as much as possible throughout, but cargo metadata is probably a special-case here because it is used by tools like rust-analyzer. I don't know enough about how cargo metadata is used by rust-analyzer to know what the right answer here is - do you need to construct a full crate graph including the dependencies of std?
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.
We need to know the entire crate graph of the standard libraries as well yes, otherwise we cannot correctly analyze things as even private dependencies can crop up in public things, think of type sizes for example.
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'm less sure that cargo metadata is a special case. If looking at to change this, at least call it out as Unresolved to make sure the Cargo team explicitly talks this over.
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.
Okay, I've changed this to include std's deps in 4b870f6.
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.
As I said, please leave this as an Unresolved Question
eafc6b2 to
4b870f6
Compare
Allow users to add explicit dependencies on standard library crates in the
Cargo.toml. This enables Cargo to determine which standard library crates are required by the crate graph withoutbuild-std-cratesbeing set and for different crates to require different standard library crates.This RFC is is part of the build-std project goal and a series of build-std RFCs:
build-std="always"(build-std: always #3874)build-std="compatible"(RFC not opened yet)build-std="match-profile"(RFC not opened yet)There is also a Zulip channel where you can ask questions about any of the build-std RFCs. This series of RFCs was drafted over many months with the help of stakeholders from many Rust project teams, we thank them for their help!
There are some details that have been noted as being worth mentioning in any eventual tracking issues:
Rendered