Adjusting -Zon-broken-pipe=kill to only be applied to rustc/rustdoc binary wrappers break two cargo tests

[jieyouxu]

In https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Modifying.20run-make.20tests.20unnecessarily.20rebuild.20stage.201.20cargo we identified the root cause of #130980 (unnecessary stage 1 cargo rebuilds) being

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

Lines 212 to 217 in e9df22f

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

The other instance of this is

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

Line 1058 in e9df22f

cargo.rustflag("-Zon-broken-pipe=kill");

As @onur-ozkan (thank you so much) pointed out:

It's not just about cargo builds. If you run x build clippy and then x build cargo, 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.

---

I tried to limit application of -Zon-broken-pipe=kill to rustc and rustdoc binary wrappers in src/bootstrap/src/bin/{rustc,rustdoc}.rs themselves, and unfortunately this leads to two cargo test failures:

---- build::close_output stdout ----
thread 'build::close_output' panicked at tests/testsuite/build.rs:6680:18:

---- expected: tests/testsuite/build.rs:6682:9
++++ actual:   In-memory
   1    1 | [COMPILING] foo v0.1.0 ([ROOT]/foo)
   2    2 | hello stderr!
   3      - [ERROR] [BROKEN_PIPE]
   4      - [WARNING] build failed, waiting for other jobs to finish...

Update with SNAPSHOTS=overwrite

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- cargo_command::closed_output_ok stdout ----
thread 'cargo_command::closed_output_ok' panicked at tests/testsuite/cargo_command.rs:549:5:
assertion failed: status.success()


failures:
    build::close_output
    cargo_command::closed_output_ok

Previous history

I'm putting up a PR (#131060) to remove conditionally adding -Zon-broken-pipe=kill, and as @saethlin pointed out we expect two cargo tests to be broken with RUSTFLAGS=-Zon-broken-pipe=kill ./x test src/tools/cargo if that is unconditionally passed to bootstrap.

---- build::close_output stdout ----
thread 'build::close_output' panicked at tests/testsuite/build.rs:6680:18:

---- expected: tests/testsuite/build.rs:6682:9
++++ actual:   In-memory
   1    1 | [COMPILING] foo v0.1.0 ([ROOT]/foo)
   2    2 | hello stderr!
   3      - [ERROR] [BROKEN_PIPE]
   4      - [WARNING] build failed, waiting for other jobs to finish...

Update with SNAPSHOTS=overwrite

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- cargo_command::closed_output_ok stdout ----
thread 'cargo_command::closed_output_ok' panicked at tests/testsuite/cargo_command.rs:549:5:
assertion failed: status.success()


failures:
    build::close_output
    cargo_command::closed_output_ok

I don't know if this breaks someone's workflow regarding to piping cargo tool tests, but in the interest of not making everyone rebuild stage1 cargo on every single invocation of ./x test run-make, I'm considering this as "known breakage" for the time being. As @saethlin suggested:

If someone wants to both set -Zon-broken-pipe and also run the Cargo test suite, I think they should file an issue and be part of a discussion about how to support that and what the costs are.

[jieyouxu]

cc @rust-lang/cargo team in case anyone has any advice regarding these two tests.

[jieyouxu]

cc @Enselic if you have any advice since this was added in #124480 for #97889 (it didn't occur to me that adding -Zon-broken-pipe=kill would cause build cache invalidation problems), presumably to workaround the cargo test failures.

[Enselic]

Thanks for the ping. Unfortunately I currently do not have the time to look into this.

[jieyouxu]

No worries that is fine, pinged just in case you had immediate context or advice.

[weihanglo]

Maybe we should only run those two tests in rust-lang/cargo CI?

[ehuss]

Can you give some more context? What are the benefits and drawbacks of -Zon-broken-pipe=kill? What does that look like to the user of rustc and cargo?

I don't quite understand #131060. It looks like it regresses something, but I'm not clear what. If it is breaking cargo tests, how is it possible for that PR to land? It seems like removing -Zon-broken-pipe=kill would be regressing whatever issue it was added to fix in the first place, but I don't know what that is.

[saethlin]

The linked PR does not break Cargo tests, it possibly breaks someone's contributor workflow that relies on bootstrap implicitly setting -Zon-broken-pipe=kill. Bootstrap can't implicitly set that flag for everything, or it breaks Cargo's tests.

The bug here is that none of the numerous people who looked at the original PR (including me) noticed that the right thing to do here is not set the flag at all, because inconsistent flag-setting creates spurious rebuilds.

[jieyouxu]

Yeah sorry I didn't know how to word the issue title accurately (please rename this issue if there's an more accurate description), #131060 will remove conditionally passed -Zon-broken-pipe=kill flags passed to cargo from bootstrap, which might break someone's workflow related to running cargo tool tests through bootstrap if they wanted to do something like ./x test src/tools/cargo | head -n 1 (AFAICT) because now we don't pass -Zon-broken-pipe=kill at all, but unconditionally setting RUSTFLAGS=-Zon-broken-pipe=kill will cause the aforementioned test failures.

