download-rustc=if-unchanged logic is incredibly fragile

[RalfJung]

See here, which was already the second time that the logic caused trouble in less than a week (first time was fixed here).

I don't think a list of "folders that can influence the build" is maintainable. At the very least it'd have to include src/bootstrap since that computes a ton of env vars and flags that influence the build.

Judging from some of the comments that are hidden in the huge thread of https://github.com/rust-lang/rust/pull/122709, one motivation here is to prepare for a planned bootstrap change where rustc is built with the beta std. Is that the key motivation? Would be good to get that clarified.

I would argue that the way to achieve this goal is to have an allowlist of "folders where changes are okay", not a denylist of "folders where changes lead to redowload". That allowlist would contain library/ once the bootstrap change is made. For now, it is not clear to me what we can add to the allowlist... maybe src/doc and src/tools? src/tools/build-helper does have the chance to influence the build, but it shouldn't contain any actual logic, so it's "probably fine".

[onur-ozkan]

Cross ref: https://github.com/rust-lang/rust/pull/131560#discussion_r1798370110

[RalfJung]

Replying to this by @Kobzol:

Merge/rollup PRs that had no change in library nor compiler: 627 (22%)

Okay that's more than I expected.

The per-dir numbers are hard to interpret since we need the union of multiple folders, and of course there will be overlap between these categories.

Also this made me realize we can add tests to the allowlist above. That alone is probably actually the bulk of the benefit.

[onur-ozkan]

I would argue that the way to achieve this goal is to have an allowlist of "folders where changes are okay", not a denylist of "folders where changes lead to redowload". That allowlist would contain library/ once the bootstrap change is made. For now, it is not clear to me what we can add to the allowlist... maybe src/doc and src/tools? src/tools/build-helper does have the chance to influence the build, but it shouldn't contain any actual logic, so it's "probably fine".

This seems like a much better approach, I agree. I'm planning to work on that change in a couple of days (but feel free to take it on if you think you can do it sooner).

[cuviper]

Judging from some of the comments that are hidden in the huge thread of #122709, one motivation here is to prepare for a planned bootstrap change where rustc is built with the beta std.

AIUI, that's only for stage0, but the compiler should still use in-tree std for later stages. The idea is to make it so library/ never needs cfg(bootstrap) anymore -- the end result should still be full bootstrapped though.

I would argue that the way to achieve this goal is to have an allowlist of "folders where changes are okay", not a denylist of "folders where changes lead to redowload".

I do like this idea! But I think library/ can't be allowed even after the stage changes.

[Kobzol]

I can't really guarantee that my scripts work properly, but I tried to check how many PRs would be cacheable if we only allowed caching for PRs that touch tests and/or src/tools, and the result was about 10%.

[cuviper]

What else are so many PRs changing? Maybe books?

[RalfJung]

The idea is to make it so library/ never needs cfg(bootstrap) anymore -- the end result should still be full bootstrapped though.

Yeah, and instead rustc will need cfg(bootstrap). I am curious to see how this will go. :)

This seems like a much better approach, I agree. I'm planning to work on that change in a couple of days (but feel free to take it on if you think you can do it sooner).

Great to hear we found a solution that we can both agree on. :)

I won't have time to work on this.

I do like this idea! But I think library/ can't be allowed even after the stage changes.

Why not? The contents of library/ cannot influence the build of the stage 0 compiler under this model. And the stage 1 compiler anyway must always be built locally.