Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert #131060 "Drop conditionally applied cargo -Zon-broken-pipe=kill flags" #131108

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Oct 1, 2024

In #131059 we found out that -Zon-broken-pipe=kill is actually load-bearing1 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 #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 #130980 until we fix it again with the different approach.

See more details at #131059 (comment) and in the timeline below.

Timeline of kill-process-on-broken-pipe behavior changes

See unix_sigpipe tracking issue #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 #34376.
  • #49606 mitigated #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 #97889, i.e. # [unix_sigpipe = "sig_dfl"] attribute.
  • #102587 migrated rustc to use #[unix_sigpipe = "sig_dfl"] instead of
    rustc_driver::set_sigpipe_handler.
  • #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 #97889, the UI for specifying sigpipe behavior
    was changed in #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.
  • [fix broken build cache caused by rustdoc builds #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 #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.
  • #130642 and later #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 #130980.

r? @onur-ozkan (or bootstrap)

Footnotes

  1. https://github.com/rust-lang/rust/issues/131059#issuecomment-2385822033

This reverts commit 5a7058c.

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.

[rust-lang#131059]: rust-lang#131059
[1]: rust-lang#131059 (comment)
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Oct 1, 2024
@onur-ozkan
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 1, 2024

📌 Commit 1c7e824 has been approved by onur-ozkan

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 1, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 2, 2024
…r-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)
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 2, 2024
…kingjubilee

Rollup of 3 pull requests

Successful merges:

 - rust-lang#130885 (panic when an interpreter error gets unintentionally discarded)
 - rust-lang#131108 (Revert rust-lang#131060 "Drop conditionally applied cargo `-Zon-broken-pipe=kill` flags")
 - rust-lang#131121 (A couple of fixes for dataflow graphviz dumps)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 42e3ff2 into rust-lang:master Oct 2, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 2, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 2, 2024
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)
@jieyouxu jieyouxu deleted the revert-broken-pipe branch October 2, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants