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

["Bob Feng" <bob.c.feng@intel.com>]

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