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.

Sign up for GitHub

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

Jump to bottom

building cargo breaks build-cache for other tools #130980

Closed
jieyouxu opened this issue Sep 28, 2024 · 4 comments · Fixed by #131060 or #131155
Closed

building cargo breaks build-cache for other tools #130980

jieyouxu opened this issue Sep 28, 2024 · 4 comments · Fixed by #131060 or #131155
Assignees
@jieyouxu
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@jieyouxu
Copy link
Member

jieyouxu commented Sep 28, 2024 •
edited
Loading

After #130739 I noticed that whenever I modify run-make tests (on Windows), stage 1 cargo gets unnecessary rebuilt which also triggers rebuilds for other tools like stage 0 rustdoc. I'm pretty sure something in my implementation is wrong w.r.t. to properly caching, but I didn't invest time in figuring out why yet, hence opening this bug to track it.

The build sequence is something like stage 0 std -> stage 1 rustc -> stage 1 std -> stage 1 tool cargo -> stage 0 compiletest / run-make-support / rustdoc.

Example repro steps:

  • Run ./x test run-make --stage 1 -> make sure stage 1 cargo is already built.
  • Modify a tests/run-make rmake.rs test.
  • Observe that stage 1 cargo gets rebuilt even though I didn't modify it.
@jieyouxu jieyouxu added A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 28, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 28, 2024
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 28, 2024
@jieyouxu
Copy link
Member Author

EDIT: oh, is it because in run-make-support or rustdoc we use prepare_tool_cargo or something that is a different staged cargo compared to the one we built with stage 1 rustc...

@jieyouxu jieyouxu assigned jieyouxu and unassigned jieyouxu Sep 28, 2024
@jieyouxu
Copy link
Member Author

jieyouxu commented Sep 28, 2024 •
edited
Loading

Actually unassigning because I don't know how to cleanly fix this just yet.

EDIT: opened thread https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Modifying.20run-make.20tests.20unnecessarily.20rebuild.20stage.201.20cargo.

@onur-ozkan
Copy link
Member

Example repro steps:

Run ./x test run-make --stage 1 -> make sure stage 1 cargo is already built.
Modify a tests/run-make rmake.rs test.
Observe that stage 1 cargo gets rebuilt even though I didn't modify it.

Running ./x test run-make --stage 1 twice is enough to reproduce cargo rebuild issue. Probably some rustflags invalidating the build cache.

This was referenced Sep 30, 2024
Closed
Merged
@onur-ozkan onur-ozkan changed the title Modifying run-make tests triggers unnecessary stage 1 cargo rebuilds building cargo breaks build-cache for other tools Sep 30, 2024
@bors bors closed this as completed in d97c661 Sep 30, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 30, 2024
@rust-timer
Unrolled build for rust-lang#131060
25fc6db 
Rollup merge of rust-lang#131060 - jieyouxu:rmake-rebuild, r=onur-ozkan

Drop conditionally applied cargo `-Zon-broken-pipe=kill` flags to fix stage 1 cargo rebuilds

The conditionally applied `-Zon-broken-pipe=kill` flag trigger rebuilds because they can invalidate previous tool build cache due to differing flags. This PR removes those flags to stop tool build cache invalidation.

> The build cache for clippy will be broken because bootstrap sets `-Zon-broken-pipe=kill` for every invocation except for cargo. Which means building cargo will invalidate any tool build cache which was built previously.
>
> *From <https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Modifying.20run-make.20tests.20unnecessarily.20rebuild.20stage.201.20cargo/near/473486972>*

Fixes rust-lang#130980.
But introduces rust-lang#131059 (breaks 2 cargo tests that relied upon some behavior related to `-Zon-broken-pipe=kill` being set).

r? `@onur-ozkan` (or bootstrap)
@jieyouxu
Copy link
Member Author

jieyouxu commented Oct 1, 2024 •
edited
Loading

Reopening because we need to revert #131060 in favor of a different approach.

@jieyouxu jieyouxu reopened this Oct 1, 2024
@jieyouxu jieyouxu mentioned this issue Oct 1, 2024
Merged
@jieyouxu jieyouxu self-assigned this Oct 1, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 2, 2024
@matthiaskrgr
Rollup merge of rust-lang#131108 - jieyouxu:revert-broken-pipe, r=onu…
73bd7a6 
…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)
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Oct 2, 2024
@workingjubilee
Rollup merge of rust-lang#131108 - jieyouxu:revert-broken-pipe, r=onu…
42e3ff2 
…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)
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 2, 2024
@rust-timer
Unrolled build for rust-lang#131108
ad4eede 
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 mentioned this issue Oct 2, 2024
Merged
RalfJung pushed a commit to RalfJung/miri that referenced this issue Oct 3, 2024
@workingjubilee
Rollup merge of #131108 - jieyouxu:revert-broken-pipe, r=onur-ozkan
563ffc6 
Revert #131060 "Drop conditionally applied cargo `-Zon-broken-pipe=kill` flags"

