[llvm-dev] Notes from GitHub Pull Requests round table

[David Chisnall via llvm-dev]

Bors was the bot I've seen used for this the most, I've also seen a 
couple of other projects with hand-rolled things.

You can build staged-review into this with the workflow discussed 
previously where there's a full-feature PR and a number of PRs that are 
raised against that PR's branch.  You then merge from the edge inwards 
and only the final wrap-up PR goes through this process.

I've also seen in proprietary projects an approach that does a little 
bit of speculation and tries running CI on the result of applying all of 
the pending merges first and falls back to one-by-one or binary search 
to find the failing commit only if the combined patch doesn't pass.  I 
don't think bors has that option, but it might not be too hard to add.

David

On 20/11/2020 01:29, Thomas Lively wrote:
> The Rust project uses this head-always-green model very successfully. 
> They have a github bot called bors that manages testing and merging PRs 
> once they have been approved by code owners. The bot interface is very 
> flexible and the bot has been extracted into its own project: 
> https://bors.tech/ <https://bors.tech/>. You can see it turning through 
> all the approved Rust PRs here: https://bors.rust-lang.org/queue/rust 
> <https://bors.rust-lang.org/queue/rust>. The only downside of this 
> system is that it can take a long time for a PR to be automatically 
> merged since PRs generally need to be tested one at a time to prevent 
> semantic merge conflicts. The fix for that is to create "rollups" in 
> which multiple PRs are tested and landed as a single unit. I'm not sure 
> whether this system would scale to the commit frequency we have in LLVM, 
> though.
> 
> On Thu, Nov 19, 2020 at 2:39 PM Mehdi AMINI via llvm-dev 
> <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> 
> 
> 
>     On Thu, Nov 19, 2020 at 1:56 AM David Chisnall via llvm-dev
>     <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> 
>         On 18/11/2020 17:36, Mike Edwards via llvm-dev wrote:
>          > Hi Keith,
>          > You should be able to access the notes here:
>          >
>         https://docs.google.com/document/d/1flP8TqS71x4KF6h98vZV3ctXADBD39_vjrXn_BQn4hQ/edit?usp=sharing
>         <https://docs.google.com/document/d/1flP8TqS71x4KF6h98vZV3ctXADBD39_vjrXn_BQn4hQ/edit?usp=sharing>
> 
>          >
>         <https://docs.google.com/document/d/1flP8TqS71x4KF6h98vZV3ctXADBD39_vjrXn_BQn4hQ/edit?usp=sharing
>         <https://docs.google.com/document/d/1flP8TqS71x4KF6h98vZV3ctXADBD39_vjrXn_BQn4hQ/edit?usp=sharing>>
>          > Let me know if you run into any issues.
> 
>         Thanks.  There's one issue that isn't directly relevant to PRs,
>         but is
>         likely to become more visible if we go to a PR workflow:
> 
>         LLVM takes quite a long time to run the tests and there's
>         usually one
>         commit in between running the test suite on a patch and pushing
>         it to
>         master.  If the result then causes test failures, you won't find
>         out
>         until some time later.  This means I'm generally only willing to
>         hit the
>         merge button near to the start of the day, when I'm likely to be
>         around
>         and paying attention when the failure notices come in, which
>         increases
>         the likelihood that there will be an intermediate patch that causes
>         problems.
> 
>         With a PR-based workflow, it becomes fairly easy to set up a
>         system to
>         manage a queue of patches to be merged, triggered by either a
>         comment
>         from a member of the project or a label (e.g. ready-to-merge) being
>         added to the PR.  The CI infrastructure then rebases the PR on
>         the PR
>         that's ahead of it in the queue (master if it's the front of the
>         queue),
>         kicks off the build, and fast-forwards master when it succeeds. 
>         Head is
>         always in a state where the build bots are happy and if an
>         intermediate
>         change causes problems then that's reflected in the PR that
>         would see
>         the failure, nowhere else.  Subsequent PRs in the queue can
>         rebased on
>         the last passing version and the merge process can continue
>         without a
>         human needing to notice the buildbot failures and revert anything.
> 
> 
>     Such a system is definitely possible, but I wouldn't consider it
>     "fairly easy". Do you have examples of OSS infra to achieve this?
>     I also don't believe such a system is a guarantee of "always green
>     master branch":
>     1) you have flaky test
>     2) it is unlikely that your pre-merge testing can cover the entire
>     matrix of configurations supported by the project: all configs
>     (assert/no assert, build with shared libraries, etc.) time all
>     sanitizers times all subprojects times all host and target platforms?.
> 
>     (also implementing such a system from scratch does not really seem
>     harder conceptually with Phabricator patches vs GitHub PR:
>     fundamentally if a phabricator patch can't be applies on top of
>     current HEAD, then the PR couldn't be rebased without conflict
>     either for example, ultimately after `arc patch` you are
>     manipulating the same git branches locally in your CI).
> 
>     Thanks,
> 
>     -- 
>     Mehdi
>     _______________________________________________
>     LLVM Developers mailing list
>     llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>     https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>     <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
>