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. |
|
Oh, it is hardcoded 🤦 This keeps getting better. So maybe we can detect if the linker is |
|
Yeah, should work. |
|
This pull request was unapproved due to being closed. |
|
This PR modifies If appropriate, please update This PR modifies If appropriate, please update |
6dcec99 to
ead43fb
Compare
|
I decide to leave this in a better state than it was, and added an option to bootstrap for debuginfo compression. @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.
Fix debuginfo compression in bootstrap
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (9372e13): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary -2.1%, secondary -2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 482.353s -> 481.6s (-0.16%) |
|
Nice artifact size wins! Looks like we weren't compressing libstd nor Cargo (I confirmed it using |
| } | ||
| } | ||
| StringOrBool::String(value) => CompressDebuginfo::from_str(&value) | ||
| .map_err(|_| D::Error::invalid_value(Unexpected::Str(&value), &"zlib or off"))?, |
There was a problem hiding this comment.
Nit:
| .map_err(|_| D::Error::invalid_value(Unexpected::Str(&value), &"zlib or off"))?, | |
| .map_err(|_| D::Error::invalid_value(Unexpected::Str(&value), &"`zlib` or `off`"))?, |
| fi | ||
|
|
||
| # Compress any remaining debuginfo to make the resulting artifacts slightly smaller | ||
| RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set rust.compress-debuginfo=zlib" |
There was a problem hiding this comment.
Maybe this should be in the dist profile?
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