Patch List for 201911 stable tag

Patch List for 201911 stable tag

[Liming Gao]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

Hi Stewards and all:
  I collect current patch lists in devel mail list. Those patch contributors request to add them for 201911 stable tag. Because the time is close to Hard Feature Freeze, I want to collect your feedback for below patches.

Feature List (those all have pass code review):
https://edk2.groups.io/g/devel/message/50602 [PATCH V2] BaseTools: Add [packages] section in dsc file

Bug List (those all have pass code review):
https://edk2.groups.io/g/devel/message/50625 [PATCH v1] MdeModulePkg/NvmExpressDxe: Fix wrong queue size for async IO queues
https://edk2.groups.io/g/devel/message/50406 [PATCH 1/1] MdePkg/Include: Add missing definitions of SMBIOS type 42h in SmBios.h
https://edk2.groups.io/g/devel/message/50661 [PATCH] UefiCpuPkg: Update the coding styles
https://edk2.groups.io/g/devel/message/50662 [PATCH] MdePkg: Update the comments of IsLanguageSupported
https://edk2.groups.io/g/devel/message/50663 [PATCH 0/3] Add missing strings for uni files
https://edk2.groups.io/g/devel/message/50866 [PATCH V1 0/2] Improve PeiInstallPeiMemory() description
https://edk2.groups.io/g/devel/message/50841 [PATCH V2 1/1] MdeModulePkg PeiCore: Fix typos

Thanks
Liming

_._,_._,_

