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.

[topecongiro]

@Mark-Simulacrum I am ok with this change, but I cannot mark the checkbox.

[RalfJung]

Agreed, generally the toolstate system works fairly well for Miri as well. Like clippy, we pin a rustc commit. This actually means even when rustc master is broken, some development can still happen on Miri as we can keep using the old pinned rustc, until someone bumps it. We also have a nightly Travis cron job so independent of the toolstate system we know if Miri works with the latest nightly rustc.

It does sometimes happen that after bumping rustc once, we realize that to bump it all the way to master we need another Miri fix. That is fairly annoying as this time we get no clear hint which PR is the culprit. So I agree with the part where whoever changed rustc in a way that broke Miri most likely is in the best position to also fix Miri. But OTOH, my understanding is that we deliberately shift this burden onto Miri maintainers to reduce the load on contributors.

[Gankra]

yes the effort/impact calculus is a bit different between An Entire Interpretter and The Docs (I was tagged into this thread for the docs I maintain).

[Centril]

But OTOH, my understanding is that we deliberately shift this burden onto Miri maintainers to reduce the load on contributors.

Fwiw I would personally be happy with moving both Miri and Clippy in-tree. :)

(The reference and the nomicon not so much because they are basically just a bunch of text files you can edit via the GH UI.)

[oli-obk]

Maybe we could use a git subtree scheme so that we can have the benefits of a separate repo for features combined with the benefits of the monorepo

[tmandry]

Pulling tools in-tree is a separate discussion IMO, and it seems like not everyone agrees. For now I'd like to get this through, so we can have greener toolstate with minimal changes to our process.

What's left? It looks like we still need some people to check their box - someone from nomicon (@Gankra, @frewsxcv) and someone from rust-by-example (@marioidival, @steveklabnik). But the manual FCP has been going for over a month with no one actually opposed, from what I can tell.

[Mark-Simulacrum]

My feeling is that we can probably move forward with the change, but we don't yet have an implementation. I need to spend a solid chunk of time to figure out the current implementation of toolstate; I have not had sufficient time to do so recently. I'm hoping to dedicate a few hours this weekend to hopefully get a PR up, though, so if nothing else comes up that'll likely happen.

[steveklabnik]

I've checked my boxes, I don't have a strong opinion here and it seems like everyone thinks this is a good change.

[jyn514]

It looks like the changes here were never implemented (#64977 was never merged). I am not sure toolstate as a concept still exists, really - all the tools that could break (clippy, rustfmt, miri) are now subtrees instead, and cargo can't be broken because it doesn't depend on compiler internals.

I'm going to close this as unplanned and use #79249 as the issue to track cleaning up bootstrap to make it more clear that toolstate isn't used any more.

[RalfJung]

Toolstate is still a thing for some of the books: https://rust-lang-nursery.github.io/rust-toolstate/

[jyn514]

Got it, thank you (I guess "toolstate" is no longer a good name then). I think books can't fail to compile in the same way tools can though, right? or is it possible to somehow make a change to the compiler that breaks books?

[RalfJung]

It's about the doctests in the books I think. But I am pretty much out of the loop here, you'll have to ask the book maintainers. (And yeah "toolstate" is not a great name any more.)

[jyn514]

https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/toolstate