Support JSON target specs in bootstrap#152677
Conversation
|
r? @jieyouxu rustbot has assigned @jieyouxu. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
cc @davidtwco I think this should either be backported to beta, or #150151 reverted on beta. Otherwise 1.94 will not be buildable. I might recommend reverting, so that all of the destabilization changes are in 1.95. |
69ece54 to
24918a5
Compare
JSON target specs were destabilized in rust-lang#150151 and rust-lang#151534. However, this broke trying to build rustc itself with a JSON target spec. This is because in a few places bootstrap is manually calling `rustc` without the ability for the user to provide additional flags (primarily, `-Zunstable-options` to enable JSON targets). There's a few different ways to fix this. One would be to change these calls to `rustc` to include flags provided by the user (such as `RUSTFLAGS_NOT_BOOTSTRAP`). Just to keep things simple, this PR proposes to just unconditionally pass `-Zunstable-options`. Another consideration here is how maintainable this is. A possible improvement here would be to have a function somewhere (BootstrapCommand, TargetSelection, free function) that would handle appropriately adding the `--target` flag. For example, that's what cargo does in [`CompileKind::add_target_arg`](https://github.com/rust-lang/cargo/blob/592058c7ce08a2ba2628b01e7dc4ce1e72b6bdff/src/cargo/core/compiler/compile_kind.rs#L144-L154). I have only tested building the compiler and a few tools like rustdoc. I have not tested doing things like building other tools, running tests, etc. This would be much easier if there was a Docker image for testing the use case of building rustc with a custom target spec (and even better if that ran in CI). After the next beta branch, using target JSON specs will become more cumbersome because target specs with the `.json` extension will now require passing `-Zjson-target-spec` (from rust-lang/cargo#16557). This does not affect target specs without the `.json` extension (such as those from RUST_TARGET_PATH). From my testing, it should be sufficient to pass `CARGOFLAGS_NOT_BOOTSTRAP="-Zjson-target-spec"`. I think that should be fine, since this is not a particularly common use case AFAIK. We could extend bootstrap to auto-detect if the target is a file path, and pass `-Zjson-target-spec` appropriately. I tried something similar in f0bdd35, which could be adapted if desired. It would be nice if all of this is documented somewhere. https://rustc-dev-guide.rust-lang.org/building/new-target.html does not really say how to build the compiler with a custom json target. Fixes rust-lang#151729
24918a5 to
b4e645f
Compare
|
@jieyouxu I realized I made a mistake. This was missing RUSTC_BOOTSTRAP. This would otherwise fail when the channel is set to stable. Do the new changes look correct to you? |
This reverts rust-lang#150151 in order to deal with rust-lang#151729 where the destabilization caused a problem with building rustc itself with JSON target specs. There's a fix at rust-lang#152677, but we would prefer to not backport that, and instead give ourselves more time to work out the kinks. Also, the destabilization was incomplete, and the rest of the changes are in 1.95 (rust-lang#151534 and rust-lang/cargo#16557), so it would be nice to keep all the changes together in one release. This reverts commit a89683d, reversing changes made to 2f1bd3f.
|
Posted a beta revert at #152721. |
|
@bors r=davidtwco,jieyouxu |
This comment has been minimized.
This comment has been minimized.
[beta] Revert destabilise target-spec-json This reverts #150151 in order to deal with #151729 where the destabilization caused a problem with building rustc itself with JSON target specs. There's a fix at #152677, but we would prefer to not backport that, and instead give ourselves more time to work out the kinks. Also, the destabilization was incomplete, and the rest of the changes are in 1.95 (#151534 and rust-lang/cargo#16557), so it would be nice to keep all the changes together in one release. This reverts commit a89683d, reversing changes made to 2f1bd3f.
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 dfbfbf7 (parent) -> 8387095 (this PR) Test differencesShow 2 test diffs2 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 8387095803f21a256a9a772ac1f9b41ed4d5aa0a --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 (8387095): 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.5%, secondary 5.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.2%, secondary -2.9%)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: 482.346s -> 480.396s (-0.40%) |
[beta] Revert destabilise target-spec-json This reverts #150151 in order to deal with #151729 where the destabilization caused a problem with building rustc itself with JSON target specs. There's a fix at #152677, but we would prefer to not backport that, and instead give ourselves more time to work out the kinks. Also, the destabilization was incomplete, and the rest of the changes are in 1.95 (#151534 and rust-lang/cargo#16557), so it would be nice to keep all the changes together in one release. This reverts commit a89683d, reversing changes made to 2f1bd3f.
[beta] Revert destabilise target-spec-json This reverts #150151 in order to deal with #151729 where the destabilization caused a problem with building rustc itself with JSON target specs. There's a fix at #152677, but we would prefer to not backport that, and instead give ourselves more time to work out the kinks. Also, the destabilization was incomplete, and the rest of the changes are in 1.95 (#151534 and rust-lang/cargo#16557), so it would be nice to keep all the changes together in one release. This reverts commit a89683d, reversing changes made to 2f1bd3f.
JSON target specs were destabilized in #150151 and #151534. However, this broke trying to build rustc itself with a JSON target spec. This is because in a few places bootstrap is manually calling
rustcwithout the ability for the user to provide additional flags (primarily,-Zunstable-optionsto enable JSON targets).There's a few different ways to fix this. One would be to change these calls to
rustcto include flags provided by the user (such asRUSTFLAGS_NOT_BOOTSTRAP). Just to keep things simple, this PR proposes to just unconditionally pass-Zunstable-options.Another consideration here is how maintainable this is. A possible improvement here would be to have a function somewhere (BootstrapCommand, TargetSelection, free function) that would handle appropriately adding the
--targetflag. For example, that's what cargo does inCompileKind::add_target_arg.I have only tested building the compiler and a few tools like rustdoc. I have not tested doing things like building other tools, running tests, etc.
This would be much easier if there was a Docker image for testing the use case of building rustc with a custom target spec (and even better if that ran in CI).
After the next beta branch, using target JSON specs will become more cumbersome because target specs with the
.jsonextension will now require passing-Zjson-target-spec(fromrust-lang/cargo#16557). This does not affect target specs without the
.jsonextension (such as those from RUST_TARGET_PATH). From my testing, it should be sufficient to passCARGOFLAGS_NOT_BOOTSTRAP="-Zjson-target-spec". I think that should be fine, since this is not a particularly common use case AFAIK. We could extend bootstrap to auto-detect if the target is a file path, and pass-Zjson-target-specappropriately. I tried something similar in ehuss@f0bdd35, which could be adapted if desired.It would be nice if all of this is documented somewhere. https://rustc-dev-guide.rust-lang.org/building/new-target.html does not really say how to build the compiler with a custom json target.
Fixes #151729