-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Move bootstrap configuration to library workspace #149514
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
Move bootstrap configuration to library workspace #149514
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use |
Doesn't rustc default to |
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.
Left some comments.
In general the changes made so far look relatively minimal and simple. However, currently just moving things to the stdlib's Cargo.toml doesn't change the fact that bootstrap can arbitrarily override them (and likely it does override a bunch of them), in CI dist jobs that can have arbitrary bootstrap configuration enabled.
If we wanted to ensure that this won't happen, we should have some sanity check that will simply deny configuring any cargo overrides and rustflags for the stdlib when we are doing a dist build on CI.
I don't know. Also somehow this mention of me never subscribed me to this PR, which is weird. |
The best I can tell this is only for linux, and any riscv target that isn't based on linux will default to no uwtables if the panic strategy isn't unwind. What we could do is enable uwtables by default for all riscv targets in the target spec, which would 1. always build rustc on riscv hosts with uwtables (which would only change something for targets with host tools and panic abort and I suspect there aren't any) and 2. always build user code with uwtables by default, which can be overridden with I don't know if that's desirable and don't really want to force the change while we don't have to - my point that profiles aren't expressive enough still stands as we might want to do a per-target override like this in the future. We might have to read the library workspace's cargo config and I don't know how reasonable that is yet. |
Thanks - yes, I think I still need to do a proper scan of the dist jobs to minimise what's set there. I'm not sure how to implement a sanity check given that bootstrap will always have to pass some configuration that can't be expressed in profiles. Is it enough to check that no CARGO_PROFILE* env vars are set? |
0bae09c to
1e439c3
Compare
This comment has been minimized.
This comment has been minimized.
|
I think that we don't have to deal with that in this PR; moving as much stuff as possible to the library workspace is already an improvement. Adding sanity checks that ensure that bootstrap doesn't do anything extra (which Cargo couldn't replicate) can be one of follow-up steps. |
1e439c3 to
ffce9ca
Compare
This comment has been minimized.
This comment has been minimized.
Looks like there's two ways to do this - a bit of a hack, which disables setting profile values (and maybe some rustflags) with an |
|
We definitely don't want to do any CI-specific checks in bootstrap, unless it's the only way to solve something. It's tricky, because I assume that some people will want to build their own std with bootstrap, with overridden settings, without the "build-std-compatibility" mode. I almost wonder if this "build-std-compatible" stdlib should be a separate bootstrap step or |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…, r=<try> Move bootstrap configuration to library workspace
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.
Left some comments and kicked off a try build, so that we can test perf. and also if -Zbuild-std still works.
…l based on the default values of rust.debug[...] keys
ffce9ca to
368504e
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
Seems to work 👍 |
|
Finished benchmarking commit (ef62af0): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary -3.3%, secondary -2.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 3.5%, secondary -4.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 475.617s -> 475.457s (-0.03%) |
Hopefully the only noticeable change would be from the test crate build probe as -Zbuild-std right now as it doesn't respect any configuration in the library workspace. |
|
I thought about the probe a bit and it seems to me that on nightly it should work the same as before, so hopefully it still works :) |
|
Oh, true, the fix only changes things after stabilisation, perhaps a bit premature 😂 Those max RSS/cycle count changes seem quite high, though only for a couple benchmarks. Is that within expectations? |
|
That's almost certainly noise. The binary size results looked more "interesting", but it's possible that PGO/BOLT perturbations change the size of the stdlib, which in turn changes the sizes of the benchmarks. To be sure, it would be useful to check the libstd before/after ( |
|
I think it's ok: https://try.diffoscope.org/gbbssutugsab.html The online version of the tool doesn't show whole diffs, but I've checked with readelf locally and for all debug sections an equal amount of lines were added vs removed. |
|
Ok, good, thanks. One thing that this changes is that it is not really possible anymore to build stdlib without optimizations, but I don't know if anyone actually used that. Let's see what happens on nightly. @bors r+ rollup=never |
|
Will make a note to fix that, thanks |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing daa90b9 (parent) -> eda76d9 (this PR) Test differencesShow 4 test diffs4 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard eda76d9d1d133effbf7facb28168fd78d75fd434 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (eda76d9): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 2.2%, secondary -3.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 470.203s -> 471.204s (0.21%) |
rust-lang/rust#149514 added a build.rs to libtest, which affected how Cargo print status message (build.rs built first so Cargo printed `Compiling` first)
### What does this PR try to resolve? rust-lang/rust#149514 added a build.rs to libtest, which affected how Cargo print status message (build.rs was scheduled first because it has one dependent) ### How to test and review this PR? See <https://github.com/rust-lang/cargo/actions/runs/21232930264/job/61094884597?pr=16506> ``` thread 'basic' (9963) panicked at tests/build-std/main.rs:179:10: ---- expected: tests/build-std/main.rs:167:27 ++++ actual: stderr 1 1 | [COMPILING] [..] 2 - ... 3 - [COMPILING] test v0.0.0 ([..]) 2 + [COMPILING] rustc-std-workspace-std v1.99.0 (/home/runner/.rustup/toolchains/nightly-[HOST_TARGET]/lib/rustlib/src/rust/library/rustc-std-workspace-std) 3 + [COMPILING] getopts v0.2.24 4 4 | [COMPILING] foo v0.0.1 ([ROOT]/foo) 5 5 | [FINISHED] `test` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s 6 6 | [RUNNING] unittests src/lib.rs (target/[HOST_TARGET]/debug/deps/foo-[HASH]) 7 7 | [RUNNING] unittests src/main.rs (target/[HOST_TARGET]/debug/deps/foo-[HASH]) 8 8 | [RUNNING] tests/smoke.rs (target/[HOST_TARGET]/debug/deps/smoke-[HASH]) 9 9 | [DOCTEST] foo ```
### What does this PR try to resolve? rust-lang/rust#149514 added a build.rs to libtest, which affected how Cargo print status message (build.rs was scheduled first because it has one dependent) ### How to test and review this PR? See <https://github.com/rust-lang/cargo/actions/runs/21232930264/job/61094884597?pr=16506> ``` thread 'basic' (9963) panicked at tests/build-std/main.rs:179:10: ---- expected: tests/build-std/main.rs:167:27 ++++ actual: stderr 1 1 | [COMPILING] [..] 2 - ... 3 - [COMPILING] test v0.0.0 ([..]) 2 + [COMPILING] rustc-std-workspace-std v1.99.0 (/home/runner/.rustup/toolchains/nightly-[HOST_TARGET]/lib/rustlib/src/rust/library/rustc-std-workspace-std) 3 + [COMPILING] getopts v0.2.24 4 4 | [COMPILING] foo v0.0.1 ([ROOT]/foo) 5 5 | [FINISHED] `test` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s 6 6 | [RUNNING] unittests src/lib.rs (target/[HOST_TARGET]/debug/deps/foo-[HASH]) 7 7 | [RUNNING] unittests src/main.rs (target/[HOST_TARGET]/debug/deps/foo-[HASH]) 8 8 | [RUNNING] tests/smoke.rs (target/[HOST_TARGET]/debug/deps/smoke-[HASH]) 9 9 | [DOCTEST] foo ```
This creates a new "dist" profile in the standard library which contains configuration for the distributed std artifacts previously contained in bootstrap, in order for a future build-std implementation to use. bootstrap.toml settings continue to override these defaults, as would any RUSTFLAGS provided. I've left some cargo features driven by bootstrap for a future patch.
Unfortunately, profiles aren't expressive enough to express per-target overrides, so this risc-v example was not able to be moved across. This could go in its own profile which Cargo would have to know to use, and then the panic-abort rustflags overrides would need duplicating again. Doesn't seem like a sustainable solution as a couple similar overrides would explode the number of lines here.
We could use a cargo config in the library workspace for this, but this then would have to be respected by Cargo's build-std implementation and I'm not yet sure about the tradeoffs there.
This patch also introduces a build probe to deal with the test crate's stability which is obviously not ideal, I'm open to other solutions here or can back that change out for now if anyone prefers.
cc @Mark-Simulacrum rust-lang/rfcs#3874