-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
fix broken build cache caused by rustdoc builds #126934
Conversation
r? @clubby789 rustbot has assigned @clubby789. Use |
@clubby789 is not active these days and this is a contributor roadblock. So, r? @Kobzol |
The flag is also set here, is that expected duplication? rust/src/bootstrap/src/core/build_steps/compile.rs Lines 1009 to 1011 in 521e707
|
// If the rustdoc 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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here saying to not add any rustflags, as that will cause undesired rebuilds?
We need to duplicate that comment for every impl Step for $tool
to make it clear, which wouldn't be not good enough either (because people can miss comments sometimes). As I mentioned in #123177 (comment), I will create a follow-up PR later this week to have this automatically checked. So the test can ensure tools never break the build cache again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 IMO one can't have too many comments (or at least, that is very hard). Tests are nice but they only catch things after someone already made their PR, when CI runs. And tests can't cover all combinations of tools either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely need a change that ensures we never end up breaking the cache again (for any tool not just rustdoc).
Added a comment for rustdoc as it's harmless to have.
This PR changes rustflags for tools and the one you linked is for rustc builds. So, it is expected. |
I wonder if we could return some "sealed" variant of But in any case, that's a more of a long-term goal, this PR looks reasonable as a hotfix. |
191f594
to
2e017f4
Compare
The comment is a (slight) improvement over the status quo. Feel free to r=me. |
@bors r=Kobzol rollup |
…obzol fix broken build cache caused by rustdoc builds Currently rustdoc breaks the build cache (due to having different rustflags) when building rustdoc before building another tool (e.g., `x test miri && x test rustdoc && x test miri`). This change fixes that by moving `on-broken-pipe` into `prepare_cargo_tool` so it is set for all tools. cc `@RalfJung` Fixes rust-lang#123177
…mpiler-errors Rollup of 7 pull requests Successful merges: - rust-lang#126893 (Eliminate the distinction between PREC_POSTFIX and PREC_PAREN precedence level) - rust-lang#126907 (Fixes for 32-bit SPARC on Linux) - rust-lang#126932 (Tweak `FlatPat::new` to avoid a temporarily-invalid state) - rust-lang#126934 (fix broken build cache caused by rustdoc builds) - rust-lang#126943 (De-duplicate all consecutive native libs regardless of their options) - rust-lang#126946 (Add missing slash in `const_eval_select` doc comment) - rust-lang#126947 (Delegation: ast lowering refactor) r? `@ghost` `@rustbot` modify labels: rollup
31e997a
to
808ac1a
Compare
This comment has been minimized.
This comment has been minimized.
Currently rustdoc breaks the build cache (due to having different rustflags) when building rustdoc before building another tool (e.g., `x test miri && x test rustdoc && x test miri`). This change fixes that by moving `on-broken-pipe` into `prepare_cargo_tool` so it is set for all tools. Signed-off-by: onur-ozkan <work@onurozkan.dev>
808ac1a
to
b3fb67e
Compare
@bors r+ rollup=iffy p=1 |
@bors r- |
@bors r=Kobzol |
☀️ Test successful - checks-actions |
Finished benchmarking commit (bae813a): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 3.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 1.6%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 694.92s -> 695.46s (0.08%) |
Currently rustdoc breaks the build cache (due to having different rustflags) when building rustdoc before building another tool (e.g.,
x test miri && x test rustdoc && x test miri
).This change fixes that by moving
on-broken-pipe
intoprepare_cargo_tool
so it is set for all tools.cc @RalfJung
Fixes #123177