Skip to content

Commit

Permalink
Auto merge of #131155 - jieyouxu:always-kill, r=onur-ozkan
Browse files Browse the repository at this point in the history
Prevent building cargo from invalidating build cache of other tools due to conditionally applied `-Zon-broken-pipe=kill` via tracked `RUSTFLAGS`

This PR fixes #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/rust#130980
Closes rust-lang/rust#131059

---

try-job: aarch64-apple
try-job: x86_64-msvc
try-job: x86_64-mingw
  • Loading branch information
bors committed Oct 8, 2024
2 parents c54c798 + b1cc7b0 commit 6e4b1ef
Showing 0 changed files with 0 additions and 0 deletions.

0 comments on commit 6e4b1ef

Please sign in to comment.