[edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
Michael D Kinney
michael.d.kinney at intel.com
Wed Jul 7 06:00:29 UTC 2021
Hi Laszlo,
I did many experiments and could not get the exact behavior I proposed.
Here is the best I can do with the behavior of GitHub and Mergify:
1) I further simplified Mergify configuration so personal builds ('push'
label not set) are no longer auto closed. Any developer doing a
personal build that wants to abandon the change should manually close
the PR. We may need to periodically review PRs that have been open
for an extended period of time and close them and developers can reopen
if that was a mistake.
2) The 'push' label always does the safest possible rebase and merge.
If many PRs are queued at a time, then each one is rebased in turn,
all CI checks are run and if all CI checks pass, then PR is
added with linear history to the base branch.
3) If a maintainer wants to manually send a sequence of PRs through
one at a time and review the state before sending the next one, then
I recommending sending each PR as a personal build (always rebase against
latest base branch before submitting PR). Review the commits and status
checks. If the PR looks good and passes all status checks and the state
from GitHub is that the PR is 'up-to-date' with the base branch, then
the maintainer can set the 'push' label and the PR is merged immediately
without re-running the status checks since there have been no changes.
If other PRs were merged into the base branch while the PR status checks
were run, then the personal build will show that the branch is out-of-date
with the base branch. The maintainer can do a local rebase and review
the changes and do a forced push of the updated PR. This will trigger the
personal build again. If that passes and the branch is 'up-to-date', then
the 'push' label can be set to immediately merge. Repeat as needed.
NOTE: If there is a lot of PR activity from other maintainers at the same
time as this manual process is being used, then the manual process may
have to rebase and force push several times until there is a quiet window.
The simplified Mergify configuration file is shown below.
queue_rules:
- name: default
conditions:
- base~=(^main|^master|^stable/)
- label=push
pull_request_rules:
- name: Automatically merge a PR when all required checks pass and 'push' label is present
conditions:
- base~=(^main|^master|^stable/)
- label=push
actions:
queue:
method: rebase
rebase_fallback: none
name: default
- name: Post a comment on a PR that can not be merged due to a merge conflict
conditions:
- base~=(^main|^master|^stable/)
- conflict
actions:
comment:
message: PR can not be merged due to conflict. Please rebase and resubmit
Let me know if this process of using personal builds and setting 'push' label
if PR is 'up-to-date' is acceptable. I have tested this process with a few
different scenarios in the edk2-codereview repo, and they all worked as expected.
Best regards,
Mike
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Monday, June 28, 2021 5:23 AM
> To: Kinney, Michael D <michael.d.kinney at intel.com>; devel at edk2.groups.io; spbrogan at outlook.com; ardb at kernel.org
> Cc: Peter Grehan <grehan at freebsd.org>; Ard Biesheuvel <ardb+tianocore at kernel.org>; Justen, Jordan L
> <jordan.l.justen at intel.com>; Sean Brogan <sean.brogan at microsoft.com>; Rebecca Cran <rebecca at bsdio.com>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
>
> On 06/24/21 00:07, Kinney, Michael D wrote:
> > Hi Laszlo,
> >
> > I understand your point.
> >
> > I am trying to balance the ease of use for developers, reducing overhead for maintainers, and
> > prevent bad commits.
> >
> > I think you are saying that you want to make sure a maintainer carefully reviews changes
> > across multiple PRs that are in the same area of code. The CI checks will of course make
> > sure the code builds and passes the basic boot tests, but those tests do not have full
> > coverage so an interaction issue between two PRs that pass build and boot but have
> > unintended behavior side effects are what require detailed manual review.
> >
> > I am going to remove the auto-rebase by default and add a optional label that can
> > be set by a maintainer to enable auto-rebase. If a maintainer is confident that
> > a set of PRs being submitted at the same time with the 'push' label are independent,
> > then the maintainer can also set 'auto-rebase'. If they are not confident, then
> > they can send PRs one at a time with only 'push' label and manually rebase each
> > additional PR and review the manual rebase to make sure there are no unintended
> > side effects.
>
> Sounds great, thank you!
> Laszlo
>
> >
> > Any objections to this direction?
> >
> > Thanks,
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Laszlo Ersek
> >> Sent: Wednesday, June 23, 2021 12:45 PM
> >> To: Kinney, Michael D <michael.d.kinney at intel.com>; devel at edk2.groups.io; spbrogan at outlook.com; ardb at kernel.org
> >> Cc: Peter Grehan <grehan at freebsd.org>; Ard Biesheuvel <ardb+tianocore at kernel.org>; Justen, Jordan L
> >> <jordan.l.justen at intel.com>; Sean Brogan <sean.brogan at microsoft.com>; Rebecca Cran <rebecca at bsdio.com>
> >> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> >>
> >> On 06/23/21 20:44, Kinney, Michael D wrote:
> >>>
> >>> Hi Laszlo,
> >>>
> >>> Thank you for the test case.
> >>>
> >>> I created 2 PRs against edk2-codereview using your patches.
> >>> I made minor update to commit messages to pass patch check.
> >>>
> >>> https://github.com/tianocore/edk2-codereview/pull/18
> >>> https://github.com/tianocore/edk2-codereview/pull/19
> >>>
> >>> Found another issue with PatchCheck for the Mergify merge commit and
> >>> fixed that.
> >>>
> >>> Mergify did process #18 and merged it in after passing all CI. Mergify
> >>> rebased #19 successfully and merged it after passing all CI. I do not
> >>> think this was your expected result.
> >>
> >> Indeed, my "desired" result at least would have been for mergify to
> >> reject the rebase.
> >>
> >>> I looked more closely at the patches you provided. They were not
> >>> overlapping in the lines of Readme.rst. This is why no merge conflict
> >>> was detected.
> >>
> >> More precisely, a contextual conflict *was* determined between the
> >> patches, but that conflict was auto-resolved.
> >>
> >> This is risky when done in an automated fashion. It is an extremely
> >> convenient feature of git, when used interactively; that is, when the
> >> auto-merge (automatic conflict resolution) is semantically verified by a
> >> human. Git takes away the chore of conflict resolution, presents a
> >> "likely good" end result, and a human only needs to *look* at the end
> >> result, not *implement* it.
> >>
> >> But that "human look" is exactly what's missing from mergify.
> >>
> >> Basically what I'd like for mergify is to turn off automatic conflict
> >> resolution.
> >>
> >> More or less, speaking in terms of the stand-alone "patch" utility
> >> <https://man7.org/linux/man-pages/man1/patch.1.html>, my preference is
> >> to set the "fuzz factor" to zero.
> >>
> >>
> >> One way a human reviews such context differences is with git-range-diff.
> >> Continuing my previous example commands:
> >>
> >> $ git range-diff --color master..b2 b1..b2-rebase
> >>
> >> 1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world
> >> @@ -6,8 +6,8 @@
> >> --- a/ReadMe.rst
> >> +++ b/ReadMe.rst
> >> @@
> >> -
> >> A modern, feature-rich, cross-platform firmware development
> >> + HELLO
> >> environment for the UEFI and PI specifications from www.uefi.org.
> >> + WORLD
> >>
> >> This output shows that the "world" addition is the same (it is identical
> >> between pre-rebase and post-rebase in the commit), but the context has
> >> changed. During the rebase, the leading empty line of the context
> >> disappeared, and a HELLO line in the middle of the leading context
> >> appeared.
> >>
> >> This result may or may not be semantically correct; it needs a human
> >> decision. What if the original purpose of the "world" patch author was
> >> to say WORLD but only without HELLO? When they looked at the code, there
> >> was no HELLO yet.
> >>
> >> git-range-diff is very powerful, but reading its output takes some
> >> getting used to. (Colorization with the "--color" option is basically
> >> required for understanding; I can't reproduce it in this email, alas.)
> >>
> >> I don't want to obsess about this forever, I just want us all to be
> >> aware that this risk exists.
> >>
> >> Thanks,
> >> Laszlo
> >>
> >>>
> >>> I then created 2 new PRs that added text to the same line # in Readme.rst.
> >>>
> >>> https://github.com/tianocore/edk2-codereview/pull/21
> >>> https://github.com/tianocore/edk2-codereview/pull/22
> >>>
> >>> PR #21 passed all CI tests and was merged. Mergify then attempted to
> >>> rebase #22 and got a merge conflict and is still in the open state waiting
> >>> for the developer to manually handle the merge conflict.
> >>
> >> This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned.
> >>
> >> My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things
> >> exist.
> >>
> >> Thanks
> >> Laszlo
> >>
> >>
> >>
> >>
> >>
> >
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77546): https://edk2.groups.io/g/devel/message/77546
Mute This Topic: https://groups.io/mt/83497624/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list