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

["Dionna Glaze" <dionnaglaze@google.com>]

Thanks for your explanation and fastidiousness, Laszlo. Much appreciated.

On Thu, Jan 12, 2023 at 7:32 AM Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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
>


-- 
-Dionna Glaze, PhD (she/her)