[-- Attachment #2: Type: text/html, Size: 5360 bytes --]

Re: Patch List for 201911 stable tag

[Laszlo Ersek]

On 11/19/19 15:25, Gao, Liming wrote:
> Hi Stewards and all:
>   I collect current patch lists in devel mail list. Those patch
>   contributors request to add them for 201911 stable tag. Because the
>   time is close to Hard Feature Freeze, I want to collect your
>   feedback for below patches.
>
> Feature List (those all have pass code review):
> https://edk2.groups.io/g/devel/message/50602 [PATCH V2] BaseTools: Add [packages] section in dsc file

This patch can be merged during the Soft Feature Freeze. It was posted
before the Soft Feature Freeze, and also reviewed (by Bob, i.e. a
BaseTools Maintainer) before the Soft Feature Freeze.

As far as I can see, there is still an outstanding question from you, to
Zhiju ("Can you show what test are done for this new support?"), so I
think we should await the response to that.

Note that the patch should not be merged once the Hard Feature Freeze
starts, so there are ~3 days for Zhiju to answer the question about
testing (and for you to acknowledge that you are OK with the reply).

> Bug List (those all have pass code review):
> https://edk2.groups.io/g/devel/message/50625 [PATCH v1] MdeModulePkg/NvmExpressDxe: Fix wrong queue size for async IO queues

Looks very much like a bugfix to me, so it's suitable for merging even
during the Hard Feature Freeze.

> https://edk2.groups.io/g/devel/message/50406 [PATCH 1/1] MdePkg/Include: Add missing definitions of SMBIOS type 42h in SmBios.h

Based on Abner's response in the thread, this change does not appear
necessary for fixing actual functionality bugs; it rather completes a
previously incomplete feature addition. And Abner is not in a rush to
catch the upcoming stable tag with the patch. I suggest to delay it.

If others disagree, I won't insist; the above is just my preference.

> https://edk2.groups.io/g/devel/message/50661 [PATCH] UefiCpuPkg: Update the coding styles

Hmmm, quite undecided on this one. Does not fix a functionality bug
either, but what it fixes *are* a coding style bugs, and the patch is
low risk. I'm leaning towards merging it.

> https://edk2.groups.io/g/devel/message/50662 [PATCH] MdePkg: Update the comments of IsLanguageSupported

This was even reviewed by a package maintainer (= you) before the SFF,
so it can definitely go in.

> https://edk2.groups.io/g/devel/message/50663 [PATCH 0/3] Add missing strings for uni files

First of all, the structure of this series is wrong; please see my
feedback here:

  https://edk2.groups.io/g/devel/message/50666

(The two patches discussed just above were incorrectly included in the
same posting.)

Second, the three patches for the UNI files add too much brand new text
for my taste, for them to be considered bugfixes. The patches were
posted in time for the SFF, but the maintainer reviews came too late:

  https://edk2.groups.io/g/devel/message/50872
  https://edk2.groups.io/g/devel/message/50869
  https://edk2.groups.io/g/devel/message/50870

I suggest postponing.

> https://edk2.groups.io/g/devel/message/50866 [PATCH V1 0/2] Improve PeiInstallPeiMemory() description

I'm seriously confused by the subject prefixes in this patch thread.
What's going on with the version numbers?

  [edk2-devel] [PATCH V1 0/2] Improve PeiInstallPeiMemory() description
  [edk2-devel] [PATCH V3 1/2] MdeModulePkg PeiCore: Improve PeiInstallPeiMemory() description
  [edk2-devel] [PATCH V1 2/2] MdePkg PiPeiCis.h: Improve PeiInstallPeiMemory() description

Other than that... I'm torn. I guess I could be convinced that these
patches are indeed bugfixes, so I'm leaning towards merging them.

> https://edk2.groups.io/g/devel/message/50841 [PATCH V2 1/1] MdeModulePkg PeiCore: Fix typos

Personally I'm not happy about this patch. It's way too large for my taste:

 MdeModulePkg/Core/Pei/PeiMain.inf             | 10 ++--
 MdeModulePkg/Core/Pei/FwVol/FwVol.h           | 20 +++----
 MdeModulePkg/Core/Pei/PeiMain.h               | 52 ++++++++--------
 MdeModulePkg/Core/Pei/Dependency/Dependency.c | 12 ++--
 MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 51 ++++++++--------
 MdeModulePkg/Core/Pei/FwVol/FwVol.c           | 63 ++++++++++----------
 MdeModulePkg/Core/Pei/Hob/Hob.c               |  4 +-
 MdeModulePkg/Core/Pei/Image/Image.c           | 10 ++--
 MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 18 +++---
 MdeModulePkg/Core/Pei/PeiMain/PeiMain.c       |  2 +-
 MdeModulePkg/Core/Pei/Ppi/Ppi.c               |  4 +-
 MdeModulePkg/Core/Pei/Security/Security.c     | 12 ++--
 12 files changed, 129 insertions(+), 129 deletions(-)

and it mixes multiple kinds of changes:

"Fixes typos and clarifies some wording throughout PeiCore."

When reviewing such a patch, the reviewer has a difficult time telling
apart purely syntactic (typo) fixes from semantic (wording) fixes. As a
reviewer I would suggest splitting this patch at least in two (typos vs.
semantics). Then I could be convinced such a set of two patches is
purely a bugfix.

I'm leaning towards "postpone" on this one, but I can see why people
would think "that's arbitrary". I guess I'll have to defer to others in
this instance.

Thanks
Laszlo

Re: Patch List for 201911 stable tag

[Leif Lindholm]

On Tue, Nov 19, 2019 at 06:50:19PM +0100, Laszlo Ersek wrote:
> On 11/19/19 15:25, Gao, Liming wrote:
> > Hi Stewards and all:
> >   I collect current patch lists in devel mail list. Those patch
> >   contributors request to add them for 201911 stable tag. Because the
> >   time is close to Hard Feature Freeze, I want to collect your
> >   feedback for below patches.
> >
> > Feature List (those all have pass code review):
> > https://edk2.groups.io/g/devel/message/50602 [PATCH V2] BaseTools: Add [packages] section in dsc file
> 
> This patch can be merged during the Soft Feature Freeze. It was posted
> before the Soft Feature Freeze, and also reviewed (by Bob, i.e. a
> BaseTools Maintainer) before the Soft Feature Freeze.
> 
> As far as I can see, there is still an outstanding question from you, to
> Zhiju ("Can you show what test are done for this new support?"), so I
> think we should await the response to that.
> 
> Note that the patch should not be merged once the Hard Feature Freeze
> starts, so there are ~3 days for Zhiju to answer the question about
> testing (and for you to acknowledge that you are OK with the reply).

Agreed.

> > Bug List (those all have pass code review):
> > https://edk2.groups.io/g/devel/message/50625 [PATCH v1] MdeModulePkg/NvmExpressDxe: Fix wrong queue size for async IO queues
> 
> Looks very much like a bugfix to me, so it's suitable for merging even
> during the Hard Feature Freeze.

I agree. But I am still slightly nervous about changing such a
fundamental part of such a fundamental driver. Certainly if it is
going in, I want it in ASAP, not just at the end of soft freeze - to
give us as much time as possible to revert it if the fix exposes
latent errors in previously working systems.

> > https://edk2.groups.io/g/devel/message/50406 [PATCH 1/1] MdePkg/Include: Add missing definitions of SMBIOS type 42h in SmBios.h
> 
> Based on Abner's response in the thread, this change does not appear
> necessary for fixing actual functionality bugs; it rather completes a
> previously incomplete feature addition. And Abner is not in a rush to
> catch the upcoming stable tag with the patch. I suggest to delay it.
> 
> If others disagree, I won't insist; the above is just my preference.

I'm OK either way.

> > https://edk2.groups.io/g/devel/message/50661 [PATCH] UefiCpuPkg: Update the coding styles
> 
> Hmmm, quite undecided on this one. Does not fix a functionality bug
> either, but what it fixes *are* a coding style bugs, and the patch is
> low risk. I'm leaning towards merging it.

I am against merging this, even though it's low-risk.

The process says:
"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 received Acks 4 days late.

If it came with a commit message indicating the incorrect comment
syntax caused problems with document generation, then maybe it could
be considered from a bugfix standpoint. But it didn't and it's too
late to re-scope the change at this point.

I also dislike the mixing of doxygen formating changes and plain
whitespace changes. Even though trivial, it ought to be split up.

> > https://edk2.groups.io/g/devel/message/50662 [PATCH] MdePkg: Update the comments of IsLanguageSupported
> 
> This was even reviewed by a package maintainer (= you) before the SFF,
> so it can definitely go in.

Agree (if cutting it close).

> > https://edk2.groups.io/g/devel/message/50663 [PATCH 0/3] Add missing strings for uni files
> 
> First of all, the structure of this series is wrong; please see my
> feedback here:
> 
>   https://edk2.groups.io/g/devel/message/50666
> 
> (The two patches discussed just above were incorrectly included in the
> same posting.)
> 
> Second, the three patches for the UNI files add too much brand new text
> for my taste, for them to be considered bugfixes. The patches were
> posted in time for the SFF, but the maintainer reviews came too late:
> 
>   https://edk2.groups.io/g/devel/message/50872
>   https://edk2.groups.io/g/devel/message/50869
>   https://edk2.groups.io/g/devel/message/50870
> 
> I suggest postponing.

Agree.

> > https://edk2.groups.io/g/devel/message/50866 [PATCH V1 0/2] Improve PeiInstallPeiMemory() description
> 
> I'm seriously confused by the subject prefixes in this patch thread.
> What's going on with the version numbers?
> 
>   [edk2-devel] [PATCH V1 0/2] Improve PeiInstallPeiMemory() description
>   [edk2-devel] [PATCH V3 1/2] MdeModulePkg PeiCore: Improve PeiInstallPeiMemory() description
>   [edk2-devel] [PATCH V1 2/2] MdePkg PiPeiCis.h: Improve PeiInstallPeiMemory() description
> 
> Other than that... I'm torn. I guess I could be convinced that these
> patches are indeed bugfixes, so I'm leaning towards merging them.

Non-functional change submitted after start of soft-freeze?
I don't see why it should be considered.

> > https://edk2.groups.io/g/devel/message/50841 [PATCH V2 1/1] MdeModulePkg PeiCore: Fix typos
> 
> Personally I'm not happy about this patch. It's way too large for my taste:
> 
>  MdeModulePkg/Core/Pei/PeiMain.inf             | 10 ++--
>  MdeModulePkg/Core/Pei/FwVol/FwVol.h           | 20 +++----
>  MdeModulePkg/Core/Pei/PeiMain.h               | 52 ++++++++--------
>  MdeModulePkg/Core/Pei/Dependency/Dependency.c | 12 ++--
>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 51 ++++++++--------
>  MdeModulePkg/Core/Pei/FwVol/FwVol.c           | 63 ++++++++++----------
>  MdeModulePkg/Core/Pei/Hob/Hob.c               |  4 +-
>  MdeModulePkg/Core/Pei/Image/Image.c           | 10 ++--
>  MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 18 +++---
>  MdeModulePkg/Core/Pei/PeiMain/PeiMain.c       |  2 +-
>  MdeModulePkg/Core/Pei/Ppi/Ppi.c               |  4 +-
>  MdeModulePkg/Core/Pei/Security/Security.c     | 12 ++--
>  12 files changed, 129 insertions(+), 129 deletions(-)
> 
> and it mixes multiple kinds of changes:
> 
> "Fixes typos and clarifies some wording throughout PeiCore."
> 
> When reviewing such a patch, the reviewer has a difficult time telling
> apart purely syntactic (typo) fixes from semantic (wording) fixes. As a
> reviewer I would suggest splitting this patch at least in two (typos vs.
> semantics). Then I could be convinced such a set of two patches is
> purely a bugfix.
> 
> I'm leaning towards "postpone" on this one, but I can see why people
> would think "that's arbitrary". I guess I'll have to defer to others in
> this instance.

Non-functional change submitted after start of soft-freeze?
I don't see why it should be considered.

I also agree on the needs splitting up bit.

Best Regards,

Leif

Re: [edk2-devel] Patch List for 201911 stable tag

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Leif
> Lindholm
> Sent: Wednesday, November 20, 2019 3:02 AM
> To: Laszlo Ersek
> Cc: Gao, Liming; Kinney, Michael D; 'afish@apple.com'; devel@edk2.groups.io
> Subject: Re: [edk2-devel] Patch List for 201911 stable tag
> 
> On Tue, Nov 19, 2019 at 06:50:19PM +0100, Laszlo Ersek wrote:
> > On 11/19/19 15:25, Gao, Liming wrote:
> > > Hi Stewards and all:
> > >   I collect current patch lists in devel mail list. Those patch
> > >   contributors request to add them for 201911 stable tag. Because the
> > >   time is close to Hard Feature Freeze, I want to collect your
> > >   feedback for below patches.
> > >
> > > Feature List (those all have pass code review):
> > > https://edk2.groups.io/g/devel/message/50602 [PATCH V2] BaseTools: Add
> [packages] section in dsc file
> >
> > This patch can be merged during the Soft Feature Freeze. It was posted
> > before the Soft Feature Freeze, and also reviewed (by Bob, i.e. a
> > BaseTools Maintainer) before the Soft Feature Freeze.
> >
> > As far as I can see, there is still an outstanding question from you, to
> > Zhiju ("Can you show what test are done for this new support?"), so I
> > think we should await the response to that.
> >
> > Note that the patch should not be merged once the Hard Feature Freeze
> > starts, so there are ~3 days for Zhiju to answer the question about
> > testing (and for you to acknowledge that you are OK with the reply).
> 
> Agreed.
> 
> > > Bug List (those all have pass code review):
> > > https://edk2.groups.io/g/devel/message/50625 [PATCH v1]
> MdeModulePkg/NvmExpressDxe: Fix wrong queue size for async IO queues
> >
> > Looks very much like a bugfix to me, so it's suitable for merging even
> > during the Hard Feature Freeze.
> 
> I agree. But I am still slightly nervous about changing such a
> fundamental part of such a fundamental driver. Certainly if it is
> going in, I want it in ASAP, not just at the end of soft freeze - to
> give us as much time as possible to revert it if the fix exposes
> latent errors in previously working systems.


Thanks Leif,

I will create a pull request to push this bugfix ASAP.

Best Regards,
Hao Wu


> 
> > > https://edk2.groups.io/g/devel/message/50406 [PATCH 1/1]
> MdePkg/Include: Add missing definitions of SMBIOS type 42h in SmBios.h
> >
> > Based on Abner's response in the thread, this change does not appear
> > necessary for fixing actual functionality bugs; it rather completes a
> > previously incomplete feature addition. And Abner is not in a rush to
> > catch the upcoming stable tag with the patch. I suggest to delay it.
> >
> > If others disagree, I won't insist; the above is just my preference.
> 
> I'm OK either way.
> 
> > > https://edk2.groups.io/g/devel/message/50661 [PATCH] UefiCpuPkg:
> Update the coding styles
> >
> > Hmmm, quite undecided on this one. Does not fix a functionality bug
> > either, but what it fixes *are* a coding style bugs, and the patch is
> > low risk. I'm leaning towards merging it.
> 
> I am against merging this, even though it's low-risk.
> 
> The process says:
> "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 received Acks 4 days late.
> 
> If it came with a commit message indicating the incorrect comment
> syntax caused problems with document generation, then maybe it could
> be considered from a bugfix standpoint. But it didn't and it's too
> late to re-scope the change at this point.
> 
> I also dislike the mixing of doxygen formating changes and plain
> whitespace changes. Even though trivial, it ought to be split up.
> 
> > > https://edk2.groups.io/g/devel/message/50662 [PATCH] MdePkg: Update
> the comments of IsLanguageSupported
> >
> > This was even reviewed by a package maintainer (= you) before the SFF,
> > so it can definitely go in.
> 
> Agree (if cutting it close).
> 
> > > https://edk2.groups.io/g/devel/message/50663 [PATCH 0/3] Add missing
> strings for uni files
> >
> > First of all, the structure of this series is wrong; please see my
> > feedback here:
> >
> >   https://edk2.groups.io/g/devel/message/50666
> >
> > (The two patches discussed just above were incorrectly included in the
> > same posting.)
> >
> > Second, the three patches for the UNI files add too much brand new text
> > for my taste, for them to be considered bugfixes. The patches were
> > posted in time for the SFF, but the maintainer reviews came too late:
> >
> >   https://edk2.groups.io/g/devel/message/50872
> >   https://edk2.groups.io/g/devel/message/50869
> >   https://edk2.groups.io/g/devel/message/50870
> >
> > I suggest postponing.
> 
> Agree.
> 
> > > https://edk2.groups.io/g/devel/message/50866 [PATCH V1 0/2] Improve
> PeiInstallPeiMemory() description
> >
> > I'm seriously confused by the subject prefixes in this patch thread.
> > What's going on with the version numbers?
> >
> >   [edk2-devel] [PATCH V1 0/2] Improve PeiInstallPeiMemory() description
> >   [edk2-devel] [PATCH V3 1/2] MdeModulePkg PeiCore: Improve
> PeiInstallPeiMemory() description
> >   [edk2-devel] [PATCH V1 2/2] MdePkg PiPeiCis.h: Improve
> PeiInstallPeiMemory() description
> >
> > Other than that... I'm torn. I guess I could be convinced that these
> > patches are indeed bugfixes, so I'm leaning towards merging them.
> 
> Non-functional change submitted after start of soft-freeze?
> I don't see why it should be considered.
> 
> > > https://edk2.groups.io/g/devel/message/50841 [PATCH V2 1/1]
> MdeModulePkg PeiCore: Fix typos
> >
> > Personally I'm not happy about this patch. It's way too large for my taste:
> >
> >  MdeModulePkg/Core/Pei/PeiMain.inf             | 10 ++--
> >  MdeModulePkg/Core/Pei/FwVol/FwVol.h           | 20 +++----
> >  MdeModulePkg/Core/Pei/PeiMain.h               | 52 ++++++++--------
> >  MdeModulePkg/Core/Pei/Dependency/Dependency.c | 12 ++--
> >  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 51 ++++++++--------
> >  MdeModulePkg/Core/Pei/FwVol/FwVol.c           | 63 ++++++++++----------
> >  MdeModulePkg/Core/Pei/Hob/Hob.c               |  4 +-
> >  MdeModulePkg/Core/Pei/Image/Image.c           | 10 ++--
> >  MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 18 +++---
> >  MdeModulePkg/Core/Pei/PeiMain/PeiMain.c       |  2 +-
> >  MdeModulePkg/Core/Pei/Ppi/Ppi.c               |  4 +-
> >  MdeModulePkg/Core/Pei/Security/Security.c     | 12 ++--
> >  12 files changed, 129 insertions(+), 129 deletions(-)
> >
> > and it mixes multiple kinds of changes:
> >
> > "Fixes typos and clarifies some wording throughout PeiCore."
> >
> > When reviewing such a patch, the reviewer has a difficult time telling
> > apart purely syntactic (typo) fixes from semantic (wording) fixes. As a
> > reviewer I would suggest splitting this patch at least in two (typos vs.
> > semantics). Then I could be convinced such a set of two patches is
> > purely a bugfix.
> >
> > I'm leaning towards "postpone" on this one, but I can see why people
> > would think "that's arbitrary". I guess I'll have to defer to others in
> > this instance.
> 
> Non-functional change submitted after start of soft-freeze?
> I don't see why it should be considered.
> 
> I also agree on the needs splitting up bit.
> 
> Best Regards,
> 
> Leif
> 
> 

Re: [edk2-devel] Patch List for 201911 stable tag

[Liming Gao]

Laszlo and Leif:
  Thanks for your detail review. I will continue to monitor the coming changes for 201911 stable tag. 

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif Lindholm
> Sent: Wednesday, November 20, 2019 3:02 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; 'afish@apple.com' <afish@apple.com>;
> devel@edk2.groups.io
> Subject: Re: [edk2-devel] Patch List for 201911 stable tag
> 
> On Tue, Nov 19, 2019 at 06:50:19PM +0100, Laszlo Ersek wrote:
> > On 11/19/19 15:25, Gao, Liming wrote:
> > > Hi Stewards and all:
> > >   I collect current patch lists in devel mail list. Those patch
> > >   contributors request to add them for 201911 stable tag. Because the
> > >   time is close to Hard Feature Freeze, I want to collect your
> > >   feedback for below patches.
> > >
> > > Feature List (those all have pass code review):
> > > https://edk2.groups.io/g/devel/message/50602 [PATCH V2] BaseTools: Add [packages] section in dsc file
> >
> > This patch can be merged during the Soft Feature Freeze. It was posted
> > before the Soft Feature Freeze, and also reviewed (by Bob, i.e. a
> > BaseTools Maintainer) before the Soft Feature Freeze.
> >
> > As far as I can see, there is still an outstanding question from you, to
> > Zhiju ("Can you show what test are done for this new support?"), so I
> > think we should await the response to that.
> >
> > Note that the patch should not be merged once the Hard Feature Freeze
> > starts, so there are ~3 days for Zhiju to answer the question about
> > testing (and for you to acknowledge that you are OK with the reply).
> 
> Agreed.
> 
> > > Bug List (those all have pass code review):
> > > https://edk2.groups.io/g/devel/message/50625 [PATCH v1] MdeModulePkg/NvmExpressDxe: Fix wrong queue size for async IO
> queues
> >
> > Looks very much like a bugfix to me, so it's suitable for merging even
> > during the Hard Feature Freeze.
> 
> I agree. But I am still slightly nervous about changing such a
> fundamental part of such a fundamental driver. Certainly if it is
> going in, I want it in ASAP, not just at the end of soft freeze - to
> give us as much time as possible to revert it if the fix exposes
> latent errors in previously working systems.
> 
> > > https://edk2.groups.io/g/devel/message/50406 [PATCH 1/1] MdePkg/Include: Add missing definitions of SMBIOS type 42h in
> SmBios.h
> >
> > Based on Abner's response in the thread, this change does not appear
> > necessary for fixing actual functionality bugs; it rather completes a
> > previously incomplete feature addition. And Abner is not in a rush to
> > catch the upcoming stable tag with the patch. I suggest to delay it.
> >
> > If others disagree, I won't insist; the above is just my preference.
> 
> I'm OK either way.
> 
> > > https://edk2.groups.io/g/devel/message/50661 [PATCH] UefiCpuPkg: Update the coding styles
> >
> > Hmmm, quite undecided on this one. Does not fix a functionality bug
> > either, but what it fixes *are* a coding style bugs, and the patch is
> > low risk. I'm leaning towards merging it.
> 
> I am against merging this, even though it's low-risk.
> 
> The process says:
> "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 received Acks 4 days late.
> 
> If it came with a commit message indicating the incorrect comment
> syntax caused problems with document generation, then maybe it could
> be considered from a bugfix standpoint. But it didn't and it's too
> late to re-scope the change at this point.
> 
> I also dislike the mixing of doxygen formating changes and plain
> whitespace changes. Even though trivial, it ought to be split up.
> 
> > > https://edk2.groups.io/g/devel/message/50662 [PATCH] MdePkg: Update the comments of IsLanguageSupported
> >
> > This was even reviewed by a package maintainer (= you) before the SFF,
> > so it can definitely go in.
> 
> Agree (if cutting it close).
> 
> > > https://edk2.groups.io/g/devel/message/50663 [PATCH 0/3] Add missing strings for uni files
> >
> > First of all, the structure of this series is wrong; please see my
> > feedback here:
> >
> >   https://edk2.groups.io/g/devel/message/50666
> >
> > (The two patches discussed just above were incorrectly included in the
> > same posting.)
> >
> > Second, the three patches for the UNI files add too much brand new text
> > for my taste, for them to be considered bugfixes. The patches were
> > posted in time for the SFF, but the maintainer reviews came too late:
> >
> >   https://edk2.groups.io/g/devel/message/50872
> >   https://edk2.groups.io/g/devel/message/50869
> >   https://edk2.groups.io/g/devel/message/50870
> >
> > I suggest postponing.
> 
> Agree.
> 
> > > https://edk2.groups.io/g/devel/message/50866 [PATCH V1 0/2] Improve PeiInstallPeiMemory() description
> >
> > I'm seriously confused by the subject prefixes in this patch thread.
> > What's going on with the version numbers?
> >
> >   [edk2-devel] [PATCH V1 0/2] Improve PeiInstallPeiMemory() description
> >   [edk2-devel] [PATCH V3 1/2] MdeModulePkg PeiCore: Improve PeiInstallPeiMemory() description
> >   [edk2-devel] [PATCH V1 2/2] MdePkg PiPeiCis.h: Improve PeiInstallPeiMemory() description
> >
> > Other than that... I'm torn. I guess I could be convinced that these
> > patches are indeed bugfixes, so I'm leaning towards merging them.
> 
> Non-functional change submitted after start of soft-freeze?
> I don't see why it should be considered.
> 
> > > https://edk2.groups.io/g/devel/message/50841 [PATCH V2 1/1] MdeModulePkg PeiCore: Fix typos
> >
> > Personally I'm not happy about this patch. It's way too large for my taste:
> >
> >  MdeModulePkg/Core/Pei/PeiMain.inf             | 10 ++--
> >  MdeModulePkg/Core/Pei/FwVol/FwVol.h           | 20 +++----
> >  MdeModulePkg/Core/Pei/PeiMain.h               | 52 ++++++++--------
> >  MdeModulePkg/Core/Pei/Dependency/Dependency.c | 12 ++--
> >  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 51 ++++++++--------
> >  MdeModulePkg/Core/Pei/FwVol/FwVol.c           | 63 ++++++++++----------
> >  MdeModulePkg/Core/Pei/Hob/Hob.c               |  4 +-
> >  MdeModulePkg/Core/Pei/Image/Image.c           | 10 ++--
> >  MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 18 +++---
> >  MdeModulePkg/Core/Pei/PeiMain/PeiMain.c       |  2 +-
> >  MdeModulePkg/Core/Pei/Ppi/Ppi.c               |  4 +-
> >  MdeModulePkg/Core/Pei/Security/Security.c     | 12 ++--
> >  12 files changed, 129 insertions(+), 129 deletions(-)
> >
> > and it mixes multiple kinds of changes:
> >
> > "Fixes typos and clarifies some wording throughout PeiCore."
> >
> > When reviewing such a patch, the reviewer has a difficult time telling
> > apart purely syntactic (typo) fixes from semantic (wording) fixes. As a
> > reviewer I would suggest splitting this patch at least in two (typos vs.
> > semantics). Then I could be convinced such a set of two patches is
> > purely a bugfix.
> >
> > I'm leaning towards "postpone" on this one, but I can see why people
> > would think "that's arbitrary". I guess I'll have to defer to others in
> > this instance.
> 
> Non-functional change submitted after start of soft-freeze?
> I don't see why it should be considered.
> 
> I also agree on the needs splitting up bit.
> 
> Best Regards,
> 
> Leif
> 
> 

Re: [edk2-devel] Patch List for 201911 stable tag

[Liming Gao]

Hi Stewards and all:
  New bugs are for 201911 stable tag. Can you give the comments for them?

Bug List (those all have pass code review):
https://edk2.groups.io/g/devel/message/50931 [PATCH] MdeModulePkg: LzmaCustomDecompressLib.inf don't support EBC anymore
https://edk2.groups.io/g/devel/message/50990 [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable
https://edk2.groups.io/g/devel/message/50989 [PATCH V2] BaseTools:fix regression issue for platform .map file
https://edk2.groups.io/g/devel/message/50936 [PATCH] BaseTools:fixed Build failed issue for Non-English OS

Thanks
Liming
>-----Original Message-----
>From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>Liming Gao
>Sent: Wednesday, November 20, 2019 10:52 PM
>To: devel@edk2.groups.io; leif.lindholm@linaro.org; Laszlo Ersek
><lersek@redhat.com>
>Cc: Kinney, Michael D <michael.d.kinney@intel.com>; 'afish@apple.com'
><afish@apple.com>
>Subject: Re: [edk2-devel] Patch List for 201911 stable tag
>
>Laszlo and Leif:
>  Thanks for your detail review. I will continue to monitor the coming changes
>for 201911 stable tag.
>
>Thanks
>Liming
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif
>Lindholm
>> Sent: Wednesday, November 20, 2019 3:02 AM
>> To: Laszlo Ersek <lersek@redhat.com>
>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
><michael.d.kinney@intel.com>; 'afish@apple.com' <afish@apple.com>;
>> devel@edk2.groups.io
>> Subject: Re: [edk2-devel] Patch List for 201911 stable tag
>>
>> On Tue, Nov 19, 2019 at 06:50:19PM +0100, Laszlo Ersek wrote:
>> > On 11/19/19 15:25, Gao, Liming wrote:
>> > > Hi Stewards and all:
>> > >   I collect current patch lists in devel mail list. Those patch
>> > >   contributors request to add them for 201911 stable tag. Because the
>> > >   time is close to Hard Feature Freeze, I want to collect your
>> > >   feedback for below patches.
>> > >
>> > > Feature List (those all have pass code review):
>> > > https://edk2.groups.io/g/devel/message/50602 [PATCH V2] BaseTools:
>Add [packages] section in dsc file
>> >
>> > This patch can be merged during the Soft Feature Freeze. It was posted
>> > before the Soft Feature Freeze, and also reviewed (by Bob, i.e. a
>> > BaseTools Maintainer) before the Soft Feature Freeze.
>> >
>> > As far as I can see, there is still an outstanding question from you, to
>> > Zhiju ("Can you show what test are done for this new support?"), so I
>> > think we should await the response to that.
>> >
>> > Note that the patch should not be merged once the Hard Feature Freeze
>> > starts, so there are ~3 days for Zhiju to answer the question about
>> > testing (and for you to acknowledge that you are OK with the reply).
>>
>> Agreed.
>>
>> > > Bug List (those all have pass code review):
>> > > https://edk2.groups.io/g/devel/message/50625 [PATCH v1]
>MdeModulePkg/NvmExpressDxe: Fix wrong queue size for async IO
>> queues
>> >
>> > Looks very much like a bugfix to me, so it's suitable for merging even
>> > during the Hard Feature Freeze.
>>
>> I agree. But I am still slightly nervous about changing such a
>> fundamental part of such a fundamental driver. Certainly if it is
>> going in, I want it in ASAP, not just at the end of soft freeze - to
>> give us as much time as possible to revert it if the fix exposes
>> latent errors in previously working systems.
>>
>> > > https://edk2.groups.io/g/devel/message/50406 [PATCH 1/1]
>MdePkg/Include: Add missing definitions of SMBIOS type 42h in
>> SmBios.h
>> >
>> > Based on Abner's response in the thread, this change does not appear
>> > necessary for fixing actual functionality bugs; it rather completes a
>> > previously incomplete feature addition. And Abner is not in a rush to
>> > catch the upcoming stable tag with the patch. I suggest to delay it.
>> >
>> > If others disagree, I won't insist; the above is just my preference.
>>
>> I'm OK either way.
>>
>> > > https://edk2.groups.io/g/devel/message/50661 [PATCH] UefiCpuPkg:
>Update the coding styles
>> >
>> > Hmmm, quite undecided on this one. Does not fix a functionality bug
>> > either, but what it fixes *are* a coding style bugs, and the patch is
>> > low risk. I'm leaning towards merging it.
>>
>> I am against merging this, even though it's low-risk.
>>
>> The process says:
>> "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 received Acks 4 days late.
>>
>> If it came with a commit message indicating the incorrect comment
>> syntax caused problems with document generation, then maybe it could
>> be considered from a bugfix standpoint. But it didn't and it's too
>> late to re-scope the change at this point.
>>
>> I also dislike the mixing of doxygen formating changes and plain
>> whitespace changes. Even though trivial, it ought to be split up.
>>
>> > > https://edk2.groups.io/g/devel/message/50662 [PATCH] MdePkg:
>Update the comments of IsLanguageSupported
>> >
>> > This was even reviewed by a package maintainer (= you) before the SFF,
>> > so it can definitely go in.
>>
>> Agree (if cutting it close).
>>
>> > > https://edk2.groups.io/g/devel/message/50663 [PATCH 0/3] Add
>missing strings for uni files
>> >
>> > First of all, the structure of this series is wrong; please see my
>> > feedback here:
>> >
>> >   https://edk2.groups.io/g/devel/message/50666
>> >
>> > (The two patches discussed just above were incorrectly included in the
>> > same posting.)
>> >
>> > Second, the three patches for the UNI files add too much brand new text
>> > for my taste, for them to be considered bugfixes. The patches were
>> > posted in time for the SFF, but the maintainer reviews came too late:
>> >
>> >   https://edk2.groups.io/g/devel/message/50872
>> >   https://edk2.groups.io/g/devel/message/50869
>> >   https://edk2.groups.io/g/devel/message/50870
>> >
>> > I suggest postponing.
>>
>> Agree.
>>
>> > > https://edk2.groups.io/g/devel/message/50866 [PATCH V1 0/2]
>Improve PeiInstallPeiMemory() description
>> >
>> > I'm seriously confused by the subject prefixes in this patch thread.
>> > What's going on with the version numbers?
>> >
>> >   [edk2-devel] [PATCH V1 0/2] Improve PeiInstallPeiMemory() description
>> >   [edk2-devel] [PATCH V3 1/2] MdeModulePkg PeiCore: Improve
>PeiInstallPeiMemory() description
>> >   [edk2-devel] [PATCH V1 2/2] MdePkg PiPeiCis.h: Improve
>PeiInstallPeiMemory() description
>> >
>> > Other than that... I'm torn. I guess I could be convinced that these
>> > patches are indeed bugfixes, so I'm leaning towards merging them.
>>
>> Non-functional change submitted after start of soft-freeze?
>> I don't see why it should be considered.
>>
>> > > https://edk2.groups.io/g/devel/message/50841 [PATCH V2 1/1]
>MdeModulePkg PeiCore: Fix typos
>> >
>> > Personally I'm not happy about this patch. It's way too large for my taste:
>> >
>> >  MdeModulePkg/Core/Pei/PeiMain.inf             | 10 ++--
>> >  MdeModulePkg/Core/Pei/FwVol/FwVol.h           | 20 +++----
>> >  MdeModulePkg/Core/Pei/PeiMain.h               | 52 ++++++++--------
>> >  MdeModulePkg/Core/Pei/Dependency/Dependency.c | 12 ++--
>> >  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 51 ++++++++--------
>> >  MdeModulePkg/Core/Pei/FwVol/FwVol.c           | 63 ++++++++++----------
>> >  MdeModulePkg/Core/Pei/Hob/Hob.c               |  4 +-
>> >  MdeModulePkg/Core/Pei/Image/Image.c           | 10 ++--
>> >  MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 18 +++---
>> >  MdeModulePkg/Core/Pei/PeiMain/PeiMain.c       |  2 +-
>> >  MdeModulePkg/Core/Pei/Ppi/Ppi.c               |  4 +-
>> >  MdeModulePkg/Core/Pei/Security/Security.c     | 12 ++--
>> >  12 files changed, 129 insertions(+), 129 deletions(-)
>> >
>> > and it mixes multiple kinds of changes:
>> >
>> > "Fixes typos and clarifies some wording throughout PeiCore."
>> >
>> > When reviewing such a patch, the reviewer has a difficult time telling
>> > apart purely syntactic (typo) fixes from semantic (wording) fixes. As a
>> > reviewer I would suggest splitting this patch at least in two (typos vs.
>> > semantics). Then I could be convinced such a set of two patches is
>> > purely a bugfix.
>> >
>> > I'm leaning towards "postpone" on this one, but I can see why people
>> > would think "that's arbitrary". I guess I'll have to defer to others in
>> > this instance.
>>
>> Non-functional change submitted after start of soft-freeze?
>> I don't see why it should be considered.
>>
>> I also agree on the needs splitting up bit.
>>
>> Best Regards,
>>
>> Leif
>>
>>
>
>
>

Re: [edk2-devel] Patch List for 201911 stable tag

[Leif Lindholm]

On Thu, Nov 21, 2019 at 08:57:12AM +0000, Gao, Liming wrote:
> Hi Stewards and all:
>   New bugs are for 201911 stable tag. Can you give the comments for them?
> 
> Bug List (those all have pass code review):
> https://edk2.groups.io/g/devel/message/50931 [PATCH] MdeModulePkg: LzmaCustomDecompressLib.inf don't support EBC anymore

This can wait until after the stable tag *unless* that causes serious
issues for CI. (And I don't consider non-zero build failures for EBC
to be serious.)
The fix only addresses building the component standalone as part of
MdeModulePkg, so it affects no real platforms.

I have some comments with regards to the implementation as well, but
I'll give those on the actual patch email.

> https://edk2.groups.io/g/devel/message/50990 [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable

Those two bugs are poster children for why long functions should be
avoided.

OK for me to include in stable tag.

> https://edk2.groups.io/g/devel/message/50989 [PATCH V2] BaseTools:fix regression issue for platform .map file

OK for me to include *if* the commit message (and bugzilla) are
updated to clarify the issue. "is missing" is passive language, and
hence fails to convey what is causing the problem. Does this mean
that the 'build' command fails to generate these lines?

Also, a comment on why this change (of moving this hunk 18 lines up)
resolves the issue should be included.

> https://edk2.groups.io/g/devel/message/50936 [PATCH] BaseTools:fixed Build failed issue for Non-English OS

I agree that a fix for this bug must be included in the stable tag.
But both the BZ and the commit message need improving.
And seeing a fix that consists solely of adding "errors='ignore'"
worries me somewhat.

Please explain *what* goes wrong when using Non-English OS (and which
OS), as well as why "errors='ignore'" does not cause further problems.

Best Regards,

Leif

> Thanks
> Liming
> >-----Original Message-----
> >From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> >Liming Gao
> >Sent: Wednesday, November 20, 2019 10:52 PM
> >To: devel@edk2.groups.io; leif.lindholm@linaro.org; Laszlo Ersek
> ><lersek@redhat.com>
> >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; 'afish@apple.com'
> ><afish@apple.com>
> >Subject: Re: [edk2-devel] Patch List for 201911 stable tag
> >
> >Laszlo and Leif:
> >  Thanks for your detail review. I will continue to monitor the coming changes
> >for 201911 stable tag.
> >
> >Thanks
> >Liming
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif
> >Lindholm
> >> Sent: Wednesday, November 20, 2019 3:02 AM
> >> To: Laszlo Ersek <lersek@redhat.com>
> >> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> ><michael.d.kinney@intel.com>; 'afish@apple.com' <afish@apple.com>;
> >> devel@edk2.groups.io
> >> Subject: Re: [edk2-devel] Patch List for 201911 stable tag
> >>
> >> On Tue, Nov 19, 2019 at 06:50:19PM +0100, Laszlo Ersek wrote:
> >> > On 11/19/19 15:25, Gao, Liming wrote:
> >> > > Hi Stewards and all:
> >> > >   I collect current patch lists in devel mail list. Those patch
> >> > >   contributors request to add them for 201911 stable tag. Because the
> >> > >   time is close to Hard Feature Freeze, I want to collect your
> >> > >   feedback for below patches.
> >> > >
> >> > > Feature List (those all have pass code review):
> >> > > https://edk2.groups.io/g/devel/message/50602 [PATCH V2] BaseTools:
> >Add [packages] section in dsc file
> >> >
> >> > This patch can be merged during the Soft Feature Freeze. It was posted
> >> > before the Soft Feature Freeze, and also reviewed (by Bob, i.e. a
> >> > BaseTools Maintainer) before the Soft Feature Freeze.
> >> >
> >> > As far as I can see, there is still an outstanding question from you, to
> >> > Zhiju ("Can you show what test are done for this new support?"), so I
> >> > think we should await the response to that.
> >> >
> >> > Note that the patch should not be merged once the Hard Feature Freeze
> >> > starts, so there are ~3 days for Zhiju to answer the question about
> >> > testing (and for you to acknowledge that you are OK with the reply).
> >>
> >> Agreed.
> >>
> >> > > Bug List (those all have pass code review):
> >> > > https://edk2.groups.io/g/devel/message/50625 [PATCH v1]
> >MdeModulePkg/NvmExpressDxe: Fix wrong queue size for async IO
> >> queues
> >> >
> >> > Looks very much like a bugfix to me, so it's suitable for merging even
> >> > during the Hard Feature Freeze.
> >>
> >> I agree. But I am still slightly nervous about changing such a
> >> fundamental part of such a fundamental driver. Certainly if it is
> >> going in, I want it in ASAP, not just at the end of soft freeze - to
> >> give us as much time as possible to revert it if the fix exposes
> >> latent errors in previously working systems.
> >>
> >> > > https://edk2.groups.io/g/devel/message/50406 [PATCH 1/1]
> >MdePkg/Include: Add missing definitions of SMBIOS type 42h in
> >> SmBios.h
> >> >
> >> > Based on Abner's response in the thread, this change does not appear
> >> > necessary for fixing actual functionality bugs; it rather completes a
> >> > previously incomplete feature addition. And Abner is not in a rush to
> >> > catch the upcoming stable tag with the patch. I suggest to delay it.
> >> >
> >> > If others disagree, I won't insist; the above is just my preference.
> >>
> >> I'm OK either way.
> >>
> >> > > https://edk2.groups.io/g/devel/message/50661 [PATCH] UefiCpuPkg:
> >Update the coding styles
> >> >
> >> > Hmmm, quite undecided on this one. Does not fix a functionality bug
> >> > either, but what it fixes *are* a coding style bugs, and the patch is
> >> > low risk. I'm leaning towards merging it.
> >>
> >> I am against merging this, even though it's low-risk.
> >>
> >> The process says:
> >> "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 received Acks 4 days late.
> >>
> >> If it came with a commit message indicating the incorrect comment
> >> syntax caused problems with document generation, then maybe it could
> >> be considered from a bugfix standpoint. But it didn't and it's too
> >> late to re-scope the change at this point.
> >>
> >> I also dislike the mixing of doxygen formating changes and plain
> >> whitespace changes. Even though trivial, it ought to be split up.
> >>
> >> > > https://edk2.groups.io/g/devel/message/50662 [PATCH] MdePkg:
> >Update the comments of IsLanguageSupported
> >> >
> >> > This was even reviewed by a package maintainer (= you) before the SFF,
> >> > so it can definitely go in.
> >>
> >> Agree (if cutting it close).
> >>
> >> > > https://edk2.groups.io/g/devel/message/50663 [PATCH 0/3] Add
> >missing strings for uni files
> >> >
> >> > First of all, the structure of this series is wrong; please see my
> >> > feedback here:
> >> >
> >> >   https://edk2.groups.io/g/devel/message/50666
> >> >
> >> > (The two patches discussed just above were incorrectly included in the
> >> > same posting.)
> >> >
> >> > Second, the three patches for the UNI files add too much brand new text
> >> > for my taste, for them to be considered bugfixes. The patches were
> >> > posted in time for the SFF, but the maintainer reviews came too late:
> >> >
> >> >   https://edk2.groups.io/g/devel/message/50872
> >> >   https://edk2.groups.io/g/devel/message/50869
> >> >   https://edk2.groups.io/g/devel/message/50870
> >> >
> >> > I suggest postponing.
> >>
> >> Agree.
> >>
> >> > > https://edk2.groups.io/g/devel/message/50866 [PATCH V1 0/2]
> >Improve PeiInstallPeiMemory() description
> >> >
> >> > I'm seriously confused by the subject prefixes in this patch thread.
> >> > What's going on with the version numbers?
> >> >
> >> >   [edk2-devel] [PATCH V1 0/2] Improve PeiInstallPeiMemory() description
> >> >   [edk2-devel] [PATCH V3 1/2] MdeModulePkg PeiCore: Improve
> >PeiInstallPeiMemory() description
> >> >   [edk2-devel] [PATCH V1 2/2] MdePkg PiPeiCis.h: Improve
> >PeiInstallPeiMemory() description
> >> >
> >> > Other than that... I'm torn. I guess I could be convinced that these
> >> > patches are indeed bugfixes, so I'm leaning towards merging them.
> >>
> >> Non-functional change submitted after start of soft-freeze?
> >> I don't see why it should be considered.
> >>
> >> > > https://edk2.groups.io/g/devel/message/50841 [PATCH V2 1/1]
> >MdeModulePkg PeiCore: Fix typos
> >> >
> >> > Personally I'm not happy about this patch. It's way too large for my taste:
> >> >
> >> >  MdeModulePkg/Core/Pei/PeiMain.inf             | 10 ++--
> >> >  MdeModulePkg/Core/Pei/FwVol/FwVol.h           | 20 +++----
> >> >  MdeModulePkg/Core/Pei/PeiMain.h               | 52 ++++++++--------
> >> >  MdeModulePkg/Core/Pei/Dependency/Dependency.c | 12 ++--
> >> >  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 51 ++++++++--------
> >> >  MdeModulePkg/Core/Pei/FwVol/FwVol.c           | 63 ++++++++++----------
> >> >  MdeModulePkg/Core/Pei/Hob/Hob.c               |  4 +-
> >> >  MdeModulePkg/Core/Pei/Image/Image.c           | 10 ++--
> >> >  MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 18 +++---
> >> >  MdeModulePkg/Core/Pei/PeiMain/PeiMain.c       |  2 +-
> >> >  MdeModulePkg/Core/Pei/Ppi/Ppi.c               |  4 +-
> >> >  MdeModulePkg/Core/Pei/Security/Security.c     | 12 ++--
> >> >  12 files changed, 129 insertions(+), 129 deletions(-)
> >> >
> >> > and it mixes multiple kinds of changes:
> >> >
> >> > "Fixes typos and clarifies some wording throughout PeiCore."
> >> >
> >> > When reviewing such a patch, the reviewer has a difficult time telling
> >> > apart purely syntactic (typo) fixes from semantic (wording) fixes. As a
> >> > reviewer I would suggest splitting this patch at least in two (typos vs.
> >> > semantics). Then I could be convinced such a set of two patches is
> >> > purely a bugfix.
> >> >
> >> > I'm leaning towards "postpone" on this one, but I can see why people
> >> > would think "that's arbitrary". I guess I'll have to defer to others in
> >> > this instance.
> >>
> >> Non-functional change submitted after start of soft-freeze?
> >> I don't see why it should be considered.
> >>
> >> I also agree on the needs splitting up bit.
> >>
> >> Best Regards,
> >>
> >> Leif
> >>
> >>
> >
> >
> >
> 

Re: [edk2-devel] Patch List for 201911 stable tag

[Laszlo Ersek]

On 11/21/19 11:37, Leif Lindholm wrote:
> On Thu, Nov 21, 2019 at 08:57:12AM +0000, Gao, Liming wrote:
>> Hi Stewards and all:
>>   New bugs are for 201911 stable tag. Can you give the comments for them?
>>
>> Bug List (those all have pass code review):
>> https://edk2.groups.io/g/devel/message/50931 [PATCH] MdeModulePkg: LzmaCustomDecompressLib.inf don't support EBC anymore
> 
> This can wait until after the stable tag *unless* that causes serious
> issues for CI. (And I don't consider non-zero build failures for EBC
> to be serious.)
> The fix only addresses building the component standalone as part of
> MdeModulePkg, so it affects no real platforms.

I would have beem more permissive with this patch, but I'm OK with the
above resolution too.

> 
> I have some comments with regards to the implementation as well, but
> I'll give those on the actual patch email.
> 
>> https://edk2.groups.io/g/devel/message/50990 [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable
> 
> Those two bugs are poster children for why long functions should be
> avoided.
> 
> OK for me to include in stable tag.

Agreed.

> 
>> https://edk2.groups.io/g/devel/message/50989 [PATCH V2] BaseTools:fix regression issue for platform .map file
> 
> OK for me to include *if* the commit message (and bugzilla) are
> updated to clarify the issue. "is missing" is passive language, and
> hence fails to convey what is causing the problem. Does this mean
> that the 'build' command fails to generate these lines?
> 
> Also, a comment on why this change (of moving this hunk 18 lines up)
> resolves the issue should be included.

Agreed; please add a reminder to the commit message (even on push) about
what the specific (missing) lines in the map files are good for in the
first place.

> 
>> https://edk2.groups.io/g/devel/message/50936 [PATCH] BaseTools:fixed Build failed issue for Non-English OS
> 
> I agree that a fix for this bug must be included in the stable tag.
> But both the BZ and the commit message need improving.
> And seeing a fix that consists solely of adding "errors='ignore'"
> worries me somewhat.
> 
> Please explain *what* goes wrong when using Non-English OS (and which
> OS), as well as why "errors='ignore'" does not cause further problems.

I think this patch is plain wrong. I'll comment on it in its own thread.

Thanks
Laszlo

Re: [edk2-devel] Patch List for 201911 stable tag

[Bob Feng]

Patch "fix regression issue for platform .map file" was pushed.

More comments inline.

Thanks,
Bob
-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
Sent: Friday, November 22, 2019 1:26 AM
To: Leif Lindholm <leif.lindholm@linaro.org>; Gao, Liming <liming.gao@intel.com>
Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; 'afish@apple.com' <afish@apple.com>
Subject: Re: [edk2-devel] Patch List for 201911 stable tag

On 11/21/19 11:37, Leif Lindholm wrote:
> On Thu, Nov 21, 2019 at 08:57:12AM +0000, Gao, Liming wrote:
>> Hi Stewards and all:
>>   New bugs are for 201911 stable tag. Can you give the comments for them?
>>
>> Bug List (those all have pass code review):
>> https://edk2.groups.io/g/devel/message/50931 [PATCH] MdeModulePkg: 
>> LzmaCustomDecompressLib.inf don't support EBC anymore
> 
> This can wait until after the stable tag *unless* that causes serious 
> issues for CI. (And I don't consider non-zero build failures for EBC 
> to be serious.) The fix only addresses building the component 
> standalone as part of MdeModulePkg, so it affects no real platforms.

I would have beem more permissive with this patch, but I'm OK with the above resolution too.

> 
> I have some comments with regards to the implementation as well, but 
> I'll give those on the actual patch email.
> 
>> https://edk2.groups.io/g/devel/message/50990 [PATCH V1 1/1] 
>> MdeModulePkg/Variable: Initialize local variable
> 
> Those two bugs are poster children for why long functions should be 
> avoided.
> 
> OK for me to include in stable tag.

Agreed.

> 
>> https://edk2.groups.io/g/devel/message/50989 [PATCH V2] BaseTools:fix 
>> regression issue for platform .map file
> 
> OK for me to include *if* the commit message (and bugzilla) are 
> updated to clarify the issue. "is missing" is passive language, and 
> hence fails to convey what is causing the problem. Does this mean that 
> the 'build' command fails to generate these lines?
> 
> Also, a comment on why this change (of moving this hunk 18 lines up) 
> resolves the issue should be included.

Agreed; please add a reminder to the commit message (even on push) about what the specific (missing) lines in the map files are good for in the first place.

[Bob] The commit message was updated https://edk2.groups.io/g/devel/message/51212
And the patch was pushed.

> 
>> https://edk2.groups.io/g/devel/message/50936 [PATCH] BaseTools:fixed 
>> Build failed issue for Non-English OS
> 
> I agree that a fix for this bug must be included in the stable tag.
> But both the BZ and the commit message need improving.
> And seeing a fix that consists solely of adding "errors='ignore'"
> worries me somewhat.
> 
> Please explain *what* goes wrong when using Non-English OS (and which 
> OS), as well as why "errors='ignore'" does not cause further problems.

I think this patch is plain wrong. I'll comment on it in its own thread.

Thanks
Laszlo