-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
| 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.
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 would strongly consider how annoying opt-out traits have been on the language design level, and "default-features" and feature migrations too, so we don't repeat the same mistakes here.
If we require core-only users to explicitly depend on core, yes they have type a few more characters, but we side step all those problems from the get-go.
(For what is is worth, the outcome I think is most likely is that in the coming decade, as we find ways to move more standard library functionality onto crates.io "full userland", core eventually becomes something that also depends on other crates. This is exactly analogous to the "oops, Sized is now longer the first trait in the hierarchy" problem, and close to the "oops, I want to add a new default feature, how do people opt out?" problem.)
Writing the core dependency one per crate is a lot cheaper than writing T: Sized in every bound, so let's just do it! Problem avoided!
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.
Non-cargo users are important to consider here - they may need to add features to their own build systems if passing in stdlib crates via --extern becomes mandatory in any sense.
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.
If we require core-only users to explicitly depend on core, yes they have type a few more characters, but we side step all those problems from the get-go.
This is exactly what we already propose, if you depend only on core then you need to explicitly write your dependency on core to remove the implicit dependencies on alloc and 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.
OK that sounds fine for external creates. Sorry for my misunderstanding.
New related question, that I think needs a call-out in the RFC:
How is core itself to be packaged? (Or really all standard library crates?) It would be nice for them to explicitly have no stdlib deps, rather than rely on today's hacks.
I see two approaches, which I'll Alan-Kay-style call eager and late binding:
-
late binding: Standard library crates use
builtin = trueon other standard library crates, and rely on the perma-unstable patching mechanism to make those refer to the right crates (that are presumably in the repo). -
eager binding: standard library creates just refer to each other with
pathdependencies, like today.
I think late binding is better. But the problem exist either way:
-
In the case of late binding,
coreneeds a way to have no (implicit or explicit) standard library deps, so it doesn't get a dependency on itself in thebuild-std = "always"case. -
In the case of early binding, all standard library crates need to have no (implicit or explicit) standard library deps for the same reason, because their
pathdeps on their fellow standard library crates serve that purpose 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.
I think we expect that core would use a perma-unstable cargo feature to disable implicit builtin deps, which we don't mention in the RFC.
I think our intention was that path deps for std/alloc/core will disable the implicit standard library dependencies as otherwise this would limit their use to nightly (using the same feature core would use) unless a builtin dep could be added to otherwise disable them. We should probably mention this explicitly.
I think with those two facts in play standard library crates could either use path deps or patched builtin deps and there isn't a practical difference between the two.
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.
Perma way to disable implicit standard library deps without creating implicit ones sounds good to me.
I think with those two facts in play standard library crates could either use path deps or patched builtin deps and there isn't a practical difference between the two.
The practical difference is if someone (presumably working on some obscure platform) wants to patch our one dep but keep the others. Patching just the builtin dep vs patching that and also some path deps is a bit easier.
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 some wording around this in 97de9cd
|
|
||
| [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.
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 guess that my main opinion here is that it's something that requires immediate attention to be fixed when it doesn't really affect the ability to build the code. If someone runs cargo add std --optional, this suddenly prevents rust-analyzer from working at all until it's fixed, and while the error should definitely be fixed before publishing the package, I fail to see how this should completely deadlock cargo's ability to build the crate at all until it's 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.
cargo add could just automatically add core if you're adding std or alloc.
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.
cargo addcould just automatically addcoreif you're addingstdoralloc.
Added this in 2b189a4
eafc6b2 to
4b870f6
Compare
| > ├── rand v0.7.3 | ||
| > │ ├── getrandom v0.1.14 | ||
| > │ │ ├── cfg-if v0.1.10 | ||
| > │ │ │ └── core v0.0.0 |
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 get why these don't have a usable version number, but IMHO, this should either display as core (built-in) or use the current Rust toolchain version instead. I can see the latter being considered very useful for the theoretical long-term case where crates could potentially be compiled with different libstd versions and then linked together, e.g. via dynamic linking.
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.
Removed the version number in fc16fa8
|
As a heads up, we are planning to start an FCP proposal on this RFC shortly after #3873 (build-std context) has been approved. |
As part of rust-lang#3883, we have switched the template to stop using level-1 headings which isn't the way mdbook is intended to be used.
| > Explicit dependencies are passed to rustc without the `noprelude` modifier | ||
| > ([?][rationale-explicit-noprelude]). When adding an explicit dependency, users | ||
| > may need to adjust their code (removing extraneous `extern crate` statements | ||
| > or root-relative paths, like `::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.
I have a concern.
When developing a library with an optional std or alloc dependency, it is useful that those crates’ preludes are never in scope, because it means that all uses of alloc or std contain a textual mention of those crates, making the code easier to author and review for no_std compatibility without necessarily needing to run a build with a configuration lacking std or alloc.
Unless I misunderstand what this paragraph means, using explicit std dependencies will mean that it is no longer possible to author a crate in this strict style, because enabling the dependency always enables its prelude.
I don't fully follow the rationale for why this behavior is more consistent. The rationale seems to talk about not passing a noprelude flag being more consistent, but as a user of rustc + Cargo, I am not familiar with that flag at all — to me it is an implementation detail. To me, it would be more consistent to say that the contents of the prelude seen by the code will stay consistent across all build configurations.
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.
Big 👍 from me. This is one of the larger reasons I've always advocated for #[cfg(feature = "std")] extern crate std; instead of #![cfg_attr(not(feature = "std"), no_std)]: it keeps the prelude consistent across all configurations.
Especially because running tests almost always requires std active it becomes very easy to accidentally commit code that uses std paths where it should have used core paths.
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.
There might be some confusion here. This is just referring to the extern prelude (that is, the presence of the std or alloc names), not the standard library prelude. The injection of the std::prelude or core::prelude library prelude will still be controlled by #![no_std].
The consistency here is referring to the behavior of crate names being added to the extern prelude whenever they are a direct dependency. Just like if you have an optional dependency on crate foo, the name foo appears in the extern prelude when that optional dependency is enabled.
Does that answer your question? Or are you doing something with extern crate std; in some nested module?
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.
That seems to address my concern, but then I still do not understand what the text is meant to express. What would be an example of “removing extraneous … root-relative paths, like ::std”? What would those paths become?
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 because using --extern std (as explicit builtin dependencies will use) and extern crate std (as is currently injected by rustc when not using #![no_std] are not entirely equivalent. I think that might only be an issue on the 2015 edition as the behaviour of paths starting with :: changed. Users would have to replace ::std... with std... or else see "crate not found" or similar errors
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.
That makes sense. I think the text should be revised to make that explicit, because I think that it is not obvious what the text means to people like me, who came to Rust after the release of the 2018 edition.
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.
Yea, I don't expect anyone to really encounter this (and is maybe a little confusing as written in the RFC since I don't think anyone will need to do this). The difference is small and subtle.
-
2015 behavior:
#![no_std] extern crate std;- Puts
stdinto the crate root and in the extern prelude. use std::fs;works (relative to root).::std::fs::read_to_string()works (relative to root).std::fs::read_to_string()works (extern prelude).
- Puts
#![no_std]and--extern std- Puts
stdinto the extern prelude, but not the crate root. use std::fs;does not work (not in crate root).- You will need to continue to use
extern crate std;
- You will need to continue to use
::std::fs::read_to_string()does not work (stdis not in the root).- You will need to continue to use
extern crate std;or don't use root-relative path.
- You will need to continue to use
std::fs::read_to_string()works (extern prelude).
- Puts
-
2018 behavior:
#![no_std] extern crate std;- Puts
stdinto the crate root and in the extern prelude. (same as 2015) use std::fs;works (extern prelude). (Mechanism differs to 2015, but similar result).::std::fs::read_to_string()works (extern prelude). (Mechanism differs to 2015, but similar result).std::fs::read_to_string()works (extern prelude). (same as 2015)
- Puts
#![no_std]and--extern std- Puts
stdinto the extern prelude, but not the crate root. (same as 2015) use std::fs;works (extern prelude). (Differs to 2015)::std::fs::read_to_string()works (extern prelude). (Differs to 2015)std::fs::read_to_string()works (extern prelude). (same as 2015)
- Puts
There is also an edge case that we hit in rust-lang/wg-cargo-std-aware#40 where migrating to explicit dependencies may require some other changes in your code. A macro-expanded extern crate can't shadow those from --extern. I believe it may be possible to do something like use alloc as std; in that kind of scenario, though I'm not sure I would really recommend structuring things that way (that particular crate no longer does).
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 little bit of wording around this in cd9b4da, let me know if that is sufficient or if more is necessary
| > When adding an explicit dependency, users > may need to adjust their code | ||
| > (removing extraneous `extern crate` statements > or root-relative paths, | ||
| > like `::std` - this will likely only be the case on the 2015 edition). |
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.
Looks like there was a word-wrapping issue.
| > When adding an explicit dependency, users > may need to adjust their code | |
| > (removing extraneous `extern crate` statements > or root-relative paths, | |
| > like `::std` - this will likely only be the case on the 2015 edition). | |
| > When adding an explicit dependency, users may need to adjust their code | |
| > (removing extraneous `extern crate` statements or root-relative paths, | |
| > like `::std` - this will likely only be the case on the 2015 edition). |
|
Thanks everyone! I think we're at a stage where we can start a proposal for FCP. @rfcbot fcp merge cargo,crates-io |
|
Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed. 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. |
| on the `builtin` source using the `[source]` Cargo config table | ||
| ([?][rationale-source-replacement]), and nor is it possible to override | ||
| `builtin` dependencies with the `[replace]` sections or `paths` overrides | ||
| ([?][rationale-overriding-builtins]), though [patching][patches] is permitted. |
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 this call out it is unstable? I got confused enough and I chose to follow the links but others might not
| ### Public and private dependencies | ||
| [public-and-private-dependencies]: #public-and-private-dependencies | ||
|
|
||
| Implicit and explicit dependencies on the standard library default to being |
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 thought this was only for implicit dependencies and the example and rationale only talk about implicit
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.
cargo add section says that it will automatically add public = true for built-in dependencies which was motivated by explicit dependencies defaulting to private.
| [build-std-always][rfcs#3874-standard-library-crate-stability]). Any explicit | ||
| standard library dependency present in any dependency table will disable the | ||
| implicit dependencies (e.g. an explicit `builtin` or `path` dependency from | ||
| `std` will disable the implicit 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 makes it sound like we need to scan [*dependencies] and [target.*.*dependencies] to know whether implicit deps are used but I thought it was more local than that.
| ### Why disallow source replacement on `builtin` packages? | ||
| [rationale-source-replacement]: #why-disallow-source-replacement-on-builtin-packages | ||
|
|
||
| Modifying the source code of the standard library in the `rust-src` component is | ||
| not supported. Source replacement of the `builtin` source could be a way to | ||
| support this in future but this is out-of-scope for this proposal. | ||
|
|
||
| See [*Allow `builtin` source replacement*][future-source-replacement]. | ||
|
|
||
| ↩ [*Proposal*][proposal] | ||
|
|
||
| ### Why not permit overriding dependencies with `replace` or `paths`? | ||
| [rationale-overriding-builtins]: #why-not-permit-overriding-dependencies-with-replace-or-paths | ||
|
|
||
| Similarly to [source replacement][rationale-source-replacement], easing | ||
| modification of the standard library sources is out-of-scope for this proposal. |
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.
Doesn't all of this also apply to patch?
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.
Ah, that is a couple questions down
| ### Why add standard library dependencies to `Cargo.lock`? | ||
| [rationale-cargo-lock]: #why-add-standard-library-dependencies-to-cargolock | ||
|
|
||
| `Cargo.lock` is a direct serialisation of a resolve and that must be a two-way | ||
| non-lossy process in order to make the `Cargo.lock` useful without doing further | ||
| resolution to fill in missing `builtin` packages. |
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.
Note that we will need to support Cargo.lock without builtins anyways because we don't force the use of new lockfile versions.
|
|
||
| ↩ [*Proposal*][proposal] | ||
|
|
||
| ### Why unstably permit patching of the standard library 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 isn't linked to from where patches are talked about
| ### Why add standard library crates to Cargo's index? | ||
| [rationale-cargo-index]: #why-add-standard-library-crates-to-cargos-index | ||
|
|
||
| When Cargo builds the dependency graph, it is driven by the index (not | ||
| `Cargo.toml`), so builtin dependencies need to be included in the index. |
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.
Is this about adding them to the index (as top-level packages), which isn't needed, or adding dependencies on the standard library to the index format?
| ### What syntax is used to patch dependencies on the standard library in `Cargo.toml`? | ||
| [unresolved-patch-syntax]: #what-syntax-is-used-to-patch-dependencies-on-the-standard-library-in-cargotoml | ||
|
|
||
| `[patch.builtin]` is the natural syntax given `builtin` is a new source, but may | ||
| be needlessly different to existing packages. | ||
|
|
||
| ↩ [*Patches*][patches] |
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.
Considering this is unstable for internal purposes, what works the easiest for implementation seems most appropriate.
| [`cargo metadata`][cargo-metadata] will emit `std`, `alloc` and `core` | ||
| dependencies to the metadata emitted by `cargo metadata` (when those crates are | ||
| explicit dependencies). `source` would be set to `builtin` and the remaining | ||
| fields would be set like any other dependency. See also unresolved question | ||
| [*Should `cargo metadata` include the standard library's dependencies?*][unresolved-cargo-metadata]. |
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.
Isn't there a question about compatibility as this would break users on upgrade?
| ### Replace `#![no_std]` as the source-of-truth for whether a crate depends on `std` | ||
| [future-replace-no_std]: #replace-no_std-as-the-source-of-truth-for-whether-a-crate-depends-on-std | ||
|
|
||
| Crates can currently use the crate attribute `#![no_std]` to indicate a lack of | ||
| dependency on `std`. Introducing `build-std.crates` from [RFC #3874][rfcs#3874] | ||
| or explicit dependencies would add a second way for the user to indicate a lack | ||
| of dependency on the standard library. It could therefore be desirable to | ||
| deprecate `#![no_std]` so that there remains only a single way to express a | ||
| dependency on the standard library. | ||
|
|
||
| `#![no_std]` serves two purposes - it stops the compiler from adding `std` to | ||
| the extern prelude and it prevents the user from depending on anything from | ||
| `std` accidentally. rustc's default behaviour of loading `std` when not | ||
| explicitly provided the crate via an `--extern` flag should be preserved for | ||
| backwards-compatibility with existing direct invocations of rustc. | ||
|
|
||
| Initially, if a crate has the `#![no_std]` attribute and has implicit | ||
| dependencies on the standard library in its `Cargo.toml`, a lint could be | ||
| emitted to suggest that their Cargo dependencies are adjusted. | ||
|
|
||
| Eventually, `#![no_std]` could instead become a compiler flag which would | ||
| indicate to the compiler that `std` should not be loaded by default and that | ||
| `core`'s prelude should be used instead. Cargo would use this flag when driving | ||
| rustc, providing explicit paths to the newly-built or pre-built standard library | ||
| crates, just as with any other dependency. | ||
|
|
||
| In addition, uses of the `#![no_std]` attribute could be migrated to denying a | ||
| lint which would prevent use of items from `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.
Is this saying that if I only depend on core.builtin = true then I still have to put in #![no_std]? If so, that seems wrong to me
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