Tracking Issue for bootstrap spurious rebuilds

[jieyouxu]

This is a tracking issue for collecting spurious rebuild issues in bootstrap. Spurious rebuilds refer to rebuilds that seem unnecessary, e.g. if running ./x test run-make twice in a row without modifying any sources rebuilds cargo. Note that is this not always the case, e.g. at the time when this issue was created, mir-opt tests build a special std with mir-opt RUSTFLAGS, which will need to be rebuilt if trying to go to stage2 because a stage2 rustc will expect a stage1 "standard" std build without mir-opt RUSTFLAGS (there are other solutions to that, not in the scope of this issue).

Categories

NOTE: There is only currently one category of spurious rebuilds that I am acutely aware of, it's entirely possible to have other classes of causes for spurious rebuilds.

Differing RUSTFLAGS

While some of these issues are closed, the fixes are usually stopgap solutions.

• Clippy gets rebuilt twice on every change, and rustdoc also gets rebuilt #131636
• Running ./x test mir-opt rebuilds std every time #131437
• Bootstrap re-builds everything after "x.py check" #130108
  • Remark: this is RUSTFLAGS too via cargo.configure_linker.
• Running run-make tests performs unnecessary rebuilds #126464
• Running ./x.py test miri --stage 0 twice rebuilds miri, cargo-miri, and rustdoc #123177

Preventing spurious rebuilds due to differing RUSTFLAGS

To solve the class of spurious rebuilds due to differing RUSTFLAGS, we will need to properly handle them. Specifically, we need to (#131636 (comment)):

• Centralize RUSTFLAGS handling to forbid naive "conditional" RUSTFLAGS.
• Conditional RUSTFLAGS should be propagated to bootstrap shims using environment variable (something like IMPLICIT_RUSTFLAGS) so they can't invalidate the build cache.
• Ensure bootstrap should never set any conditional rustflag explicitly on cargo.

Discussion

https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Mechanism.20for.20better.20RUSTFLAGS.20handling

[RalfJung]

Worth to solve this problem properly along with a test coverage.. I think, all rustflags should be handled from a single place and every conditional rustflags should be propagated to bootstrap shims using environment variable (something like IMPLICIT_RUSTFLAGS) so they can't invalidate the build cache. Bootstrap should never set any conditional rustflag explicitly on cargo.

Setting flags in the wrapper can also be an issue because it means cargo doesn't know about them and if bootstrap picks different flags, cargo won't know it has to rebuild. So that can easily lead to the opposite bug where a rebuild is needed due to config changes or a bootstrap update, but then a rebuild doesn't happen.

[onur-ozkan]

Setting flags in the wrapper can also be an issue because it means cargo doesn't know about them and if bootstrap picks different flags

It’s worth mentioning in the implementation that we shouldn’t pass a rustflags to the wrapper if it’s propagated dynamically (e.g., from config.toml). But in most cases bootstrap sets rustflags consistently so this situation shouldn't happen.

[RalfJung]

in most cases bootstrap sets rustflags consistently

Remember that the build cache might have been built with a previous version of bootstrap. Bootstrap doesn't set the flags consistently over time, a git pull can change them, and then it's important to do a rebuild.

[jieyouxu]

Hm, would a possible mechanism be to record which RUSTFLAGS a particular tool/lib was built with, then perform additional invalidation to the build cache from a bootstrap perspective, and not just from cargo (as the untracked env vars are not known to cargo)?

[RalfJung]

Well then we get rebuilds again. ;)

We really need to ensure that the flags are consistent. Maybe by using better APIs that avoid having to set the same flags in 3 different places, or so. It's not an easy problem.

[onur-ozkan]

Maybe by using better APIs that avoid having to set the same flags in 3 different places, or so.

We will do that already but it's not enough. We can create a mechanism to clear the cache when untracked/implicit rustflags (ones that are propagated to wrappers from bootstrap) change. I think this is the most sensible solution as it solves the main issue. In CI, it's already a one-shot build, and for developers it shouldn't be too much trouble to trigger a rebuild occasionally (this should be rare since we don't expect to change untracked rustflags too often).

[RalfJung]

We can create a mechanism to clear the cache when untracked flags change.

That's exactly what cargo already does if we tell it about the flags, so what's the benefit of that over the status quo?

The fix to "we have rebuilds because the flags are different" can't be "stop the rebuilds" (that replaces rebuilds by a worse bug: producing the wrong output), it has to be "make the flags not different". So moving the flag application into the RUSTC_WRAPPER doesn't help.

[jieyouxu]

The fix to "we have rebuilds because the flags are different" can't be "stop the rebuilds" (that replaces rebuilds by a worse bug: producing the wrong output), it has to be "make the flags not different". So moving the flag application into the RUSTC_WRAPPER doesn't help.

I wonder if we can do the other direction: enforce the rule where build caches may only be shared if they share the same RUSTFLAGS/RUSTDOCFLAGS, and ban conditional or "invisible to cargo" rustflags. This will mean less sharing between e.g. cargo and the other tools because the other tools want RUSTFLAGS += -Zon-broken-pipe=kill but cargo must not get it, however we might argue that using an untracked flag for tool cargo specifically to hide it from cargo is an antipattern already. That we have some workspace hacks doesn't make this very straightforward, I haven't looked at how possible this is with our current cargo setup.

[onur-ozkan]

That's exactly what cargo already does if we tell it about the flags, so what's the benefit of that over the status quo?

Sorry if I wasn't clear. I was thinking about a mechanism that triggers when things change (which can happen after git pull) in wrapper module.

