Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Laszlo Ersek

On 05/10/20 23:43, Rebecca Cran wrote:

On 5/10/20 3:29 PM, Michael D Kinney wrote:

There is no difference between CI checks run during code review
and the final CI checks before merge.  I think it is an interesting
conversation to decide how many times those CI checks should be
run and if they should run automatically on every change during
review or on demand.
I'd suggest following what other Github projects do, which I think is to
run the CI checks automatically on every change that's made in a pull
request - I don't know if it might also be necessary to run them during
the merge, if master has changed in the meantime. That gives the
_submitter_ feedback about any changes they need to make, instead of
having to wait until the maintainer tells them their change has broken
something: it speeds up the development process.
Build-testing at every stage through a patch series is important for
ensuring bisectability.

But there's a critical ingredient to that: based on the assumption that
our build system / build rules are good, the builds mentioned above
should be *incremental*.

That is, if we have a patch set with 10 patches, then then the first
patch in the series should trigger a complete build, and the 9 later
patches should trigger only incremental builds.

(During a bisection, the same commits wouldn't be visited in that same
order of course, but that's where the sanity of the build system / build
rules comes in! Basically, if your builds succeed with a linear
progression through the series, then the build system / build rules
ought to *guarantee* that the same "tree states" will build
incrementally just fine when visited in any particular order. "git
checkout" updates the relevant files, and the build system should be
able to derive the minimum set of necessary actions.

Anyway, digression ends.)

The incremental nature of builds is important for saving energy, and
also for saving developer time. The above 10-part example series should
not take 10 times as long to build as 10 independent patches, submitted
in isolation. Patches#2 through #10 should only rebuild a few modules
each (unless lib class headers, protocol headers and such are modified).

Mergify is more flexible.  We want to make sure the git history
is linear with not git merges and supports both single patches
and patch series without squashing.  GitHub merge button by
default squashes all commits into a single commit.
Wouldn't disabling all but "Allow rebase merging" do the same thing
without the additional potential failure point? Though it sounds like
we've resolved the problems with Mergify, so it's not important.
mergify has been pretty stable for me!


Join to automatically receive all group messages.