Skip to content

Commit 89b9e58

Browse files
committed
Improved rollup selection criteria
1 parent 353d9dc commit 89b9e58

File tree

1 file changed

+19
-17
lines changed

1 file changed

+19
-17
lines changed

src/release/rollups.md

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -38,32 +38,34 @@ queue has been merged.
3838
2. Run the following command in the pull request thread:
3939

4040
```
41-
@bors r+ rollup=never p=<NUMBER_OF_PRS_IN_ROLLUP>
41+
@bors r+ rollup=never p=5
4242
````
4343
4444
3. If the rollup fails, use the logs rust-log-analyzer
4545
provides to bisect the failure to a specific PR and do
4646
`@bors r-`. If the PR is running, you need to do `@bors r- retry`. Otherwise,
4747
your rollup succeeded. If it did, proceed to the next rollup (every now and
4848
then let `rollup=never` and toolstate PRs progress).
49-
4. Recreate the rollup without the offending PR starting again from **1.**
49+
4. Recreate the rollup without the offending PR starting again from **1.**. There's a link in the rollup PR's body to automatically prefill the rollup UI with the existing PRs (minus any PRs that have been `r-`d)
5050
5151
## Selecting Pull Requests
5252
53-
This is something you will learn to improve over time. Some basic tips include
54-
(from obvious to less):
55-
56-
1. Avoid `rollup=never` PRs (these are red in the interface).
57-
2. Include all PRs marked with `rollup=always` (these are green). Try to check
58-
if some of the pull requests in this list shouldn't be rolled up — in the
59-
interest of time you can do so sometimes after you've made the rollup PR.
60-
3. Avoid pull requests that touch the CI configuration or bootstrap.
61-
(Unless the CI PRs have been marked as `rollup`. -- see 2.)
62-
4. Avoid having too many large diff pull requests in the same rollup.
63-
5. Avoid having too many submodule updates in the same rollup (especially LLVM).
64-
(Updating LLVM frequently forces most devs to rebuild LLVM which is not fun.)
65-
6. Do not include toolstate PRs like those fixing Miri, Clippy, etc.
66-
7. Do include docs PRs (they should hopefully be marked as green).
53+
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.
54+
55+
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 `never`s, but more experienced people might even make a rollup of 1-2 `iffy`s, 8 `maybe`s, and 10 `never`s! Massive rollups are rarely needed, but as your intuition grows you'll get better at judging risk when including PRs in a rollup.
56+
57+
Don't hesitate to downgrade the rollup status of a PR! If your intuition tells you that a `rollup=always` PR has some chances for failures, mark it `rollup=maybe` or `rollup=iffy`. A lot of the unmarked `maybe` PRs are categorized as such because the reviewer may not have considered rollupability, so it's always worth picking them with a critical eye. Similarly, if a PR causes your rollup to fail, it's worth considering changing its rollup status
58+
59+
Generally, PRs, that touch CI configuration or the bootstrapping process are probably `iffy` and should be handled with care. On the other hand, PRs that just edit docs are usually `rollup=always`.
60+
61+
Avoid having too many PRs with large diffs or submodule changes in the same rollup. Also avoid having PRs you suspect will have large perf impacts, and mark them as `rollup=never`.
62+
63+
It's tempting to avoid including `iffy` PRs at all since ideally you want your rollup to succeed. However, it's worth remembering that the job of the PR queue is to _test_ PRs, not to land them. As such, a rollup that fails because of an `iffy` PR is a good thing, since that PR would have to be tested at _some point_ anyway and it would have taken up the same amount of time to test if it never got included in a rollup. One way to look at rollups when it comes to `iffy` PRs is that a rollup is a way for a bunch of other PRs to piggyback on the CI cycle that the `iffy` PR needs anyway. If rollups avoid `iffy` PRs entirely what ends up happening is that these PRs tend to languish in the queue for a long time, which isn't good.
64+
65+
Similarly, make sure to leave some spare CI cycles so that `never` PRs also get a chance! If you're the only person making rollups it's worth letting them run during times you're not paying attention to the queue, but these days there are rollup authors in multiple time zones, so it's often best to just keep an eye on the relative size of the queue and put aside a couple CI cycles for `never` PRs, especially if they pile up.
66+
67+
Try to be fair with rollups: Rollups are a way for things to jump the queue. For `rollup=maybe` PRs, try to include the oldest one (at the top of the section) so that newer PRs aren't jumping the queue over older PRs entirely. You don't have to include every PR older than PRs included in your rollup, but try to include the oldest. Similar to the perspective around `iffy`, it's useful to look at a rollup as a way for other PRs to piggyback on the CI cycle of the oldest PR in queue.
68+
6769
6870
## Failed rollups
6971
If the rollup has failed, run the `@bors retry` command if the
@@ -77,7 +79,7 @@ Once you've removed the offending PR, re-create your rollup without it (see 1.).
7779
Sometimes however, it is hard to find the offending PR. If so, use your intuition
7880
to avoid the PRs that you suspect are the problem and recreate the rollup.
7981
Another strategy is to raise the priority of the PRs you suspect,
80-
mark them as `rollup=never` and let bors test them standalone to dismiss
82+
mark them as `rollup=never` (or `iffy`) and let bors test them standalone to dismiss
8183
or confirm your hypothesis.
8284
8385
If a rollup continues to fail you can run the `@bors rollup=never` command to

0 commit comments

Comments
 (0)