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

Pass the current cargo to run-make tests #130642

Merged
merged 1 commit into from
Sep 21, 2024
Merged

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Sep 20, 2024

A couple tests were using BOOTSTRAP_CARGO with -Zbuild-std, but that
stage0 cargo might not always be in sync with in-tree changes. In
particular, those tests started failing on the beta branch because the
older cargo couldn't find the library Cargo.lock, and then couldn't
build the latest version of compiler_builtins that had nightly changes.

Fixes #130634
r? @saethlin

A couple tests were using `BOOTSTRAP_CARGO` with `-Zbuild-std`, but that
stage0 cargo might not always be in sync with in-tree changes. In
particular, those tests started failing on the beta branch because the
older cargo couldn't find the library `Cargo.lock`, and then couldn't
build the latest version of `compiler_builtins` that had nightly changes.
@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc 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 Sep 20, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 20, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@cuviper
Copy link
Member Author

cuviper commented Sep 20, 2024

I have locally confirmed that this fixes the problem on the beta branch too, so it might be worth a backport. For my test, I used ./x test run-make --target x86_64-pc-windows-gnu, rather than the arm targets that failed in dist-various-1, since that also has the naked_asm! and I already have those cross tools available.

@rustbot label beta-nominated

(I already worked around the problem on beta by marking those tests only-nightly, but we can revert that if we backport this as a better fix.)

@rustbot rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 20, 2024
@saethlin
Copy link
Member

Oh wow. I didn't realize how much was missing from bootstrap to make this work. Some quick grep suggests we should evict BOOTSTRAP_CARGO entirely; I suspect the only other users of it (tests/run-make/thumb-none-qemu/rmake.rs and src/tools/run-make-support/src/external_deps/cargo.rs) are also some form of arcane time bomb.

@cuviper
Copy link
Member Author

cuviper commented Sep 20, 2024

