Re: [edk2-devel] [edk2] Soft Feature Freeze starts today for edk2-stable201905

["Liming Gao" <liming.gao@intel.com>]

Laszlo:
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, May 22, 2019 5:01 AM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Cetola, Stephano
> <stephano.cetola@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm <leif.lindholm@linaro.org>
> Subject: Re: [edk2-devel] [edk2] Soft Feature Freeze starts today for edk2-stable201905
> 
> Hi,
> 
> On 05/17/19 10:56, Liming Gao wrote:
> > Hi, all
> >   https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
> > lists edk2-stable201905 tag planning. Now, we enter into Soft Feature
> > Freeze phase. In this phase, the feature without Reviewed-by or
> > Acked-by tags will be delayed after the upcoming stable tag. The patch
> > review can continue without break. Below is edk2-stable201905 tag
> > planning.
> >
> > Date (00:00:00 UTC-8) Description
> > 2018-03-08 (12PM) Beginning of development
> > 2019-05-17 Soft Feature Freeze
> > 2019-05-24 Hard Feature Freeze
> > 2019-05-31 Release
> 
> We need to establish better enforcement for the soft feature freeze, or
> else, we need to redefine what the soft feature freeze means.
> 
> According to the current definition, and schedule, a patch that is not a
> bugfix may be committed after 2019-05-17 08:00 UTC only if it has
> received sufficient Acked-by / Reviewed-by tags on the list before the
> exact same timestamp (2019-05-17 08:00 UTC).
> 
>   https://github.com/lersek/edk2/wiki/SoftFeatureFreeze#what-is-the-soft-feature-freeze
> 
> > The soft feature freeze is the beginning of the stabilization phase of
> > edk2's development process. By the date of the soft feature freeze,
> > developers must have sent their patches to the mailing list *and*
> > received positive maintainer reviews (Reviewed-by or Acked-by tags).
> > This means that features, and in particular non-trivial ones, must
> > have been accepted by maintainers before the soft freeze date.
> >
> > Between the soft feature freeze and the hard feature freeze,
> > previously reviewed and unit-tested features may be applied (or
> > merged) to the master branch, for integration testing. Feature
> > addition or enablement patches posted *or* reviewed after the soft
> > feature freeze will be delayed until after the upcoming stable tag.
> 
> So let's check the recent commit history...
> 
> git log --oneline --reverse --since='2019-05-17T08:00:00+00:00'
> 
> > 8da8daafc905 ShellPkg: acpiview: Add GT Frame Number validation to GTDT parser
> 
> Reviewed on 2019-05-17, at 00:08 UTC:
> 
>   http://mid.mail-archive.com/3CE959C139B4C44DBEA1810E3AA6F9000B7D374D@SHSMSX101.ccr.corp.intel.com
> 
> Valid commit.
> 
> 
> > 1887b995a359 ShellPkg/UefiShellAcpiViewCommandLib: Fix PPTT cache attributes validation
> 
> This patch received multiple R-b's, in time, including one from a
> designated ShellPkg Reviewer:
> 
>   http://mid.mail-archive.com/DB6PR0802MB237582494362A29FEBE0DD3E84330@DB6PR0802MB2375.eurprd08.prod.outlook.com
>   http://mid.mail-archive.com/3C0D5C461C9E904E8F62152F6274C0BB40BD4110@SHSMSX104.ccr.corp.intel.com
>   http://mid.mail-archive.com/3CE959C139B4C44DBEA1810E3AA6F9000B7D3832@SHSMSX101.ccr.corp.intel.com
> 
> Valid commit. (This patch is a bugfix, even.)
> 
> 
> > 41ac2076a7c6 UefiCpuPkg CpuCommonFeaturesLib: Remove CPU generation check
> 
> This is an invalid commit during the soft feature freeze. The change is
> *not* a bugfix (it is a change that helps with the future use of a
> function). And sufficient review was given only *after* the deadline, at
> 13:10 UTC:
> 
>   http://mid.mail-archive.com/734D49CCEBEEF84792F5B80ED585239D5C1476E8@SHSMSX104.ccr.corp.intel.com
> 

I don't see this is in edk2 feature planning. We may need to give some guideline to define what change is a feature or a bug.

