Fix debuginfo compression in bootstrap#158169
Conversation
|
Thanks! @bors r+ |
This comment has been minimized.
This comment has been minimized.
Fix cc rs flag if supported Found in https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/weird.20code.20in.20fill_target_compiler/with/604588655. When `cc-rs` checks if a compiler flag is supported, it tries to read `out_dir` to generate the source file in. However, when this option is not set, then the check silently fails and the flag is considered to be unsupported. Since we didn't set `out_dir`, all such added flags were simply ignored. Normally, it reads `OUT_DIR`, which is fine if `cc-rs` is used within a build script. But if it is used elsewhere, then this is a pretty gnarly footgun in `cc-rs`, because the failure is [completely silent](https://github.com/rust-lang/cc-rs/blob/main/src/lib.rs#L1483). CC @madsmtm or @NobodyXu in case you have thoughts on that :) r? @bjorn3
This comment has been minimized.
This comment has been minimized.
|
💔 Test for b946733 failed: CI. Failed job:
|
|
@bors try jobs=x86_64-gnu-distcheck |
This comment has been minimized.
This comment has been minimized.
Fix cc rs flag if supported try-job: x86_64-gnu-distcheck
|
Thanks, I've opened an issue #1765, I think this is something we should fix. We could use a tempfile instead for it |
I think you meant rust-lang/cc-rs#1765 😉 |
|
@bors r=bjorn3 Let's try again. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Fix debuginfo compression in bootstrap try-job: dist-x86_64-msvc try-job: dist-x86_64-apple try-job: dist-x86_64-linux
|
💥 Test timed out after |
|
The Apple timeout looks spurious. I enabled the compression only on Linux-like OSes, for now. @rustbot ready |
|
@bors r+ |
…, r=bjorn3 Fix debuginfo compression in bootstrap Found through https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/weird.20code.20in.20fill_target_compiler/with/604588655. This PR solves a few issues with debuginfo compression in bootstrap. The main issue was that debuginfo compression through the `-gz` flag wasn't enabled, by mistake. When `cc-rs` checks if a compiler flag is supported, it tries to read `out_dir` to generate the source file in. However, when this option is not set, then the check silently fails and the flag is considered to be unsupported. Since we didn't set `out_dir`, all such added flags were simply ignored. Normally, it reads `OUT_DIR`, which is fine if `cc-rs` is used within a build script. But if it is used elsewhere, then this is a pretty gnarly footgun in `cc-rs`, because the failure is [completely silent](https://github.com/rust-lang/cc-rs/blob/main/src/lib.rs#L1483). CC @madsmtm or @NobodyXu in case you have thoughts on that :) After that was fixed, I had to change the used compression flag from `-gz` to `--compress-debug-sections`, to support both BFD and LLD linkers. And to solve it properly, and allow `dist` CI jobs to configure debuginfo compression for all targets that they build, I added a new bootstrap option to configure debuginfo compression. I wanted the flag to be configurable both globally and per target. At the same time, the flag applies both to C/C++ and Rust. We currently don't have such config area in `bootstrap.toml`, and `[build]` didn't seem like the right place, so I shoved it into `[rust]` (while documenting that it also applies to C/C++). r? @bjorn3 try-job: dist-x86_64-msvc try-job: dist-x86_64-apple try-job: dist-x86_64-linux
…uwer Rollup of 5 pull requests Successful merges: - #158169 (Fix debuginfo compression in bootstrap) - #158613 (Fix getrandom fallback test on platforms with `panic=abort`) - #158620 (Remove skip_norm_w/i/p().def_id with a helper) - #158633 (Remove unnecessary `Clone` derives on resolver types) - #158634 (Add missing `needs_drop` check to `DroplessArena`.)
…, r=bjorn3 Fix debuginfo compression in bootstrap Found through https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/weird.20code.20in.20fill_target_compiler/with/604588655. This PR solves a few issues with debuginfo compression in bootstrap. The main issue was that debuginfo compression through the `-gz` flag wasn't enabled, by mistake. When `cc-rs` checks if a compiler flag is supported, it tries to read `out_dir` to generate the source file in. However, when this option is not set, then the check silently fails and the flag is considered to be unsupported. Since we didn't set `out_dir`, all such added flags were simply ignored. Normally, it reads `OUT_DIR`, which is fine if `cc-rs` is used within a build script. But if it is used elsewhere, then this is a pretty gnarly footgun in `cc-rs`, because the failure is [completely silent](https://github.com/rust-lang/cc-rs/blob/main/src/lib.rs#L1483). CC @madsmtm or @NobodyXu in case you have thoughts on that :) After that was fixed, I had to change the used compression flag from `-gz` to `--compress-debug-sections`, to support both BFD and LLD linkers. And to solve it properly, and allow `dist` CI jobs to configure debuginfo compression for all targets that they build, I added a new bootstrap option to configure debuginfo compression. I wanted the flag to be configurable both globally and per target. At the same time, the flag applies both to C/C++ and Rust. We currently don't have such config area in `bootstrap.toml`, and `[build]` didn't seem like the right place, so I shoved it into `[rust]` (while documenting that it also applies to C/C++). r? @bjorn3 try-job: dist-x86_64-msvc try-job: dist-x86_64-apple try-job: dist-x86_64-linux
…uwer Rollup of 8 pull requests Successful merges: - #150075 (Implement clamp_to) - #158169 (Fix debuginfo compression in bootstrap) - #158397 (delegation: support simplest output `Self` mapping) - #158613 (Fix getrandom fallback test on platforms with `panic=abort`) - #158620 (Remove skip_norm_w/i/p().def_id with a helper) - #158633 (Remove unnecessary `Clone` derives on resolver types) - #158634 (Add missing `needs_drop` check to `DroplessArena`.) - #158647 (Document `strip_circumfix` behavior on overlapping prefix and suffix.)
|
💔 I suspect this PR failed tests as part of a rollup After fixing the problem, consider running a try job for the failed job before re-approving. Link to failure: #158660 (comment) |
|
This pull request was unapproved. This PR was contained in a rollup (#158660), which was unapproved. |
|
Hmm, @bjorn3 Any ideas? :) |
|
I guess only passing it on Linux for now and opening an issue for getting it to work on other targets is fine. We didn't compress debuginfo at all before this PR anyway, so it is not regressing anything. |
|
@bors try |
This comment has been minimized.
This comment has been minimized.
Fix debuginfo compression in bootstrap try-job: dist-x86_64-msvc try-job: dist-x86_64-apple try-job: dist-x86_64-linux try-job: dist-various-1 try-job: dist-various-2
|
Ok, makes sense. @rustbot ready |
View all comments
Found through https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/weird.20code.20in.20fill_target_compiler/with/604588655.
This PR solves a few issues with debuginfo compression in bootstrap.
The main issue was that debuginfo compression through the
-gzflag wasn't enabled, by mistake.When
cc-rschecks if a compiler flag is supported, it tries to readout_dirto generate the source file in. However, when this option is not set, then the check silently fails and the flag is considered to be unsupported. Since we didn't setout_dir, all such added flags were simply ignored.Normally, it reads
OUT_DIR, which is fine ifcc-rsis used within a build script. But if it is used elsewhere, then this is a pretty gnarly footgun incc-rs, because the failure is completely silent. CC @madsmtm or @NobodyXu in case you have thoughts on that :)After that was fixed, I had to change the used compression flag from
-gzto--compress-debug-sections, to support both BFD and LLD linkers.And to solve it properly, and allow
distCI jobs to configure debuginfo compression for all targets that they build, I added a new bootstrap option to configure debuginfo compression.I wanted the flag to be configurable both globally and per target. At the same time, the flag applies both to C/C++ and Rust. We currently don't have such config area in
bootstrap.toml, and[build]didn't seem like the right place, so I shoved it into[rust](while documenting that it also applies to C/C++).r? @bjorn3
try-job: dist-x86_64-msvc
try-job: dist-x86_64-apple
try-job: dist-x86_64-linux
try-job: dist-various-1
try-job: dist-various-2