update Rollups process #775
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) some time within the next two weeks. |
rustbot
added
the
S-waiting-on-review
|
|
||
| ## Selecting Pull Requests | ||
|
|
||
| The queue is sorted by rollup status. In general, a good rollup includes one or two `iffy` PRs (if available), a bunch of `maybe` (unmarked) PRs, and a large pile of `always` PRs. A rollup should never include `rollup=never` PRs. | ||
| The queue is sorted by rollup status. In general, a good rollup contains a bunch of `maybe` (unmarked) PRs, and a large pile of `always` PRs. You can include one or two `iffy` PRs if you are confident that they will pass. |
There was a problem hiding this comment.
I'm not sure including "iffy" PRs is a good idea in rollups. I personally tend to never includ them.
There was a problem hiding this comment.
That's kinda the whole reason we have "iffy". If you never include them, then they may as well be marked "never".
There was a problem hiding this comment.
I personally consider iffy PRs to be the whole point of rollups, tbh. Some PRs are clearly rollup=never if you are almost certain they will fail or if they have perf implications, but some of the iffy PRs are for the "it can fail, but we're not too sure" cases: it's great if it passes full CI, in which case we saved a bunch of time. It's also great if it fails full CI, in which case the failure time is amortized.
| If a PR in a rollup continues to fail you can run the `@bors rollup=never` command to | ||
| never rollup the PR in question. |
There was a problem hiding this comment.
| If a PR in a rollup continues to fail you can run the `@bors rollup=never` command to | |
| never rollup the PR in question. | |
| If a PR in a rollup continues to fail you can run the `@bors rollup=never` command to | |
| ensure the PR gets tested independently, since it's likely it will fail again in the future. |
| Sometimes however, it is hard to find the offending PR. If so, use your intuition | ||
| to avoid the PRs that you suspect are the problem and recreate the rollup. | ||
| Another strategy is to raise the priority of the PRs you suspect, | ||
| mark them as `rollup=never` (or `iffy`) and let bors test them standalone to dismiss | ||
| or confirm your hypothesis. | ||
| or confirm your hypothesis, or split the rollup into 2 smaller ones until are certain of the failure cause. If a PR was found to be the cause and other PRs were "wrongfully" `iffy`'d, they can of course be reprioritised as `maybe` again. |
There was a problem hiding this comment.
| or confirm your hypothesis, or split the rollup into 2 smaller ones until are certain of the failure cause. If a PR was found to be the cause and other PRs were "wrongfully" `iffy`'d, they can of course be reprioritised as `maybe` again. | |
| or confirm your hypothesis, or split the rollup into 2 smaller ones until you are certain of the failure cause. If a PR was found to be the cause and other PRs were "wrongfully" marked `iffy`, they can of course be reprioritised as `maybe` again with `@bors rollup=maybe` or `@bors rollup-`. |
| at fault. The failed rollup PR can be closed. | ||
| failure was spurious (e.g. due to a network problem or a timeout). | ||
| There may be a matching `CI-spurious-fail-.*` label that you can use to tag the PR as such, to help discover common fail patterns. | ||
| If it wasn't spurious, find the offending PR and throw it out by copying a link to the rust-logs-analyzer comment, |
There was a problem hiding this comment.
| If it wasn't spurious, find the offending PR and throw it out by copying a link to the rust-logs-analyzer comment, | |
| If it wasn't spurious, find the offending PR and return it to the author to be fixed by copying a link to the rust-log-analyzer comment, |
|
|
||
| ## Selecting Pull Requests | ||
|
|
||
| The queue is sorted by rollup status. In general, a good rollup includes one or two `iffy` PRs (if available), a bunch of `maybe` (unmarked) PRs, and a large pile of `always` PRs. A rollup should never include `rollup=never` PRs. | ||
| The queue is sorted by rollup status. In general, a good rollup contains a bunch of `maybe` (unmarked) PRs, and a large pile of `always` PRs. You can include one or two `iffy` PRs if you are confident that they will pass. |
There was a problem hiding this comment.
That's kinda the whole reason we have "iffy". If you never include them, then they may as well be marked "never".
|
|
||
| The actual absolute size of the rollup can depend based on experience, people new to making rollups might start with including 1 `iffy`, 4 `maybe`s, and 5 `always`s, but more experienced people might even make a rollup of 1-2 `iffy`s, 8 `maybe`s, and 10 `always`s! Massive rollups are rarely needed, but as your intuition grows you'll get better at judging risk when including PRs in a rollup. | ||
| Don't try to make mega-rollups (15-20 PRs that merge half or more of the entire queue all at once) to keep the number of perf or bug regressions per merge as low as possible and keep potential regressions [bisectable]. |
There was a problem hiding this comment.
| Don't try to make mega-rollups (15-20 PRs that merge half or more of the entire queue all at once) to keep the number of perf or bug regressions per merge as low as possible and keep potential regressions [bisectable]. | |
| Limit the size of rollups, even if the queue is backed up -- large rollups run the risk of failing or merge conflicts, and smaller rollups keep potential regressions [bisectable]. On average, rollups are <N> PRs large, often varying from <N - M> to <N + M> depending on the number of `rollup=always` PRs that can be included. |
There was a problem hiding this comment.
choose some value for N and M.
There was a problem hiding this comment.
note that thanks to unrolled builds, bisection can be done within a roll-up and cargo bisect rustc does that
| ``` | ||
| @bors r+ rollup=never p=5 | ||
| ```` | ||
|
|
||
| where 5 is the number of PRs contained in your rollup. |
There was a problem hiding this comment.
I have been thinking that what the existing documentation says (rollups always p=5) might be better than p=pr_count that is usually done. There are a lot of cases where 7 PRs get rolled up with p=7 and sit in the queue for a number of hours, only to be leapfrogged by a p=8 rollup that contains much newer PRs. Seems to have a tendency to break FIFO.
There was a problem hiding this comment.
Yes that's why I added the "Rollups should not overlap" rule.
There was a problem hiding this comment.
we can just ping the contributors who are assigning rollup p>5 (I've done this a couple of times myself) and just point them to p=5 being the good default and having that respect queue order. It helps to elaborate on the reasoning for choice of p=5 and not arbitrary p=N where N is the number of PRs rolled up.
If we update the docs here and then still notice contributors assigning p > 5 to rollups, we can just ping them and point them to the update docs here.
There was a problem hiding this comment.
(Thanks for the edits)
Just wanted to say, we should make these rollup advice as accessible and useful to onboard other contributors w/ r+ perms who may want to help to do rollups too.
There was a problem hiding this comment.
(Somewhat unrelated, but also I feel like the p=??? advice is at times too vague to be useful, at least when I read some of the forge docs related to p=??? advice)
There was a problem hiding this comment.
Maybe we should have something like
tool/subtree updates p=5 and rollups p=5 as default, so that we still have some headroom to 4 or 3 things if needed
There was a problem hiding this comment.
Yeah, I feel like the tool/subtree updates should receive same priority as rollups and all pinned at p=5 exactly, good point.
There was a problem hiding this comment.
mmh imo it makes sense to prioritise subtree/module updates over rollups since doing a tool sync is often more complicated than rebasing PRs
There was a problem hiding this comment.
Just, they should receive a consistent p between themselves, but otherwise it feels whatever to me.
cc @compiler-errors @jieyouxu @GuillaumeGomez
Rendered