> 
> > 59f20e8d7100 ShellPkg: Update DSC to use NetworkPkg's include fragment file
> 
> Invalid commit. This is not a bugfix but a feature patch. (The commit
> message should have referenced
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1293>.) The one R-b that
> it received was posted *way* after the deadline, on 2019-05-20, at 00:28
> UTC:
> 
>   http://mid.mail-archive.com/3CE959C139B4C44DBEA1810E3AA6F9000B7D3DB1@SHSMSX101.ccr.corp.intel.com
> 
> 
> > 48f43c2c56ee EmbeddedPkg: Update DSC to use NetworkPkg's include fragment file
> 
> Invalid commit. This is a feature patch (and it should have referenced
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1293>). The Reviewed-by
> was posted after the deadline, at 09:00 UTC.
> 
>   http://mid.mail-archive.com/20190517090006.ko36fbne4vprai3w@bivouac.eciton.net
> 
The changes in ShellPkg and EmbeddedPkg refers to NetworkPkg's include fragment file.
NetworkPkg's include fragment file have been added. Package DSC should use them.
So, I regard those missing change as the bug fix. 

> 
> > 7b84de939489 ShellPkg: Display VENDOR_ID in ASCII when parsing PPTT
> 
> Valid commit; the sufficient R-b was given in time:
> 
>   http://mid.mail-archive.com/3CE959C139B4C44DBEA1810E3AA6F9000B7D21C2@SHSMSX101.ccr.corp.intel.com
> 
> (This patch could also be considered a bugfix, I believe.)
> 
> 
> > 911efe279ec3 ShellPkg: Add NetworkPkg/NetworkPkg.dec as the package dependency
> 
> This is an invalid commit, during the soft feature freeze. Not only was
> the reviewer feedback posted after the soft feature freeze, the patch
> *itself* was posted after the same, on 2019-05-20, at 13:09.
> 
>   http://mid.mail-archive.com/20190520130920.9464-2-liming.gao@intel.com
> 
> And, this is not a bugfix but a feature enablement patch -- the commit
> message says, "NetLib *will be moved* from MdeModulePkg and NetworkPkg"
> (emphasis mine).
> 
> The commit message also doesn't reference
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1293>.
> 
> 
> > 110d4729b58e EmulatorPkg: Add NetworkPkg/NetworkPkg.dec as the package dependency
> 
> Invalid, same as commit 911efe279ec3 above.
> 
Network module movement patch have been reviewed. https://edk2.groups.io/g/devel/message/40840?p=,,,20,0,0,0::Created,,siyuan,20,2,0,31628785
When I plan to push it, I find the above issues. Because the feature to move network modules from MdeModulePkg to NetworkPkg have been reviewed, 
I regard them also bugs. 

Here, I don't want to argue whether they are feature or bug. I just want to share my thinking, and collect feedback, then work out 
the clear rule so that all developers can follow.

> ... No, wait, this is worse: this patch had not received *any* review
> feedback on the list:
> 
>   http://mid.mail-archive.com/20190520130920.9464-3-liming.gao@intel.com
> 
> but it was committed with Ray's R-b. I don't understand.
> 
I see Ray has replied the mail. 

> 
> > cc99ea9422be Maintainers.txt: remove UTF-8 BOM wrongly added in commit 147e6e70
> 
> Valid commit: obvious bugfix.
> 
> 
> > 66b845ae06f1 BaseTools: Fix private includes for FILE_GUID override
> 
> Seems like a valid commit: a bugfix.
> 
> There is one wart though -- the commit includes Bob's R-b, but that R-b
> had never been posted to the list:
> 
>   http://mid.mail-archive.com/20190516111728.33584-1-bob.c.feng@intel.com
> 
> 
> > a7ef158b0752 BaseTools: Library hashing fix and optimization for --hash feature
> 
> Looks like a bugfix to me, for a previously added feature (--hash),
> hence arguably a valid commit.
> 
> 
> So that's 5 invalid commits out of 11 commits, during the soft feature
> freeze thus far. It's obvious that the current soft feature freeze
> criteria are not being honored. So, again, either we need to enforce
> them with tools better than just self-discipline, or we must change the
> SFF criteria.

4 commits are all from mine. So, most people follows the rule. This is not bad. 

> 
> Thanks
> Laszlo