Support overriding initial rustc and cargo paths #102266
Conversation
This restores functionality broken by rust-lang#98483. Unfortunately, it doesn't add a test to verify this works, but in this case we notice pretty quickly as perf uses this functionality and so reports breakage immediately after merging.
rustbot
added
the
T-bootstrap
rust-highfive
added
the
S-waiting-on-review
| @@ -912,10 +910,14 @@ impl Config { | |||
| config.out = crate::util::absolute(&config.out); | |||
| } | |||
|
|
|||
| if !has_custom_rustc && !config.initial_rustc.starts_with(&config.out) { | |||
There was a problem hiding this comment.
I don't really understand what this check was trying to accomplish -- it seems pretty odd to me that config.out matters here. The new logic just trusts build.rustc or build.cargo, and falls back to the stage0 path as before.
There was a problem hiding this comment.
I don't quite understand why this behavior is different than before; don't we set initial_rustc on line 906 above? I think the bug is that I forgot the same check for initial_cargo.
That said, I agree the new logic is simpler and easier to understand, so 👍 for using it for initial_rustc as well.
I don't really understand what this check was trying to accomplish -- it seems pretty odd to me that config.out matters
I was trying to be conservative and only change the behavior when bootstrap was downloaded from CI (i.e. config.out wasn't a parent of the RUSTC env variable bootstrap was built with). But I think your approach is fine too and easier to maintain - ideally we wouldn't set RUSTC in the build script at all.
There was a problem hiding this comment.
I don't think the build-script set RUSTC matters at all at this point? It's not getting read by config.rs anyway, so I would hope it doesn't :)
There was a problem hiding this comment.
Oh you're right, I'd forgotten I'd completely removed that: https://github.com/rust-lang/rust/pull/98483/files#diff-7eddc76f1be9eca2599a9ae58c65ffe247fbdff9b02ef687439894cab9afe749L781
Yes, this is absolutely the right fix then if there's no default for initial_rustc. I think we can go ahead and remove bootstrap's build script too, I'll make a follow-up PR for that :)
| @@ -912,10 +910,14 @@ impl Config { | |||
| config.out = crate::util::absolute(&config.out); | |||
| } | |||
|
|
|||
| if !has_custom_rustc && !config.initial_rustc.starts_with(&config.out) { | |||
There was a problem hiding this comment.
I don't quite understand why this behavior is different than before; don't we set initial_rustc on line 906 above? I think the bug is that I forgot the same check for initial_cargo.
That said, I agree the new logic is simpler and easier to understand, so 👍 for using it for initial_rustc as well.
I don't really understand what this check was trying to accomplish -- it seems pretty odd to me that config.out matters
I was trying to be conservative and only change the behavior when bootstrap was downloaded from CI (i.e. config.out wasn't a parent of the RUSTC env variable bootstrap was built with). But I think your approach is fine too and easier to maintain - ideally we wouldn't set RUSTC in the build script at all.
|
@bors r+ p=25 (fixes perf) |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (f3fafbb): 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)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Footnotes |
…k-Simulacrum Don't set RUSTC in the bootstrap build script We no longer use this for anything since https://github.com/rust-lang/rust/pull/98483/files#diff-7eddc76f1be9eca2599a9ae58c65ffe247fbdff9b02ef687439894cab9afe749L781. Remove it, so that we spuriously rebuild bootstrap fewer times on Windows (where PATH changes often). Helps with rust-lang#92369. cc rust-lang#102266 r? `@Mark-Simulacrum`
…k-Simulacrum Don't set RUSTC in the bootstrap build script We no longer use this for anything since https://github.com/rust-lang/rust/pull/98483/files#diff-7eddc76f1be9eca2599a9ae58c65ffe247fbdff9b02ef687439894cab9afe749L781. Remove it, so that we spuriously rebuild bootstrap fewer times on Windows (where PATH changes often). Helps with rust-lang#92369. cc rust-lang#102266 r? ``@Mark-Simulacrum``
This restores functionality broken by #98483. Unfortunately, it doesn't add a test to verify this works, but in this case we notice pretty quickly as perf uses this functionality and so reports breakage immediately after merging.
r? @jyn514
cc https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/rustc.20and.20cargo.20option.20broken.20in.20config.2Etoml, https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/Rustc.20benchmark.20broken