build-std: first iteration #1
Conversation
|
Addressed the feedback so far, also pushed a bunch more non-context content that I had drafts of - there's plenty of TODO comments and things to fix in those parts, definitely not done yet, but will flesh out more and improve over the coming days. |
Refined what toml keys are valid for `builtin = true` deps
Assorted review comments
Expanded proposed lockfile behaviour
There was a problem hiding this comment.
First of all, thank you for doing the huge amount of work here! My comments may seem negative, but my intent is to try to improve things and warn you about how some of this seems from a first look.
I worry that the scope of this is too large, and that there are a many things in here that are going to be hard sells. There are a lot of breaking changes in here which I feel like people won't find acceptable. I worry that trying to post this as an RFC likely will result in at least a year of discussion before we can get people on board to approve it. I don't want to sound too negative, this is really great stuff! I'm just warning you that this is going to be very difficult.
My confidence is low that this will be able to support all targets out the gate. I would strongly recommend thinking about a slower, more controlled rollout (rust-lang/wg-cargo-std-aware#86).
I'm worried about how this is setting up an expectation that just building the standard library for all targets will "just work" for everyone. I think that is going to be extremely difficult, especially with the other goal of expecting it to match the pre-built standard library. That's why I was strongly recommending that this be an opt-in process, instead of automatically enabled.
There doesn't seem to be much discussion about downloading the source. I don't think we can expect users to manually download the rust-src component. At a minimum, I think it would need to be made a required component (or just ship with rustc). This could also be a breaking change for non-rustup users. This would also be a breaking change for existing vendored users. This could also be a breaking change for users with partial mirrors of crates.io.
I'm a bit worried about the goal of being able to match the pre-built standard library is really high. I think that is going to be very difficult.
For example, I don't see much discussion about optimized compiler-builtins and such.
I don't see much discussion in here about compiler-rt or profiler support.
I'm unclear on how this would work with RUSTFLAGS. Cargo has no knowledge of what those are, and so if the user sets RUSTFLAGS, that seems like it could run into some significant conflicts (particularly with some of the "target modifier" flags that were previously mentioned).
GitHub's UI is choking on this large PR and the large number of comments. I can live with it for now, but I think if the intent is to post this in its entirety on the RFC repo, it could make things difficult. That was also partly why we started the repo at https://github.com/rust-lang/wg-cargo-std-aware/, so that individual threads could be split into issues and we could discuss and come to resolution on them, and to write separate documents and work on them somewhat independently.
text/0000-build-std.md
Outdated
| [rationale-breakage]: #why-permit-breakage-of-nightly-build-std-users-using-tier-three-targets | ||
|
|
||
| As this use case would previously have been using the unstable | ||
| `-Zbuild-std=core`, this user must be on a nightly toolchain. This proposal |
There was a problem hiding this comment.
I don't entirely agree with this. I believe there are people building rustc with their own targets, and shipping it themselves, thus having a "stable" version of these targets.
There was a problem hiding this comment.
I don't think these targets are "stable" in the sense that the Rust project provides any guarantees for them. If a user is maintaining a downstream fork then they're taking on responsibility to keep that working. I don't think it's especially reasonable to constrain what we can do to save those users from having to fix their fork.
I appreciate it! This is absolutely the type of feedback that I'm looking for. It's late here at the moment so I don't have time to reply to your individual points but will try to do so tomorrow. |
text/0000-build-std.md
Outdated
| ### Why not allow profile overrides to override the standard library's dependencies? | ||
| [rationale-why-not-override-std-deps]: #why-not-allow-profile-overrides-to-override-the-standard-librarys-dependencies | ||
|
|
||
| The dependencies of the standard library are an implementation detail and ought | ||
| not be observable to the user. |
There was a problem hiding this comment.
Perhaps as a future extension this could be allowed on nightly.
text/0000-build-std.md
Outdated
| [`cargo add`][cargo-add] will add `core`, `alloc` or `std` explicitly to the | ||
| manifest if invoked with those crate names: |
There was a problem hiding this comment.
Hmm, interesting. Perhaps it should also enforce the nightly toolchain when trying to add it with features.
|
(didn't write a summary comment in posting the review, so will do so here) I'd like to echo ehuss in thanking the people who put in the work to make this happen. I'm happy that a much more comprehensive proposal is being developed. I share some concerns though.
Note that there's a perspective of ecosystem churn here: if all
Well, I think there is inherent complexity in build-std. On the Zulip thread I proposed seeking team approval for an experiment to better inform the design (this was how the const traits feature evolved), where the code experimental support could be ripped out/completely refactored at any time if the team doesn't feel like the direction is correct (a sort of an initial nothing is guaranteed approach), it depends on how people feel about a similar approach here.
More specifically for me, typing review comments for the diff becomes really laggy. I heard that GitHub is trying to improve stuff on that front though. I still prefer the RFC+GitHub review system for threaded discussion over issue, because I feel like the latter is more fragmented. |
text/0000-build-std.md
Outdated
| needing to keep track of whether `std` is supported is necessary. However, it is | ||
| not obvious why keeping track of whether `alloc` and `core` are supported | ||
| individually is necessary: | ||
|
|
||
| Most targets will support `core`. `core` would only be set to `false` for very | ||
| experimental targets which do not support build-std at all. `alloc` would be set | ||
| to `false` for those targets that do not support allocation. |
There was a problem hiding this comment.
A more thorough discussion on what it means for a target to have standard_library_support seems missing.
I am particularly interested in alloc because that will probably be the most contentious part. The current example in the specific section states x86_64-unknown-none as having both core and alloc support but mipsel-sony-psx as only having core support; However the rustc doc on the mipsel-sony-psx target explicitly guides users of the target to build alloc:
cargo build -Zbuild-std=core,alloc --target mipsel-sony-psx
Furthermore, it is unclear why x86_64-unknown-none is considered as support[ing] allocation. The standard procedure with build-std with that target is to bring-your-own-allocator. There needs to be a crate that specifies the global allocator.
There was a problem hiding this comment.
mipsel-sony-psx was just a target I chose at random assuming it didn't have alloc support, so that's just an oversight.
I used x86_64-unknown-none as an example because it's my understanding that we ship liballoc.rlib for that target today as a pre-built artifact.
I think it's okay to permit the user to build alloc and eventually have compilation fail for lack of a global allocator. I don't think that's much different from a core-only crate not including a panic handler crate. I'm not sure if it is a responsibility of build-std/Cargo to ensure that circumstance doesn't happen.
There was a problem hiding this comment.
I want to assume that ability to build core implies an ability to build alloc. That for me eliminates the need to separate alloc from core.
The goals section also mention an ability for people to build standard library crates that a target normally doesn't support (e.g. only using some features from std), I wonder how this ties in with a target enabling what kinds of stdlib crates downstream people can build? Maybe there should be an override option for nightly users?
text/0000-build-std.md
Outdated
| 1. **Re-building the standard library with different codegen flags or profile** | ||
| ([wg-cargo-std-aware#2]) | ||
|
|
||
| - Embedded users need to optimise aggressively for size, due to the limited | ||
| space available on their target platforms, which can be achieved in Cargo by | ||
| setting `opt-level = s/z` and `panic = "abort"` in their profile. However, | ||
| these settings will not apply to the pre-built standard library | ||
| - Similarly, when deploying to known environments, use of `target-cpu` or | ||
| `target-feature` can improve the performance of code generation or allow the | ||
| use of newer hardware features than the target's baseline provides. As above, | ||
| these configuration will not apply to the pre-built standard library | ||
| - While the pre-built standard library is built to support debugging without | ||
| compromising size and performance by setting `debuginfo=1`, this isn't | ||
| ideal, and building the standard library with the dev profile would provide | ||
| a better experience | ||
|
|
||
| 2. **Unblock stabilisation of ABI-modifying compiler flags** | ||
|
|
||
| - Any compiler flags which change the ABI cannot currently be stabilised as they | ||
| would immediately mismatch with the pre-built standard library | ||
| - Without an ability to rebuild the standard library using these flags, it is | ||
| impossible to use them effectively and safely if stabilised | ||
| - ABI-modifying flags are designated as target modifiers ([rfcs#3716]/[rust#136966]) | ||
| and require that the same value for the flag is passed to all compilation units | ||
| - Flags which need to be set across the entire crate graph to uphold some | ||
| property (i.e. enhanced security) are also target modifiers | ||
| - For example: sanitizers, control flow integriy, `-Zfixed-x18`, etc |
There was a problem hiding this comment.
Question: How important/negotiable are these goals if they will significantly increase the contentiousness of this proposal?
Stabilizing ABI-modifying flags or otherwise custom build options, as described here, might lead to problems regarding what guarantees, especially the libs team can provide. It's hard to know nor test that all possible combinations of build options/flags/targets work, and it seems like a particularly hard goal.
Making this available on nightly is at least more realistic. But I feel like if we wanted to ship anything to stable, it might be better to only focus on building tier-3 targets for a stable MVP.
There was a problem hiding this comment.
I think this highlights something the RFC is missing - what we propose the project's stability guarantees ought to be for what build-std enables.
This is somewhere that I expect I might diverge from other teams, but I'm absolutely comfortable with build-std enabling users to do things that might not work or build, and for the mechanisms that enable that to be stable, as long as we are clear about what guarantees we do and do not provide.
I think if we only want to support configurations on stable that are well-tested and guaranteed-to-work then build-std will be much less useful feature for the vast majority of users.
|
I'll reply to the inline comments one-by-one as I get around to addressing them. This discussion has reminded me that I think another missing section we have is a proposed stability guarantees for build-std - what we should and shouldn't promise will always work. build-std will inherently let the user build configurations that haven't been tested and might not always work, so we should be clear about what we're promising.
Which parts of the scope would you suggest cutting? I'm open to dropping parts if we can. I figure that the core of build-std is "what do we rebuild and when" and I've tried to keep it focused on providing a good answer to those questions and only including things which are necessary in support of those. I didn't want to propose anything that I felt was a worse-than-it-could-be solution to those questions just to avoid tricky decisions or complexity though. I didn't think I had too many breaking changes in the RFC, at least not to stable users. I expect that this wouldn't just work for existing users of build-std on nightly but I don't think that's especially important. I don't mind breaking anyone choosing to use nightly features, if there's a bit of churn there to get those people onto a feature that has the possibility of being stabilised then I think that's absolutely worth it. I should include a section/appendix on known breaking changes being proposed - so if there are others that you spot that I have missed, please let me know. I expect that the approach we'll take with this is to try and propose something reasonable and achievable, and then spent a decent chunk of time asking for feedback from a growing set of project members that we invite to give feedback, so that we can make sure that their concerns are addressed and that they're on-board with what we're proposing. By the time we actually open it as an RFC, I'd want many of the affected parties to already have seen the RFC and be largely on-board with it.
I'm absolutely open to doing this. This isn't described in the RFC and should be, but one of my motivations for the
I think build-std being as-transparent-as-possible is one of the key aspects of the RFC that I'd like to keep. I think there are aspects of build-std, like enabling features, that are relatively easy to separate and not address in this RFC. Unless we can argue that a detail can be postponed and doesn't need to be addressed now, or that an alternative is better from a technical/user experience perspective, I'd like to try and pursue as close to the ideal for build-std as we can. I don't think that's incompatible with trying to keep the scope small, I'm happy to cut as many extraneous parts as we can, but with what remains - like how-build-std is enabled - I'd like to go for a solution as close to the ideal as possible. More concretely, I think what we're proposing in the current draft is pretty close to an opt-in, we could default to It might be that you disagree that a transparent build-std is the wrong user experience in general, not just because it's more complex to propose/implement, and if that's the case, I'd love to know more about why that is. Matching the pre-built standard library is certainly more difficult, and it's the part of the RFC that I have the least confidence in at the moment. I think it's a worthwhile goal, but I am happy to compromise on it if we can't work out a mechanism that seems feasible to do it.
I hadn't considered requiring rustup users to download That said, I'd be happy to write that While we haven't discussed the non-rustup use-case specifically in the RFC, it was my intention that by documenting the path in the sysroot that sources are expected to be found, that would be sufficient for non-rustup users (e.g. distro packages) to know how to set up their toolchain such that build-std could work. I don't think that is a breaking change for those users though: if build-std defaulted to I hadn't considered existing vendored users or those with partial mirrors of crates.io. I'll make sure we consider that and mention it in the RFC.
I've noted this myself in one of the comments above, these are gaps that we just need to address.
I'll mention this in the RFC, but I expected that like today, Cargo would not look at
GitHub is struggling, but I'm not sure how better to do this - a repo runs the risk of discussions/information becoming very disjoint and hard to follow, requiring an effort like this to tie all the threads back together again. I could split the RFC up into different files in a
I think this is true, there would be churn. I assumed that this churn would largely affect users already on nightly using build-std, rather than no-std crates being used on stable. I think that's fine. It's not great, sure, but if that part of the Rust ecosystem needs to update their crates so they can use a build-std that has a chance of being stabilised, I think that's fine and worth it. It's possible there are things we've missed here for targets like |
|
btw, it would be nice to use |
We'll squash all the history before this is actually proposed, but I'll make sure you're added to the acknowledgements section. |
|
Just to review my proposal of reducing scope:
Does that make sense? |
Assorted review updates
Removed implicit direct deps as it was unnecessary - implicit deps still includes core & alloc as before.
Plus various review updates
| The default behaviour of `compiler_builtins` can and should be improved to allow | ||
| `build-std` to match the prebuilt standard library as much as possible. The | ||
| `compiler-rt` sources should be distributed with `rust-src` (as `libunwind` is | ||
| today). By default `compiler_builtin`'s `build.rs` should make a best-effort |
There was a problem hiding this comment.
I think this section should be removed and we have to accept that by default build-std will often perform worse that prebuilt for now.
The issue is that when a target-modifier is changed, CFLAGS (which should be advertised as being respected) needs to be changed at the same time. If it isn't then the best case scenario for the user is a big meaty linker error and the worst case is some kind of CFI not being fully applied. I'm not sure how otherwise to make the best-effort approach work.
| re-builds the standard library crates which rustc then uses instead of the | ||
| pre-built standard library from the sysroot. | ||
|
|
||
| `-Zbuild-std` builds `std` by default. `test` is also built if `std` is being |
There was a problem hiding this comment.
To clarify a minor point here, the default is dynamic. If rustc indicates that the target does not support std, then it only builds core. It uses the std field from --print=target-spec-json (added in rust-lang/rust#122305).
|
As discussed in the sync call, we've rewritten a bunch of this to present the RFC in stages following your feedback. Much of the existing feedback is rendered irrelevant and we've tried to address everything else, so re-opening this in #2 to avoid having to uncollapse GitHub threads. We're still to discuss interactions of per-pkg-target in the RFC, but that'll be added by @adamgemmell soon. |
Rendered