Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices

["Laszlo Ersek" <lersek@redhat.com>]

On 1/12/23 14:38, Ard Biesheuvel wrote:
> On Thu, 12 Jan 2023 at 13:24, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 1/12/23 00:08, Dionna Glaze via groups.io wrote:
>>> On Thu, Nov 10, 2022 at 8:55 AM Kinney, Michael D
>>> <michael.d.kinney@intel.com> wrote:
>>>>
>>>> Hi Dionna,
>>>>
>>>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
>>>>
>>>> I think you are at step 13.  If you have not done so already,
>>>> please update the commit messages with all the Reviewed-by and
>>>> Acked-by tags and make sure your branch is rebased against the
>>>> latest edk2/master and update the PR with that content and verify
>>>> that all EDK II CI checks pass.
>>>>
>>>> At that point, it is the responsibility of the EDK II Maintainers
>>>> to review the final state of the PR and set the "push" label if
>>>> everything looks correct.

I didn't realize this had been adopted -- "upgrading" a contributor
submitted PR. (More below.)

>>>> At this moment, we are in Soft Freeze and will be in Hard Freeze
>>>> for the next 2 weeks.  If this is considered critical for the
>>>> stable tag release, then please send a request to Liming with
>>>> justification for stable tag.  Otherwise, it will be merged after
>>>> the stable tag release.
>>>>
>>>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
>>>>
>>>> Thanks,
>>>>
>>>> Mike
>>>>
>>>
>>> Hi Mike, my PR was closed without comment
>>> https://github.com/tianocore/edk2/pull/3623 so I rebased and created
>>> a new PR after the holidays and hard freeze. I hope this isn't
>>> considered bad practice https://github.com/tianocore/edk2/pull/3885
>>>
>>
>> I closed the PR -- similarly to how I manually closed 400+ stale PRs
>> then -- because it was not a maintainer-submitted PR with the "push"
>> label set. In other words, it was just a personal build, for
>> triggering a CI run.
>>
>> And, in fact regardlessly of whether it was a "push"-labeled
>> maintainer PR or just a personal PR, the PR was almost two months
>> old. Clearly stale and abandoned -- per edk2 contribution workflow,
>> anyway.
>>
>> Please excuse me for not explaining this in a comment on the PR, but
>> (as I said above) I closed 400+ stale PRs manually within 10-15
>> minutes on the github.com WebUI. The necessary muscle memory training
>> didn't allow for individual comments.
>>
>> (I actually tried scripting the mass-closure at first, with the "gh"
>> utility, but something in the github.com data model was broken, and
>> the server wouldn't allow me to close those PRs with the utility,
>> even though I had authenticated the utility under my account, with a
>> complete set of scopes -- see my report at
>> <https://github.com/cli/cli/discussions/6814>.)
>>
>> Regarding your new PR: it's again good for a personal CI run only. If
>> it completes fine, please post the patches to the mailing list, using
>> git-send-email.
>>
>> (From browsing recent list traffic, it seems that your colleague
>> Moritz Fischer <moritzf@google.com> has posted patches like that to
>> the list; please consider consulting with Moritz regarding the git
>> setup (SMTP server etc) withing Google. Cc'ing Moritz now.)
>>
>> When sending the patches like that, please CC the maintainers and
>> reviewers that are supposed to review them. Once they are happy with
>> the series, one of them will submit a PR with them, and set the
>> "push" label on the PR. When the CI run succeeds, the mergify bot
>> will merge *that* PR automatically.
>>
>
> I think we were already way past this with Dionna's work - numerous
> iterations have been posted and discussed, and the merge of the final
> version was only deferred because of the soft freeze.
>

That may very well be, but the specific PR I closed (3623) was not
queued by a maintainer, nor did it have the "push" label. So I'd expect
that now a maintainer has to submit a new PR, with the patches most
recently approved on the list.

The official documentation in the wiki at
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process#the-maintainer-process-for-the-edk-ii-project>
still says the maintainer has to use "git-am" for picking up the
patches, and to submit a new (separate) PR, with the "push" label set.
So I wouldn't expect the "push" label to be set, as an additional
maintainer action, on a contributor-submitted (pre-existent) PR.

Sorry for obsessing about this, but given the 400+ stale open PRs, it
was literally impossible to tell apart this one (3623) from the rest.
And in retrospect, I do think I was right to close 3623 as well. IIRC, I
left the PRs younger than 2 weeks alone.

Now, regarding *where* the most recent version of the series lives (i.e.
what needs to be picked up from the list by the maintainer, for
merging), that beats me. We're conducting the present discussion under
the following thread:

  [edk2-devel] [PATCH 0/3] SEV-SNP accepted memory and BeforeExitBootServices
  http://mid.mail-archive.com/20221108164616.3251967-1-dionnaglaze@google.com
  https://edk2.groups.io/g/devel/message/96088
  https://listman.redhat.com/archives/edk2-devel-archive/2022-November/055386.html

so I'm tempted to think that this is the version that needs to be
merged.

However, if I check the November 2022 archive, I see v2 as well:

  [edk2-devel] [PATCH v2 0/4] SEV-SNP accepted memory and BeforeExitBootServices
  https://listman.redhat.com/archives/edk2-devel-archive/2022-November/055402.html
  https://edk2.groups.io/g/devel/message/96104

Then again, I find, in the v2 thread:

  https://listman.redhat.com/archives/edk2-devel-archive/2022-December/056363.html
  https://edk2.groups.io/g/devel/message/97065

where Dionna writes, "we decided to go ahead and consider v1 of this
series as final".

So, in effect, this patch series had reached v8 previously (in
*October*), as a part of a larger series
(<https://listman.redhat.com/archives/edk2-devel-archive/2022-October/054823.html>),
then got re-started (split-out) as a smaller series, then reached v2
like that, then v2 got abandoned in favor of v1; all without a feature
tracking BZ linking the *evolution* of the patch series across the
various postings.

And then we're surprised stuff falls through the cracks, and I remain
the OCD person that alone finds it problematic that the PR tracker and
the BZ tracker are in total shambles ¯\_(ツ)_/¯

Anyway, Dionna pinging in this particular thread (= v1 of the split-out
series) is consistent with the comment quoted from v2 qualifying v1 the
final version. As a courtesy, while my temporarily renewed subscription
to this list lasts a bit longer, I'm going to grab the v1 series in full
from the November (gzipped mbox) archive at
<https://listman.redhat.com/archives/edk2-devel-archive/>, and merge it.

... I've picked up the patches, picked up the pending feedback tags too,
and compared the result against Dionna's latest PR
<https://github.com/tianocore/edk2/pull/3885>.

Beyond the Message-Id tags that git-am picked up on my side, I've found
another difference: patch#1 was originally authored by Sophia Wolf
<phiawolf@google.com>, but that fact has been mangled in the first patch
of <https://github.com/tianocore/edk2/pull/3885> -- commit cd4c186f1099.
Namely, in commit cd4c186f1099, the original authorship is recorded as
another line in the commit message, when it should be reflected by the
Author meta-datum in git. So I'm going to stick with what I have, from
the list archive and git-am (git-am correctly translates the From: line
back to the Author meta-datum), for the new PR:

  https://github.com/tianocore/edk2/pull/3889

Laszlo