Skip to content

Commit 73bd7a6

Browse files
authored
Rollup merge of rust-lang#131108 - jieyouxu:revert-broken-pipe, r=onur-ozkan
Revert rust-lang#131060 "Drop conditionally applied cargo `-Zon-broken-pipe=kill` flags" In [rust-lang#131059] we found out that `-Zon-broken-pipe=kill` is actually **load-bearing**[^1] for (at least) `rustc` and `rustdoc` to have the kill-process-on-broken-pipe behavior, e.g. `rustc --print=sysroot | false` will ICE and `rustdoc --print=sysroot | false` will panic on a broken pipe. This PR reverts 5a7058c (reverts PR rust-lang#131060) in favor of a future fix to *unconditionally* apply `-Zon-broken-pipe=kill` to tool builds and also not drop the `-Zon-broken-pipe=kill` flag for rustc binary builds. I could not figure out how to write a regression test for the `rustc --print=sysroot | false` behavior on Unix, so this is a plain revert for now. This revert will unfortunately reintroduce rust-lang#130980 until we fix it again with the different approach. See more details at <rust-lang#131059 (comment)> and in the timeline below. ### Timeline of kill-process-on-broken-pipe behavior changes See [`unix_sigpipe` tracking issue rust-lang#97889][rust-lang#97889] for more context around unix sigpipe handling. - From the very beginning since 2014, Rust binaries by default use `sig_ign`. This meant that if output pipe is broken yet the program tries to use `println!` and such, there will be a broken pipe panic from std. This lead to ICEs from e.g. `rustc --help | false` [rust-lang#34376]. - [rust-lang#49606] mitigated [rust-lang#34376] by adding an explicit signal handler to `rustc_driver` register a sigpipe handler with `SIG_DFL` which will cause the binary using `rustc_driver` to terminate if `rustc_driver::set_sigpipe_handler()` is called. `rustc`'s main binary wrapper uses `rustc_driver::set_sigpipe_handler()`, and so does `rustdoc`. - A more universal way to set sigpipe behavior for Unix was introduced as part of [rust-lang#97889], i.e. `# [unix_sigpipe = "sig_dfl"]` attribute. - [rust-lang#102587] migrated `rustc` to use `#[unix_sigpipe = "sig_dfl"]` instead of `rustc_driver::set_sigpipe_handler`. - [rust-lang#103495] migrated `rustdoc` to use `#[unix_sigpipe = "sig_dfl"]` instead of `rustc_driver::set_sigpipe_handler`. `rustc_driver::set_sigpipe_handler` was removed. - Following concerns about sigpipe setting UI in [rust-lang#97889], the UI for specifying sigpipe behavior was changed in [rust-lang#124480] from `#[unix_sigpipe = "sig_dfl"]` attribute to the commmand line flag `-Zon-broken-pipe=kill`. - In the same PR, `#[unix_sigpipe = "sig_dfl"]` were removed from `rustc` and `rustdoc` main binary crate entry points in favor of the command line flag. Kill-process-on-broken-pipe behavior was preserved by adding `-Zon-broken-pipe=kill` for `rustdoc` tool build step and `rustc` during compile steps. - [rust-lang#126934] added `-Zon-broken-pipe=kill` for tool builds *except* for cargo to help with some miri tests because at the time the PR was written, this would lead to a couple of cargo test failures. Conditionally setting `RUSTFLAGS` can lead to tool build invalidation, e.g. building `cargo` without `-Zon-broken-pipe=kill` but `clippy` with the flag can lead to invalidation of the tool build cache. This is not a problem at the time, because nothing (not even miri) tests built stage 1 cargo (all used initial cargo). - In [rust-lang#130634] we found out that `run-make` tests like `compiler-builtins` needed stage 1 cargo, not just beta bootstrap cargo, because there can be changes that are present in stage 1 cargo but absent in beta cargo, which was blocking a beta backport. - [rust-lang#130642] and later [rust-lang#130739] now build stage 1 cargo. And as previously mentioned, since `-Zon-broken-pipe=kill` was specifically *not* set for cargo, this caused tool build cache invalidation meaning rebuilds of stage 1 even if nothing in source was changed due to differing `RUSTFLAGS` since `run-make` also builds `rustdoc` and such [rust-lang#130980]. [rust-lang#34376]: rust-lang#34376 [rust-lang#49606]: rust-lang#49606 [rust-lang#97889]: rust-lang#97889 [rust-lang#102587]: rust-lang#102587 [rust-lang#103495]: rust-lang#103495 [rust-lang#124480]: rust-lang#124480 [rust-lang#130634]: rust-lang#130634 [rust-lang#130642]: rust-lang#130642 [rust-lang#130739]: rust-lang#130739 [rust-lang#130980]: rust-lang#130980 [rust-lang#131059]: rust-lang#131059 [^1]: rust-lang#131059 (comment) r? `@onur-ozkan` (or bootstrap)
2 parents ec51e90 + 1c7e824 commit 73bd7a6

File tree

2 files changed

+11
-4
lines changed

2 files changed

+11
-4
lines changed

src/bootstrap/src/core/build_steps/compile.rs

+4
Original file line numberDiff line numberDiff line change
@@ -1053,6 +1053,10 @@ pub fn rustc_cargo(
10531053

10541054
cargo.rustdocflag("-Zcrate-attr=warn(rust_2018_idioms)");
10551055

1056+
// If the rustc output is piped to e.g. `head -n1` we want the process to be
1057+
// killed, rather than having an error bubble up and cause a panic.
1058+
cargo.rustflag("-Zon-broken-pipe=kill");
1059+
10561060
if builder.config.llvm_enzyme {
10571061
cargo.rustflag("-l").rustflag("Enzyme-19");
10581062
}

src/bootstrap/src/core/build_steps/tool.rs

+7-4
Original file line numberDiff line numberDiff line change
@@ -200,10 +200,6 @@ pub fn prepare_tool_cargo(
200200
cargo.arg("--features").arg(features.join(", "));
201201
}
202202

203-
// Warning: be very careful with RUSTFLAGS or command-line options, as conditionally applied
204-
// RUSTFLAGS or cli flags can lead to hard-to-diagnose rebuilds due to flag differences, causing
205-
// previous tool build artifacts to get invalidated.
206-
207203
// Enable internal lints for clippy and rustdoc
208204
// NOTE: this doesn't enable lints for any other tools unless they explicitly add `#![warn(rustc::internal)]`
209205
// See https://github.com/rust-lang/rust/pull/80573#issuecomment-754010776
@@ -213,6 +209,13 @@ pub fn prepare_tool_cargo(
213209
// See https://github.com/rust-lang/rust/issues/116538
214210
cargo.rustflag("-Zunstable-options");
215211

212+
// `-Zon-broken-pipe=kill` breaks cargo tests
213+
if !path.ends_with("cargo") {
214+
// If the output is piped to e.g. `head -n1` we want the process to be killed,
215+
// rather than having an error bubble up and cause a panic.
216+
cargo.rustflag("-Zon-broken-pipe=kill");
217+
}
218+
216219
cargo
217220
}
218221

0 commit comments

Comments
 (0)