EDIT: I tried to fixup the issue description and title to be more accurate.

[ehuss]

Sorry, I'm still not clear what is being changed or what is being proposed. Also, sorry for many questions, but I don't have context on what is going on.

What happens after #131060 when someone does ./x test src/tools/cargo | head -n 1? What happened before?

Is the concern after #131060 specifically about ./x test src/tools/cargo, or is it all bootstrap commands? Some of them?

Does #131060 change the dist artifacts? If so, how? How much of this is specific to using ./x versus using rustc and cargo?

Is there an original issue related to bootstrap workflows that initiated the introduction of -Zon-broken-pipe=kill? Who is asking for a different behavior?

Looking at #124480, it seems like this stems back to #49606. How does rustc now handle sigpipe? My naive thinking is that removing -Zon-broken-pipe=kill causes that to regress, but it doesn't seem like that is what you are proposing, and clearly no tests are being broken. But I don't see how all these pieces fit together.

[ChrisDenton]

-Zon-broken-pipe=kill only affects the entry point. Setting it on libraries is useless.

I think cargo rustc could be used to add flags that only effect the final build and not any dependency.

[jieyouxu]

Sorry, I'm still not clear what is being changed or what is being proposed. Also, sorry for many questions, but I don't have context on what is going on.

... Actually thanks for asking these questions, it seems that simply removing these flags to fix rebuilds will regress rustc (and probably rustdoc) behavior with respect to broken pipes. rustc will ICE locally (e.g. rustc +stage1 --print=sysroot | false, i.e. reintroduces #49606) without being built with that flag being set.

The context is that I was trying to make ./x test run-make --stage 1 not have to rebuild stage1 cargo even if nothing changed, because -Zon-broken-pipe=kill was not universally applied to tool builds (specifically excluding cargo), which causes tool build cache invalidation because stage1 cargo has to be rebuilt due to the differing flags.

I will revert #131060, investigate why regression test for rustc doesn't catch the broken pipe ICE, and reinvestigate what is going on with -Zon-broken-pipe.

[jieyouxu]

Does #131060 change the dist artifacts? If so, how? How much of this is specific to using ./x versus using rustc and cargo?

Apparently it does, I'm putting up a revert now. This unfortunately affects how rustc itself is built.

Is there an original issue related to bootstrap workflows that initiated the introduction of -Zon-broken-pipe=kill? Who is asking for a different behavior?

This was originally related to #124480 where we changed from the in-source attribute of #[unix_sigpipe = "sig_dfl"] UI to a command-line UI -Zon-broken-pipe=kill, so they were added in an attempt to preserve the existing behavior of kill process if pipe is broken, instead of panicking (and possibly leading to an ICE for rustc specifically).

Looking at #124480, it seems like this stems back to #49606. How does rustc now handle sigpipe? My naive thinking is that removing -Zon-broken-pipe=kill causes that to regress, but it doesn't seem like that is what you are proposing, and clearly no tests are being broken. But I don't see how all these pieces fit together.

How does rustc now handle sigpipe?

Apparently through that -Zon-broken-pipe=kill flag when the rustc main binary is being built.

My naive thinking is that removing -Zon-broken-pipe=kill causes that to regress, but it doesn't seem like that is what you are proposing, and clearly no tests are being broken. But I don't see how all these pieces fit together.

AFAICT your suspicions are correct, and indeed removing the flag will regress that. I don't know why no tests are being broken, which probably means no tests were actually exercising this behavior.

[jieyouxu]

@ehuss I tried to do some digging, and here's what I came up with.

---

Note: the following investigation is to the best of my understanding and most likely contains inaccuracies and misleading representations, reader discretion is required

Context and Archaeology

• In Change SIGPIPE ui from #[unix_sigpipe = "..."] to -Zon-broken-pipe=... #124480, the UI of #[unix_sigpipe = "sig_dfl"] changed to become an unstable rustc flag -Z on-broken-pipe (cf. stabilization comment Tracking Issue for unix_sigpipe #97889 (comment)).

  • #[unix_sigpipe = "sig_dfl"] corresponds to -Zon-broken-pipe=kill.
  • Not setting -Zon-broken-pipe=kill will default to sig_ign. Note that this default behavior will lead to panic in Rust programs if the pipe is broken (e.g in a simple program ./hello_world | false). For rustc binary it means the unexpected panic will be caught by the ICE handler manifesting in an ICE ICE on "rustc --help | false" #34376. For other tools like rustdoc or cargo it depends on how unexpected panics are handled, but nothing good.
  • Change SIGPIPE ui from #[unix_sigpipe = "..."] to -Zon-broken-pipe=... #124480 removed #[unix_sigpipe = "sig_dfl"] from rustc and rustdoc main.rs (i.e. from their binaries), and set -Zon-broken-pipe=kill to preserve kill-process-on-broken-pipe behavior to not have the panics and ICEs. Before #[unix_sigpipe] was available, ICE on "rustc --help | false" #34376 was addressed by adding a sigpipe handler in rustc_driver explicitly, as done in Prevent broken pipes causing ICEs #49606. But this was changed in rustc: Use unix_sigpipe instead of rustc_driver::set_sigpipe_handler #102587 for rustc and in rustdoc: Use unix_sigpipe instead of rustc_driver::set_sigpipe_handler #103495 for rustdoc to use #[unix_sigpipe] which was intended to be a more reusable mechanism. Now Change SIGPIPE ui from #[unix_sigpipe = "..."] to -Zon-broken-pipe=... #124480 removed #[unix_sigpipe] attribute meaning that those -Zon-broken-pipe=kill flags are load-bearing, for building rustc and rustdoc binaries. Cargo might not be affected itself if it uses its own sigpipe handling, but I have not investigated that specifically.

• In fix broken build cache caused by rustdoc builds #126934 to help with Running ./x.py test miri --stage 0 twice rebuilds miri, cargo-miri, and rustdoc #123177, we added

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

• In fix broken build cache caused by rustdoc builds #126934 we moved -Zon-broken-pipe=kill being set only for Rustdoc tool build from impl Step for Rustdoc to prepare_tool_cargo to help mitigate rebuilds for Miri tests (because setting only -Zon-broken-pipe=kill for rustdoc means if you build another tool, the tool build cache will get invalidated due to differing RUSTFLAGS).
  • Note that this particular instance of -Zon-broken-pipe=kill is for tool builds and cargo, there is a different instance of -Zon-broken-pipe=kill for building the rustc binary itself in /src/core/build_steps/compile.rs.
  • Conditionally omitting -Zon-broken-pipe=kill for cargo itself does not cause tool build cache invalidation before Fix cargo staging for run-make tests #130739 (and Pass the current cargo to run-make tests #130642) because we were simply using bootstrap cargo (or rather, initial_cargo in bootstrap terms) and not try to build stage 1 cargo ourselves. Only run-make tests want to build stage 1 cargo, even miri tests just use initial cargo. Now that run-make tests builds stage 1 cargo, the omitted -Zon-broken-pipes=kill for cargo tool build specifically will lead to tool build cache invalidation due to differing RUSTFLAGS.
  • Note that I don't fully understand how tool build caches work and why differing RUSTFLAGS for one of the tools can lead to tool build cache invalidation for all of the tools built from the same stage compiler.

What about #131060?

• I tried to remove both instances of -Zon-broken-pipe=kill:
  1. The -Zon-broken-pipe=kill instance in prepare_tool_cargo: this I suspect was intended for rustdoc binary. Now I think I understand I cannot remove this flag, because to at least rustdoc binary I believe -Zon-broken-pipe=kill is load-bearing for kill-process-on-broken-pipe behavior due to #[unix_sigpipe] UI changes to command-line flag as aforementioned. Not sure how cargo tests are affected, which is why the flag is not unconditionally always set in the first place.
  2. The -Zon-broken-pipe=kill instance in src/core/build_steps/compile.rs: this I definitely should not have removed because as I described previously is now load-bearing for the rustc binary for kill-process-on-broken-pipe, otherwise it will reintroduce ICE on "rustc --help | false" #34376.

How do we fix this?

• I intend to revert Drop conditionally applied cargo -Zon-broken-pipe=kill flags to fix stage 1 cargo rebuilds #131060 now that I have a better understanding of why these flags are even there in the first place, and not having the -Zon-broken-pipes=kill flags set regresses broken-pipe behavior for rustc and rustdoc at the minimum. This effect includes dist builds of them so I practically introduced a P-critical regression to rustc and rustdoc if not also for other tools.
• I intend to try unconditionally set -Zon-broken-pipe=kill for all tool builds in prepare_tool_cargo so that (1) it preserves desired kill-process-on-broken-pipe behavior for rustdoc and other tools that are typical of cli tools and (2) it should not cause tool build cache invalidation because it is not conditionally absent from cargo. I intend to investigate/fix a few things:
  1. Work with T-cargo and T-bootstrap to figure out why some cargo tests may fail on -Zon-broken-pipe=kill and either fix those tests and/or fix how cargo is building itself.
  2. Investigate why zero tests failed in full CI and locally, in both rustc and rustdoc test suites, even if I reintroduce the ICE on "rustc --help | false" #34376 ICE for rustc and a similar panic for rustdoc. I intend to add regression tests for rustc and rustdoc for this behavior specifically, or fix the existing tests that didn't test what they intended to test.

[jieyouxu]

I tried to update PR title and PR description to better reflect the current understanding regarding the two cargo test failures related to changing where -Zon-broken-pipe=kill is applied in bootstrap.