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

["Leif Lindholm" <leif.lindholm@linaro.org>]

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
> >>
> >>
> >
> >
> >
>