The fix to "we have rebuilds because the flags are different" can't be "stop the rebuilds" (that replaces rebuilds by a worse bug: producing the wrong output), it has to be "make the flags not different". So moving the flag application into the RUSTC_WRAPPER doesn't help.

We are already aiming to do that, the problem is there are cases when you can't set the flag unconditionally for every cargo invocation, like here:

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

Lines 212 to 228 in 88f3114

	// NOTE: The root cause of needing `-Zon-broken-pipe=kill` in the first place is because `rustc`
	// and `rustdoc` doesn't gracefully handle I/O errors due to usages of raw std `println!` macros
	// which panics upon encountering broken pipes. `-Zon-broken-pipe=kill` just papers over that
	// and stops rustc/rustdoc ICEing on e.g. `rustc --print=sysroot | false`.
	//
	// cargo explicitly does not want the `-Zon-broken-pipe=kill` paper because it does actually use
	// variants of `println!` that handles I/O errors gracefully. It's also a breaking change for a
	// spawn process not written in Rust, especially if the language default handler is not
	// `SIG_IGN`. Thankfully cargo tests will break if we do set the flag.
	//
	// For the cargo discussion, see
	// <https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Applying.20.60-Zon-broken-pipe.3Dkill.60.20flags.20in.20bootstrap.3F>.
	//
	// For the rustc discussion, see
	// <https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Internal.20lint.20for.20raw.20.60print!.60.20and.20.60println!.60.3F>
	// for proper solutions.
	if !path.ends_with("cargo") {

By the way we already have multiple flags being set inside rustc wrapper, so I am not trying to add a new concept here but rather handle this sub-problem which already exists.

[onur-ozkan]

The fix to "we have rebuilds because the flags are different" can't be "stop the rebuilds" (that replaces rebuilds by a worse bug: producing the wrong output), it has to be "make the flags not different". So moving the flag application into the RUSTC_WRAPPER doesn't help.

I wonder if we can do the other direction: enforce the rule where build caches may only be shared if they share the same RUSTFLAGS/RUSTDOCFLAGS, and ban conditional or "invisible to cargo" rustflags. This will mean less sharing between e.g. cargo and the other tools because the other tools want RUSTFLAGS += -Zon-broken-pipe=kill but cargo must not get it, however we might argue that using an untracked flag for tool cargo specifically to hide it from cargo is an antipattern already. That we have some workspace hacks doesn't make this very straightforward, I haven't looked at how possible this is with our current cargo setup.

I don't think we should touch RUSTFLAGS at all and avoid logic conflict as it's already being handled by cargo.

[RalfJung]

Sorry if I wasn't clear. I'm was thinking about a mechanism that triggers when things change (which can happen after git pull) in wrapper module.

How is that different from cargo triggering a rebuild when flags change? If there is a logic bug and flags differ in expectedly, a rebuild is the safe thing to do! We should not override that logic, doing so will introduce subtle correctness bugs. The current situation is just that the APIs inside bootstrap are a bit messy and flags need to be set in multiple places to be consistently applied. That's the actual source of the issue.

[onur-ozkan]

How is that different from cargo triggering a rebuild when flags change?

cargo rebuild can happen on any command invocation, but my proposal doesn't trigger a rebuild unless the rustc wrapper (and possibly the module where we propagate the rustflags) is modified. If there are no changes to the rustc wrapper (and the module for propagating rustflags), we can assume that untracked rustflags haven't changed either.

The current situation is just that the APIs inside bootstrap are a bit messy and flags need to be set in multiple places to be consistently applied. That's the actual source of the issue.

It's not just that and I already wrote it in #131726 (comment).

[RalfJung]

cargo rebuild can happen on any command invocation, but my proposal doesn't trigger a rebuild unless the rustc wrapper (and possibly the module where we propagate the rustflags) is modified. If there are no changes to the rustc wrapper (and the module for propagating rustflags), we can assume that untracked rustflags haven't changed either.

Ah I see, you want to basically hash the src/bootstrap folder into the result of the builds it produces? Maybe there's a way to literally do that, by passing it to cargo as extra metadata.

It's not just that and I already wrote it in #131726 (comment).

Maybe the resolution here is to take a different approach to the "on broken pipe" flag? Doing this via a -Z flag anyway seems a bit strange to me. This is a library question, not a compiler question, isn't it? We don't usually control library behavior via compiler -Z flags. So we can have an unstable function in std to set the ON_BROKEN_PIPE_FLAG_USED flag. Or an (unstable) attribute on the main function that we expect rustc drivers (rustc, clippy, Miri, rustdoc) to pass.

[ChrisDenton]

You may want to raise that on the tracking issue: #97889. It was originally an attribute but was switched to a compiler flag to avoid polluting the language with library concerns.

[RalfJung]

Alternatively we could use support for per-package rustflags in cargo, and use that to set the flag on the binaries we want it for? Bootstrap doesn't have to do things that cargo can do just as well. ;)

See here for an example of what that may look like.

[jieyouxu]

The flags situation is more tricky than I initially figured:

• Depending on rustc/std/tools/others, some RUSTFLAGS need to be conditionally set.
• Lint flags sometimes goes through RUSTFLAGS but other times are explicitly set through a cargo-untracked env var to avoid lint config mismatch triggering rebuilds.
• proc-macros are... special, in relation to cross-compiling them with -Zdual-proc-macros: https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Enforcement.20of.20.60keyword_idents_2024.60.20in.20.60rustc_macros.60/near/479150151.
• There are other implications that I have not noticed yet, pending further investigation.

Temporarily unassigning myself because I still need to investigate it, and should not block any intermediate efforts.