Re: Microsoft imports - was Re: [edk2-devel] [PATCH V2] BaseTools:Add extra debugging message

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

Leif:

>-----Original Message-----
>From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>Sent: Friday, August 02, 2019 5:55 PM
>To: devel@edk2.groups.io
>Cc: Feng, Bob C <bob.c.feng@intel.com>; Fan, ZhijuX <zhijux.fan@intel.com>;
>Max Knutsen <maknutse@microsoft.com>; Philippe Mathieu-Daude
><philmd@redhat.com>; Andrew Fish <afish@apple.com>; Laszlo Ersek
><lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>Gao, Liming <liming.gao@intel.com>
>Subject: Microsoft imports - was Re: [edk2-devel] [PATCH V2] BaseTools:Add
>extra debugging message
>
>So, I'm not going to give any of the reviewers a hard time about this
>- the patch *looks* right and we've all occasionally given R-b on
>things we haven't actually tested because we don't always have the
>time. And by the time I hit it, it had already been fixed upstream.
>
>But what worries me is that not only was this an error that would have
>been caught by a simple build test - it was imported from an external
>project where the same would have applied.

I double check the code. Original code is good. This is a patch sync issue. 

>
>Chucking patched over the wall from another open source project is no
>improvement over chucking internal patches over the wall without
>proper (contributor) review or testing.

Yes. The developer unit test is important. 

>
>Or was it modified on the way across?
>
>One change I would like to see enacted *immediately* is that *any*
>imports from other open source projects state the repository and the
>commit hash that it originated from. In the commit message - and where
>BZs are referenced in the message, also copied into the BZ.
>

Good suggestion. I will update BZ to include those information. 

Thanks
Liming
>/
>    Leif
>
>On Thu, Aug 01, 2019 at 12:57:36PM +0000, Liming Gao wrote:
>> Good catch. I have pushed this patch. Can you send one new patch to fix it?
>>
>> > -----Original Message-----
>> > From: Feng, Bob C
>> > Sent: Thursday, August 1, 2019 8:20 PM
>> > To: devel@edk2.groups.io; Fan, ZhijuX <zhijux.fan@intel.com>
>> > Cc: Max Knutsen <maknutse@microsoft.com>; Gao, Liming
><liming.gao@intel.com>
>> > Subject: RE: [edk2-devel] [PATCH V2] BaseTools:Add extra debugging
>message
>> >
>> > Hi Zhiju,
>> >
>> > There is a typo in this patch. See it inline.
>> >
>> > Thanks,
>> > Bob
>> >
>> > -----Original Message-----
>> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>Fan, ZhijuX
>> > Sent: Thursday, July 25, 2019 11:02 AM
>> > To: devel@edk2.groups.io
>> > Cc: Max Knutsen <maknutse@microsoft.com>; Feng, Bob C
><bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; Fan, ZhijuX
>> > <zhijux.fan@intel.com>
>> > Subject: [edk2-devel] [PATCH V2] BaseTools:Add extra debugging
>message
>> >
>> > From: Max Knutsen <maknutse@microsoft.com>
>> >
>> > BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=2014
>> >
>> > Add extra debugging to improve error identification.
>> > Error while processing file if the file is read incorrectly
>> >
>> > This patch is going to fix that issue.
>> >
>> > Cc: Bob Feng <bob.c.feng@intel.com>
>> > Cc: Liming Gao <liming.gao@intel.com>
>> > Signed-off-by: Zhiju.Fan <zhijux.fan@intel.com>
>> > ---
>> >  BaseTools/Source/Python/AutoGen/StrGather.py | 16 ++++++++++------
>> >  BaseTools/Source/Python/Trim/Trim.py         |  4 +++-
>> >  2 files changed, 13 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/BaseTools/Source/Python/AutoGen/StrGather.py
>b/BaseTools/Source/Python/AutoGen/StrGather.py
>> > index 2e4671a433..eed30388be 100644
>> > --- a/BaseTools/Source/Python/AutoGen/StrGather.py
>> > +++ b/BaseTools/Source/Python/AutoGen/StrGather.py
>> > @@ -526,12 +526,16 @@ def SearchString(UniObjectClass, FileList,
>IsCompatibleMode):
>> >          return UniObjectClass
>> >
>> >      for File in FileList:
>> > -        if os.path.isfile(File):
>> > -            Lines = open(File, 'r')
>> > -            for Line in Lines:
>> > -                for StrName in STRING_TOKEN.findall(Line):
>> > -                    EdkLogger.debug(EdkLogger.DEBUG_5, "Found string identifier:
>" + StrName)
>> > -                    UniObjectClass.SetStringReferenced(StrName)
>> > +        try:
>> > +            if os.path.isfile(File):
>> > +                Lines = open(File, 'r')
>> > +                for Line in Lines:
>> > +                    for StrName in STRING_TOKEN.findall(Line):
>> > +                        EdkLogger.debug(EdkLogger.DEBUG_5, "Found string
>identifier: " + StrName)
>> > +                        UniObjectClass.SetStringReferenced(StrName)
>> > +        except:
>> > +            EdkLogger.error("UnicodeStringGather", AUTOGEN_ERROR,
>"SearchString: Error while processing file", File=File,
>> > RaiseError=False)
>> > +            raise
>> >
>> >      UniObjectClass.ReToken()
>> >
>> > diff --git a/BaseTools/Source/Python/Trim/Trim.py
>b/BaseTools/Source/Python/Trim/Trim.py
>> > index 43119bd7ff..8767b67f7e 100644
>> > --- a/BaseTools/Source/Python/Trim/Trim.py
>> > +++ b/BaseTools/Source/Python/Trim/Trim.py
>> > @@ -73,8 +73,10 @@ def TrimPreprocessedFile(Source, Target,
>ConvertHex, TrimLong):
>> >      try:
>> >          with open(Source, "r") as File:
>> >              Lines = File.readlines()
>> > -    except:
>> > +    except IOError:
>> >          EdkLogger.error("Trim", FILE_OPEN_FAILURE, ExtraData=Source)
>> > +    expect:
>> >
>> > Typo here. expect should except
>> >
>> > +        EdkLogger.error("Trim", AUTOGEN_ERROR, "TrimPreprocessedFile:
>Error while processing file", File=Source)
>> >
>> >      PreprocessedFile = ""
>> >      InjectedFile = ""
>> > --
>> > 2.14.1.windows.1
>> >
>> >
>> >
>>
>>
>> 
>>