I saw some other posts on this list about notes from the round tables at the conference. Did anyone take some for the GitHub round table?
Thanks!
Hi Keith,
You should be able to access the notes here: https://docs.google.com/document/d/1flP8TqS71x4KF6h98vZV3ctXADBD39_vjrXn_BQn4hQ/edit?usp=sharing
Let me know if you run into any issues.
Thanks,
Mike
Thanks for posting these notes. I wasn’t able to attend the Round Table due to scheduling conflicts (I had just moved house that week, and the conference was outside my usual work hours, so…). It’s good that people investigating the viability of other workflows, like using GitHub PRs, I just want to make sure that once any investigation has been done, it isn’t presented as some sort of fait accompli, discussed and agreed upon outside the mailing list, when there are a number of people with strong opinions against moving from Phabricator to GitHub PRs (based on the last time this discussion was raised on the mailing list). As the review process is pretty fundamental to the workflow, it’s important that changing it gets widespread agreement, or there’s a risk of alienating many developers.
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.
David
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”:
- you have flaky test
- 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,
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/. You can see it turning through all the approved Rust PRs here: 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.
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