cc @Mark-Simulacrum, who added BOOTSTRAP_CARGO in #76544. Do you agree it makes sense to let that go?

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this seems strictly less sus than BOOTSTRAP_CARGO (previously I looked the Makefile ones that used cargo and those are BOOTSTRAP_CARGO, always thought that was weird but the tests didn't fail in full CI...).

@jieyouxu
Copy link
Member

As this seems strictly more correct than BOOTSTRAP_CARGO,

@bors r+

@bors
Copy link
Contributor

bors commented Sep 21, 2024

📌 Commit 4e53640 has been approved by jieyouxu

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 Sep 21, 2024
@jieyouxu jieyouxu assigned jieyouxu and unassigned saethlin Sep 21, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Sep 21, 2024

Bit annoying that cargo now has to be built for runmake tests :/ But I don't suppose that we can get around that easily.

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#129718 (add guarantee about remove_dir and remove_file error kinds)
 - rust-lang#130598 (Add recursion limit to FFI safety lint)
 - rust-lang#130642 (Pass the current cargo to `run-make` tests)
 - rust-lang#130644 (Only expect valtree consts in codegen)
 - rust-lang#130645 (Normalize consts in writeback when GCE is enabled)
 - rust-lang#130646 (compiler: factor out `OVERFLOWING_LITERALS` impl)

r? `@ghost`
`@rustbot` modify labels: rollup
@jieyouxu
Copy link
Member

jieyouxu commented Sep 21, 2024

Bit annoying that cargo now has to be built for runmake tests :/ But I don't suppose that we can get around that easily.

Some of the tests can't really get around cargo, and I don't think it's any better to remove or ignore the tests that do need cargo (I don't think there's any good way to not rely on cargo unless we want to reimplement -Z build-std ourselves...)

EDIT: it's more accurate to say that previously for non-stage0 cases not building cargo is a bug.

@bors bors merged commit 4336628 into rust-lang:master Sep 21, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2024
Rollup merge of rust-lang#130642 - cuviper:run-make-cargo, r=jieyouxu

Pass the current cargo to `run-make` tests

A couple tests were using `BOOTSTRAP_CARGO` with `-Zbuild-std`, but that
stage0 cargo might not always be in sync with in-tree changes. In
particular, those tests started failing on the beta branch because the
older cargo couldn't find the library `Cargo.lock`, and then couldn't
build the latest version of `compiler_builtins` that had nightly changes.

Fixes rust-lang#130634
r? `@saethlin`
@bjorn3
Copy link
Member

bjorn3 commented Sep 22, 2024

This broke COMPILETEST_FORCE_STAGE0=1 ./x.py test --stage 0 tests/run-make by causing rustc to be built unnecessarily. This is supposed to test the bootstrap compiler without building a new compiler.

@saethlin
Copy link
Member

(I don't think there's any good way to not rely on cargo unless we want to reimplement -Z build-std ourselves...)

Please do not try to do that, the compiler-builtins test that I added is specifically in there to verify that -Zbuild-std works.

@jieyouxu
Copy link
Member

jieyouxu commented Sep 22, 2024

Yeah I wasn't going to try that lol

@jieyouxu
Copy link
Member

jieyouxu commented Sep 22, 2024

This broke COMPILETEST_FORCE_STAGE0=1 ./x.py test --stage 0 tests/run-make by causing rustc to be built unnecessarily. This is supposed to test the bootstrap compiler without building a new compiler.

I think this PR makes the right changes (we should not have used bootstrap cargo indiscriminantly), we just need to handle this special case. I'll take a look tomorrow.

Update: opened #130739 to pass bootstrap cargo to avoid stage1 rustc rebuild when --stage 0 + COMPILETEST_FORCE_STAGE0=1.

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Sep 24, 2024
Pass bootstrap cargo when `--stage 0` and `COMPILETEST_FORCE_STAGE0`

Follow-up to rust-lang#130642 (comment) to make sure that when

```
$ COMPILETEST_FORCE_STAGE0=1 ./x test run-make --stage 0
```

is used, bootstrap cargo is used in order to avoid building stage 1 rustc. Note that run-make tests are usually not written with `--stage 0` in mind and some tests may rely on stage1 rustc (nightly) behavior, and it is expected that some tests will fail under this invocation.

cc `@bjorn3` can you please test if this suits your needs for `cg_clif` to avoid the unnecessary stage1 rustc rebuild?

r? bootstrap
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 24, 2024
Fix cargo staging for run-make tests

Follow-up to rust-lang#130642 (comment) to make sure that when

```
$ COMPILETEST_FORCE_STAGE0=1 ./x test run-make --stage 0
```

is used, bootstrap cargo is used in order to avoid building stage 1 rustc. Note that run-make tests are usually not written with `--stage 0` in mind and some tests may rely on stage1 rustc (nightly) behavior, and it is expected that some tests will fail under this invocation.

This PR also fixes `tool::Cargo` staging in compiletest when preparing for `run-make` test mode, by chopping off a stage from the `compiler` passed to `tool::Cargo` such that when the user invokes with stage `N`

```
./x test run-make --stage N
```

the `run-make` test suite will be tested against the cargo built by stage `N` compiler. Let's take `N=1`, i.e. `--stage 1`, without chopping off a stage, previously `./x test run-make --stage 1` will cause stage 1 rustc + std to be built, then stage 2 rustc, and cargo will be produced by the stage 2 rustc, which is clearly not what we want. By chopping off a stage, it means that cargo will be produced by the stage 1 rustc.

cc rust-lang#119946, rust-lang#59864.
See discussions regarding the tool staging at https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/.E2.9C.94.20stage1.20run-make.20tests.20now.20need.20stage2.20rustc.20built.20for.20c.2E.2E.2E.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 25, 2024
Fix cargo staging for run-make tests

Follow-up to rust-lang/rust#130642 (comment) to make sure that when

```
$ COMPILETEST_FORCE_STAGE0=1 ./x test run-make --stage 0
```

is used, bootstrap cargo is used in order to avoid building stage 1 rustc. Note that run-make tests are usually not written with `--stage 0` in mind and some tests may rely on stage1 rustc (nightly) behavior, and it is expected that some tests will fail under this invocation.

This PR also fixes `tool::Cargo` staging in compiletest when preparing for `run-make` test mode, by chopping off a stage from the `compiler` passed to `tool::Cargo` such that when the user invokes with stage `N`

```
./x test run-make --stage N
```

the `run-make` test suite will be tested against the cargo built by stage `N` compiler. Let's take `N=1`, i.e. `--stage 1`, without chopping off a stage, previously `./x test run-make --stage 1` will cause stage 1 rustc + std to be built, then stage 2 rustc, and cargo will be produced by the stage 2 rustc, which is clearly not what we want. By chopping off a stage, it means that cargo will be produced by the stage 1 rustc.

cc #119946, #59864.
See discussions regarding the tool staging at https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/.E2.9C.94.20stage1.20run-make.20tests.20now.20need.20stage2.20rustc.20built.20for.20c.2E.2E.2E.
@cuviper cuviper deleted the run-make-cargo branch September 27, 2024 01:16
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)
workingjubilee added a commit to workingjubilee/rustc 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)
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)
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Oct 3, 2024
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)
@jieyouxu jieyouxu mentioned this pull request Oct 9, 2024
@cuviper
Copy link
Member Author

cuviper commented Oct 9, 2024

As mentioned in #131423 (comment), this had a number of followups that make it riskier to backport, and it's not really necessary since I marked those tests only-nightly on beta.

@rustbot label -beta-nominated

@rustbot rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 9, 2024
@jieyouxu jieyouxu added the A-compiletest Area: The compiletest test runner label Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc 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
Status: Done
Development

Successfully merging this pull request may close these issues.

run-make -Zbuild-std tests fail on compiler_builtins on older branches
7 participants