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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 26, 2024
Merged

fix broken build cache caused by rustdoc builds #126934

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions src/bootstrap/src/core/build_steps/tool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,13 @@ pub fn prepare_tool_cargo(
// See https://github.com/rust-lang/rust/issues/116538
cargo.rustflag("-Zunstable-options");

// `-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");
}

cargo
}

Expand Down Expand Up @@ -575,7 +582,8 @@ impl Step for Rustdoc {
features.push("jemalloc".to_string());
}

let mut cargo = prepare_tool_cargo(
// NOTE: Never modify the rustflags here, it breaks the build cache for other tools!
let cargo = prepare_tool_cargo(
builder,
build_compiler,
Mode::ToolRustc,
Expand All @@ -586,11 +594,6 @@ impl Step for Rustdoc {
features.as_slice(),
);

// 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");
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this was added in cde0cde, regressing the fix I made in #123192. Seems likely this will regress again.

Can you add a comment here saying to not add any rustflags, as that will cause undesired rebuilds?

Copy link
Member Author

@onur-ozkan onur-ozkan Jun 25, 2024 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Member Author

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.


let _guard = builder.msg_tool(
Kind::Build,
Mode::ToolRustc,
Expand Down
Loading