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.