From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.58051.1673537538094377009 for ; Thu, 12 Jan 2023 07:32:18 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=bBK2qt4s; spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673537537; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YDyKb02l+tQMxHEDvjipUNgP4IFRoUHb63VzEs7Ya24=; b=bBK2qt4sRSV8XUTITAY/CzQF8MkoyMMzBhsju7Hn88PZ5CIq9D1i9I6AtuvVb51obPFzN2 AswBinfEIYhx9AF4qPE4fG8/U8v3BtNKjAeT+bcCvJf/TdiLtaMTLgusjbeSZSaiekGh5J /ryhJg+6RsSPG3YGJ5A34xhc0PyLJ3c= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-533-JoD2M4sbML2-kCVwbQrfmw-1; Thu, 12 Jan 2023 10:32:11 -0500 X-MC-Unique: JoD2M4sbML2-kCVwbQrfmw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 98A04811E9C; Thu, 12 Jan 2023 15:32:10 +0000 (UTC) Received: from [10.39.192.93] (unknown [10.39.192.93]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1EF181121314; Thu, 12 Jan 2023 15:32:07 +0000 (UTC) Message-ID: <79fc5c62-922c-0db2-2c12-8557d276cce3@redhat.com> Date: Thu, 12 Jan 2023 16:32:06 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 3/3] MdeModulePkg: Notify BeforeExitBootServices in CoreExitBootServices To: Ard Biesheuvel , devel@edk2.groups.io Cc: dionnaglaze@google.com, Gerd Hoffmann , James Bottomley , "Yao, Jiewen" , Tom Lendacky , "Xu, Min M" , Andrew Fish , "Ni, Ray" , Moritz Fischer , "Kinney, Michael D" References: <20221108164616.3251967-1-dionnaglaze@google.com> <20221108164616.3251967-4-dionnaglaze@google.com> <75ed5959-fbb2-3343-e7d7-dfa17b4a2615@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 1/12/23 14:38, Ard Biesheuvel wrote: > On Thu, 12 Jan 2023 at 13:24, Laszlo Ersek 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 >>> 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 >> .) >> >> 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 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 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 (), 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 , 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 . 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 , but that fact has been mangled in the first patch of -- 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