In [#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 5a7058c5a542ec42d1fa9b524f7b4f7d6845d1e9 (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 <rust-lang/rust#131059 (comment)> and in the timeline below.

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

See [`unix_sigpipe` tracking issue #97889][#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.
- [#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].

[#34376]: rust-lang/rust#34376
[#49606]: rust-lang/rust#49606
[#97889]: rust-lang/rust#97889
[#102587]: rust-lang/rust#102587
[#103495]: rust-lang/rust#103495
[#124480]: rust-lang/rust#124480
[#130634]: rust-lang/rust#130634
[#130642]: rust-lang/rust#130642
[#130739]: rust-lang/rust#130739
[#130980]: rust-lang/rust#130980
[#131059]: rust-lang/rust#131059
[^1]: rust-lang/rust#131059 (comment)

r? ``@onur-ozkan`` (or bootstrap)
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 7, 2024
@bors
Auto merge of rust-lang#131155 - jieyouxu:always-kill, r=<try>
983e200 
Prevent building cargo from invalidating build cache of other tools due to conditionally applied `-Zon-broken-pipe=kill` via tracked `RUSTFLAGS`

This PR fixes rust-lang#130980 where building cargo invalidated the tool build caches of other tools (such as rustdoc) because `-Zon-broken-pipe=kill` was conditionally passed via `RUSTFLAGS` for other tools *except* for cargo. The differing `RUSTFLAGS` triggered tool build cache invalidation as `RUSTFLAGS` is a tracked env var -- any changes in `RUSTFLAGS` requires a rebuild.

`-Zon-broken-pipe=kill` is load-bearing for rustc and rustdoc to not ICE on broken pipes due to usages of raw std `println!` that panics without the flag being set, which manifests in ICEs.

I can't say I like the changes here, but it is what it is...

See detailed discussions and history of `-Zon-broken-pipe=kill` usage in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Internal.20lint.20for.20raw.20.60print!.60.20and.20.60println!.60.3F/near/474593815.

## Approach

This PR fixes the tool build cache invalidation by informing the `rustc` binary shim when to apply `-Zon-broken-pipe=kill` (i.e. when the rustc binary shim is not used to build cargo). This information is not communicated by `RUSTFLAGS`, which is an env var tracked by cargo, and instead uses an untracked env var `UNTRACKED_BROKEN_PIPE_FLAG` so we won't trigger tool build cache invalidation. We preserve bootstrap's behavior of not setting that flag for cargo by conditionally omitting setting `UNTRACKED_BROKEN_PIPE_FLAG` when building cargo.

Notably, the `-Zon-broken-pipe=kill` instance in https://github.com/rust-lang/rust/blob/1e5719bdc40bb553089ce83525f07dfe0b2e71e9/src/bootstrap/src/core/build_steps/compile.rs#L1058 is not modified because that is used to build rustc only and not cargo itself.

Thanks to `@cuviper` for the idea!

## Testing

### Integration testing

This PR introduces a run-make test for rustc and rustdoc that checks that when they do not ICE/panic when they encounter a broken pipe of the stdout stream.

I checked this test will catch the broken pipe ICE regression for rustc on Linux (at least) by commenting out https://github.com/rust-lang/rust/blob/1e5719bdc40bb553089ce83525f07dfe0b2e71e9/src/bootstrap/src/core/build_steps/compile.rs#L1058, and the test failed because rustc ICE'd.

### Manual testing

I have manually tried:

1. `./x clean && `./x test build --stage 1` -> `rustc +stage1 --print=sysroot | false`: no ICE.
2. `./x clean` -> `./x test run-make` twice: no stage 1 cargo rebuilds.
3. `./x clean` -> `./x build rustdoc` -> `rustdoc +stage1 --version | false`: no panics.
4. `./x test src/tools/cargo`: tests pass, notably `build::close_output` and `cargo_command::closed_output_ok` do not fail which would fail if cargo was built with `-Zon-broken-pipe=kill`.

## Related discussions

Thanks to everyone who helped!
- https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Applying.20.60-Zon-broken-pipe.3Dkill.60.20flags.20in.20bootstrap.3F
- https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Modifying.20run-make.20tests.20unnecessarily.20rebuild.20stage.201.20.2E.2E.2E
- https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Internal.20lint.20for.20raw.20.60print!.60.20and.20.60println!.60.3F

Fixes rust-lang#130980
Closes rust-lang#131059

---

try-job: aarch64-apple
try-job: x86_64-msvc
try-job: x86_64-mingw
@bors bors closed this as completed in 18deb53 Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jieyouxu jieyouxu

Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
No milestone
3 participants
@jieyouxu @onur-ozkan @rustbot