-
Notifications
You must be signed in to change notification settings - Fork 0
build-std: second iteration #2
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
Conversation
fbe72b3
to
507b9c5
Compare
Co-authored-by: Adam Gemmell <[email protected]>
507b9c5
to
599ddbf
Compare
599ddbf
to
04e538c
Compare
17413d4
to
4f811dd
Compare
4f811dd
to
660ce5b
Compare
Co-authored-by: Adam Gemmell <[email protected]>
I did some experiments with PGO:
|
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 left some pretty minor comments. In general, this was well structured and highly legible given the very technical nature. Excellent work!
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.
Just some minor comments on stage-1b.
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.
Comments on stage 2.
Pre-built available? | User's profile | Target modifiers changed? | Standard library re-built? | ||
-------------------- | -------------- | ------------------------- | -------------------------- | ||
No | N/A | N/A | Yes, std's `release` | ||
Yes | `dev` | Unchanged | Yes, std's `release` |
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 quite following this row. Why would dev
trigger a rebuild if it is always building the standard library with release settings? I would expect the pre-built to be sufficient if it isn't building std with debug settings.
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 was just a mistake in the table that we hadn't fixed in the document yet - #2 (comment)
## Why add compatibility checks in rustc to support build-std? | ||
[rationale-rustc-support]: #why-add-compatibility-checks-in-rustc-to-support-build-std | ||
|
||
Without support from rustc, Cargo would need to assume that the pre-built | ||
standard library's configuration is entirely configured in its Cargo profile and | ||
would need to compare the profile of the standard library with the profile of | ||
the user's crate and know which rustc flags are target modifiers. |
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 also not clear here. If the profile gains the ability to set target-modifiers, wouldn't cargo be able to directly compare those target modifier values?
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 avoids Cargo needing to know which flags are target modifiers and which aren't (or looking in RUSTFLAGS
for target modifiers) - Cargo can just pass every flag it would pass to the build to this compatibility check invocation and be told if it needs to rebuild the standard library or not. Elaborated on this in the RFC.
> $ rustc -Zreg-struct-return core/lib.rs -o libcore.rlib | ||
> $ rustc --emit compatibility $cargo_flags libcore.rlib | ||
> error: mixing `-Zreg-struct-return` will cause an ABI mismatch in crate `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'm a little worried about the challenges here since cargo may not necessarily know where libcore.rlib is.
As a side note, this will need to be cached. I think our existing caching code might handle it, but I'm not sure.
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.
rustc will always know where it is, so we could provide some alternative interface --compatibility-with=core
or something like that if required
Yes | `release` | Changed | Yes, std's `release` | ||
|
||
*It is assumed that changing a target modifier would be part of Cargo profiles, | ||
hence why there is no row for "target modifiers changed, profile unchanged".* |
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 having difficulty understanding this sentence in combination with the sentence below about per-target profiles not being available.
Is this stage 2 implying that it will also be adding target modifiers to the profile? Or is that a hypothetical future? If that is a new proposal for this stage, there probably should be a relatively hefty section describing the addition of a new profile field.
Are target modifiers ignored on targets that don't support that modifier?
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 assumes that target modifiers will eventually be exposed through Cargo's [profile]
options, like any other codegen flag. It doesn't propose adding any of them to that today, they're all still unstable flags in rustc anyway.
Are target modifiers ignored on targets that don't support that modifier?
I think we normally decide this on a per-flag basis, not on the basis of whether a flag is a target modifier or not, so it depends.
As PR has gotten quite large again, so I've responded to all the feedback and incorporated it into the document, squashed all the changes and opened another PR that we can use for the final rounds of feedback before we open this in rust-lang/rfcs - see #4 |
Supercedes #1. Intended for discussion with invited reviewers until we're happy enough to post publicly. Feel free to discuss on #project-goals/2025h1/build-std.
Please don't leave feedback on this unless you've been invited to. We'll invite more project members and teams to participate over time and eventually post this publicly for community feedback. With such a large proposal, this sort of staged rollout is the only way we can manage the feedback and evolve the document. If you aren't invited and post anyway, I reserve the right to just delete comments.
Rendered