* [PATCH V2] BaseTools:Add extra debugging message @ 2019-07-25 3:02 Fan, ZhijuX 2019-07-25 11:04 ` [edk2-devel] " Philippe Mathieu-Daudé 2019-08-01 12:19 ` Bob Feng 0 siblings, 2 replies; 9+ messages in thread From: Fan, ZhijuX @ 2019-07-25 3:02 UTC (permalink / raw) To: devel; +Cc: Max Knutsen, Bob Feng, Liming Gao, Zhiju . Fan 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: + EdkLogger.error("Trim", AUTOGEN_ERROR, "TrimPreprocessedFile: Error while processing file", File=Source) PreprocessedFile = "" InjectedFile = "" -- 2.14.1.windows.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH V2] BaseTools:Add extra debugging message 2019-07-25 3:02 [PATCH V2] BaseTools:Add extra debugging message Fan, ZhijuX @ 2019-07-25 11:04 ` Philippe Mathieu-Daudé 2019-07-26 6:43 ` Liming Gao 2019-08-01 12:19 ` Bob Feng 1 sibling, 1 reply; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2019-07-25 11:04 UTC (permalink / raw) To: devel, zhijux.fan; +Cc: Max Knutsen, Bob Feng, Liming Gao Nit in subject, "BaseTools:<space>Add extra debugging message" On 7/25/19 5:02 AM, Fan, ZhijuX wrote: > 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: > + EdkLogger.error("Trim", AUTOGEN_ERROR, "TrimPreprocessedFile: Error while processing file", File=Source) > > PreprocessedFile = "" > InjectedFile = "" > Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH V2] BaseTools:Add extra debugging message 2019-07-25 11:04 ` [edk2-devel] " Philippe Mathieu-Daudé @ 2019-07-26 6:43 ` Liming Gao 0 siblings, 0 replies; 9+ messages in thread From: Liming Gao @ 2019-07-26 6:43 UTC (permalink / raw) To: devel@edk2.groups.io, philmd@redhat.com, Fan, ZhijuX Cc: Max Knutsen, Feng, Bob C >-----Original Message----- >From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >Philippe Mathieu-Daudé >Sent: Thursday, July 25, 2019 7:04 PM >To: devel@edk2.groups.io; Fan, ZhijuX <zhijux.fan@intel.com> >Cc: Max Knutsen <maknutse@microsoft.com>; Feng, Bob C ><bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com> >Subject: Re: [edk2-devel] [PATCH V2] BaseTools:Add extra debugging >message > >Nit in subject, "BaseTools:<space>Add extra debugging message" Title can remove extra <space>. > >On 7/25/19 5:02 AM, Fan, ZhijuX wrote: >> 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: >> + EdkLogger.error("Trim", AUTOGEN_ERROR, "TrimPreprocessedFile: >Error while processing file", File=Source) >> >> PreprocessedFile = "" >> InjectedFile = "" >> > >Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com> > Reviewed-by: Liming Gao <liming.gao@intel.com> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH V2] BaseTools:Add extra debugging message 2019-07-25 3:02 [PATCH V2] BaseTools:Add extra debugging message Fan, ZhijuX 2019-07-25 11:04 ` [edk2-devel] " Philippe Mathieu-Daudé @ 2019-08-01 12:19 ` Bob Feng 2019-08-01 12:57 ` Liming Gao 1 sibling, 1 reply; 9+ messages in thread From: Bob Feng @ 2019-08-01 12:19 UTC (permalink / raw) To: devel@edk2.groups.io, Fan, ZhijuX; +Cc: Max Knutsen, Gao, Liming 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH V2] BaseTools:Add extra debugging message 2019-08-01 12:19 ` Bob Feng @ 2019-08-01 12:57 ` Liming Gao 2019-08-02 9:55 ` Microsoft imports - was " Leif Lindholm 0 siblings, 1 reply; 9+ messages in thread From: Liming Gao @ 2019-08-01 12:57 UTC (permalink / raw) To: Feng, Bob C, devel@edk2.groups.io, Fan, ZhijuX; +Cc: Max Knutsen 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 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Microsoft imports - was Re: [edk2-devel] [PATCH V2] BaseTools:Add extra debugging message 2019-08-01 12:57 ` Liming Gao @ 2019-08-02 9:55 ` Leif Lindholm 2019-08-02 10:06 ` Philippe Mathieu-Daudé ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Leif Lindholm @ 2019-08-02 9:55 UTC (permalink / raw) To: devel Cc: Feng, Bob C, Fan, ZhijuX, Max Knutsen, Philippe Mathieu-Daude, Andrew Fish, Laszlo Ersek, Michael D Kinney, Liming Gao 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. 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. 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. / 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 > > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Microsoft imports - was Re: [edk2-devel] [PATCH V2] BaseTools:Add extra debugging message 2019-08-02 9:55 ` Microsoft imports - was " Leif Lindholm @ 2019-08-02 10:06 ` Philippe Mathieu-Daudé 2019-08-02 21:32 ` rebecca 2019-08-07 11:19 ` Liming Gao 2 siblings, 0 replies; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2019-08-02 10:06 UTC (permalink / raw) To: devel, leif.lindholm Cc: Feng, Bob C, Fan, ZhijuX, Max Knutsen, Andrew Fish, Laszlo Ersek, Michael D Kinney, Liming Gao On 8/2/19 11:55 AM, Leif Lindholm wrote: > 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. I'm embarrassed because I reviewed it and missed the typo. FWIW, when I only review the logic looking at the patch, I reply with the Reviewed-by tag. If I apply the patch locally, build, run testing, then I use the Tested-by tag. > 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 am working on EDK2 CI in my spare time (so far a low priority assignment) and have about 60 combinations of x86/aarch64 builds covered on Linux host. However there is no testing of the build firmware yet, which is the most important part. Anyway this would have catch this mistake. Also, this effort indeed takes time and should be a community effort. > 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. I'm sorry and will be more careful in my Python/BaseTools reviews. Regards, Phil. > 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. > > / > 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 >>> >>> >>> >> >> >> >> > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Microsoft imports - was Re: [edk2-devel] [PATCH V2] BaseTools:Add extra debugging message 2019-08-02 9:55 ` Microsoft imports - was " Leif Lindholm 2019-08-02 10:06 ` Philippe Mathieu-Daudé @ 2019-08-02 21:32 ` rebecca 2019-08-07 11:19 ` Liming Gao 2 siblings, 0 replies; 9+ messages in thread From: rebecca @ 2019-08-02 21:32 UTC (permalink / raw) To: devel, leif.lindholm Cc: Feng, Bob C, Fan, ZhijuX, Max Knutsen, Philippe Mathieu-Daude, Andrew Fish, Laszlo Ersek, Michael D Kinney, Liming Gao On 2019-08-02 03:55, Leif Lindholm wrote: > 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. Could we perhaps at _least_ run a tool like pyflakes? That trivially finds the error: bcran@photon:~/workspace/edk2/BaseTools/Source/Python/Trim % pyflakes Trim.py Trim.py:181:12: invalid syntax expect: ^ -- Rebecca Cran ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Microsoft imports - was Re: [edk2-devel] [PATCH V2] BaseTools:Add extra debugging message 2019-08-02 9:55 ` Microsoft imports - was " Leif Lindholm 2019-08-02 10:06 ` Philippe Mathieu-Daudé 2019-08-02 21:32 ` rebecca @ 2019-08-07 11:19 ` Liming Gao 2 siblings, 0 replies; 9+ messages in thread From: Liming Gao @ 2019-08-07 11:19 UTC (permalink / raw) To: Leif Lindholm, devel@edk2.groups.io Cc: Feng, Bob C, Fan, ZhijuX, Max Knutsen, Philippe Mathieu-Daude, Andrew Fish, Laszlo Ersek, Kinney, Michael D 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 >> > >> > >> > >> >> >> >> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-08-07 11:19 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-07-25 3:02 [PATCH V2] BaseTools:Add extra debugging message Fan, ZhijuX 2019-07-25 11:04 ` [edk2-devel] " Philippe Mathieu-Daudé 2019-07-26 6:43 ` Liming Gao 2019-08-01 12:19 ` Bob Feng 2019-08-01 12:57 ` Liming Gao 2019-08-02 9:55 ` Microsoft imports - was " Leif Lindholm 2019-08-02 10:06 ` Philippe Mathieu-Daudé 2019-08-02 21:32 ` rebecca 2019-08-07 11:19 ` Liming Gao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox