search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.74k stars 12.64k forks source link

Tracking Issue for bootstrap spurious rebuilds #131726

Open jieyouxu opened 1 day ago

jieyouxu commented 1 day ago

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.

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 (https://github.com/rust-lang/rust/issues/131636#issuecomment-2412844566):

Discussion

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

RalfJung commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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: https://github.com/rust-lang/rust/blob/88f311479dd19288e5ad85e22cd80368489ce1e9/src/bootstrap/src/core/build_steps/tool.rs#L212-L228

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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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 https://github.com/rust-lang/rust/issues/131726#issuecomment-2413391602.

RalfJung commented 1 day ago

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 https://github.com/rust-lang/rust/issues/131726#issuecomment-2413391602.

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 commented 1 day ago

You may want to raise that on the tracking issue: https://github.com/rust-lang/rust/issues/97889. It was originally an attribute but was switched to a compiler flag to avoid polluting the language with library concerns.

RalfJung commented 1 day ago

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.