FCP for toolstate changes

[Mark-Simulacrum]

Manual FCP for this change. Please leave a comment if you can't tick the box due to lack of permissions or, preferably, ping me on Discord/Zulip (@simulacrum).

The PR has a more detailed explanation of this change, but the general summary is that if we proceed with this change, submodule changes will no longer be gated when landing into master (modulo beta week). The theory behind this is that rollups can always include all toolstate-gated submodule changes, whereas currently it's pretty much impossible.

This is in a separate issue to avoid pinging a bunch of folks when the PR itself goes through review.

The people in the following list are the MAINTAINERS as known to toolstate itself. I don't necessarily expect/want buy-in from everyone, but at least 2-3 people per tool would be good.

miri:

• @oli-obk
• @eddyb
• @RalfJung

clippy-driver:

• @flip1995
• @llogiq
• @Manishearth
• @oli-obk
• @mcarton
• @phansch
• @yaahc

rls:

• @Xanewok

rustfmt:

• @topecongiro

book:

• @steveklabnik
• @carols10cents

nomicon:

• @frewsxcv
• @Gankra

reference:

• @steveklabnik
• @Havvy
• @ehuss
• @matthewjasper

rust-by-example:

• @marioidival
• @steveklabnik

embedded-book:

• @therealprof
• @adamgreig
• @thejpster
• @andre-richter
• @korken89
• @ryankurte
• @jamesmunns

edition-guide:

• @steveklabnik
• @ehuss
• @Centril

rustc-guide:

• @spastorino
• @amanjeev
• @mark-i-m

[RalfJung]

My main concern with this is that when I make a PR that updates Miri, I usually say it "Fixes: #xxxx" for the issue tracking that Miri is currently broken -- and with this change, that might end up being just wrong (so the issue will need manual re-opening).

[RalfJung]

The PR landing this contains the statement

In most cases, when trying to land a submodule (tool) change, you mostly want to just bump the sub-commit, and don't care too much if that actually fully fixes the tool (according to @Manishearth).

(Not sure why that statement is only made there, not in the discussion issue here.)

This is not the case for Miri. When I update Miri, I always intend that to be an update that either fixes Miri or keeps Miri working (but most of the time Miri was broken and gets fixed). Not a single time have I updated Miri when it was broken, and not cared about this fixing Miri. I see no point in updating Miri from one broken state to another.

[Manishearth]

Note: that statement was made specifically for clippy, which breaks quite often and sometimes incremental updates are needed.

I do feel that we should make this configurable per-tool.

and with this change, that might end up being just wrong (so the issue will need manual re-opening).

Note that when you go from broken -> broken this will still leave the warning comment which will let you know that stuff is still broken.

[RalfJung]

this will still leave the warning comment which will let you know that stuff is still broken.

So the intention is to post that comment if the PR either regressed the toolstate or [it both updated the submodule and did not fix the tool]?

[Manishearth]

Yes

[Mark-Simulacrum]

We can fairly easily make this change exclusive to some tools, e.g. maybe even just clippy for now, since most of the books get updated somewhat rarely in one rollup submodule bump PR.

I would personally want to see the issue get closed by the "bot" when we go back to working toolstate, leaving comments on the issue for every change to the relevant submodule, regardless of whether it actually changed the state or not.

[tmandry]

Not a single time have I updated Miri when it was broken, and not cared about this fixing Miri. I see no point in updating Miri from one broken state to another.

There's not much point in doing this deliberately. But if a change that is supposed to fix a tool gets added to a rollup and doesn't, it sinks the whole rollup today.

The biggest benefit of this from my perspective is that we can be much more aggressive about including toolstate updates in rollups, which should lead to lower latency on average for those changes.

[RalfJung]

I agree rollups are a good argument. It will make our life as Miri maintainers a bit worse, but given Miri is an experimental nightly tool and rollups are crucial for landing PRs these days, I can agree with the trade-off.

Some tooling that could make this easier for us is if the bot that created the issue "Miri is broken" could also automatically close that issue when Miri is fixed again. That way, we would not have to rely on GH closing the issue and there would problems with issues being closed when they should not. However, I do not know enough about the GH API to say how realistic such a feature is.

EDIT: Oh, @Mark-Simulacrum suggested exactly that. I agree. :)
However, I don't expect this to be a terribly common issue, so extending the bot like that does not have to block the change right now. It would just be good to track this somewhere.

[Gankra]

I still maintain that including these projects in-tree is the ideal solution. That way the tools cannot ever break, everyone is responsible for keeping them working, and mutual changes can be performed atomically. e.g. When an unstable API is changed, the uses of that API must also be changed, and there is no ordering that works -- both must happen at the same time.

"Everyone is responsible" is especially important because generally the person making a breaking change can trivially fix the tools (because they intimately understand the implications), but the maintainers of the tools just get sudden broken builds and they need to spend a ton of time asking around about what changed and how they're supposed to handle it.

Additionally I think it is a significant detriment to the various pieces of documentation that they are completely hidden from the folks that develop the features being documented. Being in tree, and therefore in the main queue, would help documentation get reviewed and triaged in a timely and robust fashion.

---

Alternatively, tools could have a two-tiered CI:

• master build: always builds with the rust master, warns on failure
• pinned build: always builds with that version, hard errors on failure

(For maintenance reasons, the pinned build should auto-bump whenever the master build passes)

This would allow us to distinguish between "the build is broken because rust-lang broke us" and "the build is broken because we made bad changes". This would also "fix" the atomic unstable API problem, as rust could move first, then the tools could update to work on the new rust, and then rust could update the submodule.

As it is, we have to rely on janky hacks where:

1. the tool makes a second branch that "should" work with the new rust implementation
2. rust updates and also repoints the submodule at that branch
3. the branch is merged into the tools master
4. rust updates to point at master

This is especially problematic because branch management is not something bors and friends support doing, requiring a github-acknolwedged admin of the repo to get involved. Step 1 also bypasses CI, so we can only catch problems at step 2, sending us back to step 1.

[Manishearth]

This would also "fix" the atomic unstable API problem, as rust could move first, then the tools could update to work on the new rust, and then rust could update the submodule.

As it is, we have to rely on janky hacks where:

We ... we don't have this problem or use those janky hacks? Clippy uses rustup-toolchain-install-master, so you can only land things in clippy if it builds on rustc master.

This does mean that when rustc breaks we can't land things, which we're usually fine with.

So when rustc breaks clippy, we halt other PRs and land a rustup in clippy -- if clippy's bors accepts it it will almost definitely work in rustc, and then we make a submodule update PR to rustc which usually lands.

We sometimes make branches if we're working with the person who's working on an upcoming breaking change. E.g. I think we did some of this when @Centril was working on the if AST refactorings, because we were concerned it would cause major breakage and wanted to do it smoothly. This is rare.

We basically kinda already have the two tiered system you describe: rustc's pinned submodule warns us when we're broken, and clippy's rustc master CI prevents us from breaking things.