download-rustc=if-unchanged logic is too 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 #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: #131560 (comment)

[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.

[cuviper]

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.

I haven't checked the code, but at least the config.example.toml comments seem to say it does not build later stages either:

rust/config.example.toml

Lines 488 to 494 in 27861c4

	# Whether to download the stage 1 and 2 compilers from CI.
	# This is mostly useful for tools; if you have changes to `compiler/` or `library/` they will be ignored.
	#
	# Set this to "if-unchanged" to only download if the compiler and standard library have not been modified.
	# Set this to `true` to download unconditionally. This is useful if you are working on tools, doc-comments,
	# or library (you will be able to build the standard library without needing to build the compiler).
	#download-rustc = false

[RalfJung]

Given that, in my understanding, the entire goal of this is to make it possible to contribute to the library without having to rebuild all of rustc, that seems odd.

[onur-ozkan]

We could improve that along with the change-detection logic refactor. Also, download-rustc=true seems to make more sense for library development.

[RalfJung]

I don't want to have to change my rustc config every time I switch between library and compiler development, which I do regularly. There should be a single config that works for both (as is the case today).

[cuviper]

Given that, in my understanding, the entire goal of this is to make it possible to contribute to the library without having to rebuild all of rustc, that seems odd.

Once library/ doesn't have any cfg(bootstrap) logic, then you don't really need to use later stages to test it anymore. Just build/download rustc once, then do whatever with the library.

If the compiler download is only used for what would be the stage0->stage1 bootstrap build, then yes library can be allowed to change and still use the download. If you want to skip the stage1->stage2 compiler build, that does need to consider library, but this could be useful for tools for example.

[RalfJung]

Once library/ doesn't have any cfg(bootstrap) logic, then you don't really need to use later stages to test it anymore. Just build/download rustc once, then do whatever with the library.

Yes that's what I said, isn't it? I can change library and a ./x.py test library --stage 0 should still use the downloaded rustc.

[cuviper]

I was trying to refine what I said earlier. Yes, I agree library changes can be allowed in stage0, but not later stages.

[RalfJung]

Later stages should very rarely be needed. The only time I ever needed them in the last years is for x.py test clippy, due to #78717, which IMO is a bug in how clippy is wired up in bootstrap. I don't think we should ever try to download them -- it makes little sense: at that point stage 1 == stage 0, so we might as well do things one stage down.

[RalfJung]

Seems like this is now causing spurious test failures in #132846. See here for context.

IMO we should stop using download-rustc=if-unchanged on CI until a more robust system is in place. If we can't even trust that CI tests the actually latest version of the compiler, that's not a great place to be in.

[onur-ozkan]

We can close this since #131831 is merged.