* [Patch 0/3] BaseTools: PatchCheck to align with wiki and report error for EFI_D_* @ 2016-10-17 8:28 Yonghong Zhu 2016-10-17 8:28 ` [Patch 1/3] BaseTools: Update PatchCheck for max length of subject and message line Yonghong Zhu ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Yonghong Zhu @ 2016-10-17 8:28 UTC (permalink / raw) To: edk2-devel; +Cc: Liming Gao, Jordan Justen These patches address the following 2 bugs: https://bugzilla.tianocore.org/show_bug.cgi?id=113 https://bugzilla.tianocore.org/show_bug.cgi?id=145 Cc: Liming Gao <liming.gao@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com> Yonghong Zhu (3): BaseTools: Update PatchCheck for max length of subject and message line BaseTools: Update PatchCheck to handle the two [] as prefix BaseTools: Update PatchCheck to report error for EFI_D_* BaseTools/Scripts/PatchCheck.py | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) -- 2.6.1.windows.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Patch 1/3] BaseTools: Update PatchCheck for max length of subject and message line 2016-10-17 8:28 [Patch 0/3] BaseTools: PatchCheck to align with wiki and report error for EFI_D_* Yonghong Zhu @ 2016-10-17 8:28 ` Yonghong Zhu 2016-10-17 16:40 ` Jordan Justen 2016-10-17 8:28 ` [Patch 2/3] BaseTools: Update PatchCheck to handle the two [] as prefix Yonghong Zhu 2016-10-17 8:28 ` [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_* Yonghong Zhu 2 siblings, 1 reply; 12+ messages in thread From: Yonghong Zhu @ 2016-10-17 8:28 UTC (permalink / raw) To: edk2-devel; +Cc: Liming Gao, Jordan Justen This patch update PatchCheck.py: 1. The subject line of the commit message should be < 72 characters. 2. The other lines of the commit message should be < 76 characters. Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=113 Cc: Liming Gao <liming.gao@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com> --- BaseTools/Scripts/PatchCheck.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py index 455c130..07fca68 100755 --- a/BaseTools/Scripts/PatchCheck.py +++ b/BaseTools/Scripts/PatchCheck.py @@ -1,9 +1,9 @@ ## @file # Check a patch for various format issues # -# Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> +# Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR> # # This program and the accompanying materials are licensed and made # available under the terms and conditions of the BSD License which # accompanies this distribution. The full text of the license may be # found at http://opensource.org/licenses/bsd-license.php @@ -14,11 +14,11 @@ # from __future__ import print_function VersionNumber = '0.1' -__copyright__ = "Copyright (c) 2015, Intel Corporation All rights reserved." +__copyright__ = "Copyright (c) 2015 - 2016, Intel Corporation All rights reserved." import email import argparse import os import re @@ -195,11 +195,11 @@ class CommitMessageCheck: if count <= 0: self.error('Empty commit message!') return - if count >= 1 and len(lines[0]) > 76: + if count >= 1 and len(lines[0]) >= 72: self.error('First line of commit message (subject line) ' + 'is too long.') if count >= 1 and len(lines[0].strip()) == 0: self.error('First line of commit message (subject line) ' + @@ -208,11 +208,11 @@ class CommitMessageCheck: if count >= 2 and lines[1].strip() != '': self.error('Second line of commit message should be ' + 'empty.') for i in range(2, count): - if (len(lines[i]) > 76 and + if (len(lines[i]) >= 76 and len(lines[i].split()) > 1 and not lines[i].startswith('git-svn-id:')): self.error('Line %d of commit message is too long.' % (i + 1)) last_sig_line = None -- 2.6.1.windows.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Patch 1/3] BaseTools: Update PatchCheck for max length of subject and message line 2016-10-17 8:28 ` [Patch 1/3] BaseTools: Update PatchCheck for max length of subject and message line Yonghong Zhu @ 2016-10-17 16:40 ` Jordan Justen 2016-10-18 0:49 ` Zhu, Yonghong 0 siblings, 1 reply; 12+ messages in thread From: Jordan Justen @ 2016-10-17 16:40 UTC (permalink / raw) To: Yonghong Zhu, edk2-devel; +Cc: Liming Gao On 2016-10-17 01:28:36, Yonghong Zhu wrote: > This patch update PatchCheck.py: > 1. The subject line of the commit message should be < 72 characters. > 2. The other lines of the commit message should be < 76 characters. > > Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=113 > Can you remove the blank line after 'Fixes:'? The end of the commit message has various tags, like Fixes, Cc, Contributed-under, Signed-off-by, Reviewed-by, etc. They can be one tag per line and grouped together. Patch 1 Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com> > --- > BaseTools/Scripts/PatchCheck.py | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py > index 455c130..07fca68 100755 > --- a/BaseTools/Scripts/PatchCheck.py > +++ b/BaseTools/Scripts/PatchCheck.py > @@ -1,9 +1,9 @@ > ## @file > # Check a patch for various format issues > # > -# Copyright (c) 2015, Intel Corporation. All rights reserved.<BR> > +# Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR> > # > # This program and the accompanying materials are licensed and made > # available under the terms and conditions of the BSD License which > # accompanies this distribution. The full text of the license may be > # found at http://opensource.org/licenses/bsd-license.php > @@ -14,11 +14,11 @@ > # > > from __future__ import print_function > > VersionNumber = '0.1' > -__copyright__ = "Copyright (c) 2015, Intel Corporation All rights reserved." > +__copyright__ = "Copyright (c) 2015 - 2016, Intel Corporation All rights reserved." > > import email > import argparse > import os > import re > @@ -195,11 +195,11 @@ class CommitMessageCheck: > > if count <= 0: > self.error('Empty commit message!') > return > > - if count >= 1 and len(lines[0]) > 76: > + if count >= 1 and len(lines[0]) >= 72: > self.error('First line of commit message (subject line) ' + > 'is too long.') > > if count >= 1 and len(lines[0].strip()) == 0: > self.error('First line of commit message (subject line) ' + > @@ -208,11 +208,11 @@ class CommitMessageCheck: > if count >= 2 and lines[1].strip() != '': > self.error('Second line of commit message should be ' + > 'empty.') > > for i in range(2, count): > - if (len(lines[i]) > 76 and > + if (len(lines[i]) >= 76 and > len(lines[i].split()) > 1 and > not lines[i].startswith('git-svn-id:')): > self.error('Line %d of commit message is too long.' % (i + 1)) > > last_sig_line = None > -- > 2.6.1.windows.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 1/3] BaseTools: Update PatchCheck for max length of subject and message line 2016-10-17 16:40 ` Jordan Justen @ 2016-10-18 0:49 ` Zhu, Yonghong 0 siblings, 0 replies; 12+ messages in thread From: Zhu, Yonghong @ 2016-10-18 0:49 UTC (permalink / raw) To: Justen, Jordan L, edk2-devel@lists.01.org; +Cc: Gao, Liming, Zhu, Yonghong I will remove the blank line after 'Fixes:' when I push this patch. thanks for the comment. Best Regards, Zhu Yonghong -----Original Message----- From: Justen, Jordan L Sent: Tuesday, October 18, 2016 12:40 AM To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org Cc: Gao, Liming <liming.gao@intel.com> Subject: Re: [edk2] [Patch 1/3] BaseTools: Update PatchCheck for max length of subject and message line On 2016-10-17 01:28:36, Yonghong Zhu wrote: > This patch update PatchCheck.py: > 1. The subject line of the commit message should be < 72 characters. > 2. The other lines of the commit message should be < 76 characters. > > Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=113 > Can you remove the blank line after 'Fixes:'? The end of the commit message has various tags, like Fixes, Cc, Contributed-under, Signed-off-by, Reviewed-by, etc. They can be one tag per line and grouped together. Patch 1 Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com> > --- > BaseTools/Scripts/PatchCheck.py | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/BaseTools/Scripts/PatchCheck.py > b/BaseTools/Scripts/PatchCheck.py index 455c130..07fca68 100755 > --- a/BaseTools/Scripts/PatchCheck.py > +++ b/BaseTools/Scripts/PatchCheck.py > @@ -1,9 +1,9 @@ > ## @file > # Check a patch for various format issues # -# Copyright (c) 2015, > Intel Corporation. All rights reserved.<BR> > +# Copyright (c) 2015 - 2016, Intel Corporation. All rights > +reserved.<BR> > # > # This program and the accompanying materials are licensed and made > # available under the terms and conditions of the BSD License which > # accompanies this distribution. The full text of the license may be > # found at http://opensource.org/licenses/bsd-license.php > @@ -14,11 +14,11 @@ > # > > from __future__ import print_function > > VersionNumber = '0.1' > -__copyright__ = "Copyright (c) 2015, Intel Corporation All rights reserved." > +__copyright__ = "Copyright (c) 2015 - 2016, Intel Corporation All rights reserved." > > import email > import argparse > import os > import re > @@ -195,11 +195,11 @@ class CommitMessageCheck: > > if count <= 0: > self.error('Empty commit message!') > return > > - if count >= 1 and len(lines[0]) > 76: > + if count >= 1 and len(lines[0]) >= 72: > self.error('First line of commit message (subject line) ' + > 'is too long.') > > if count >= 1 and len(lines[0].strip()) == 0: > self.error('First line of commit message (subject line) ' > + @@ -208,11 +208,11 @@ class CommitMessageCheck: > if count >= 2 and lines[1].strip() != '': > self.error('Second line of commit message should be ' + > 'empty.') > > for i in range(2, count): > - if (len(lines[i]) > 76 and > + if (len(lines[i]) >= 76 and > len(lines[i].split()) > 1 and > not lines[i].startswith('git-svn-id:')): > self.error('Line %d of commit message is too long.' % > (i + 1)) > > last_sig_line = None > -- > 2.6.1.windows.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Patch 2/3] BaseTools: Update PatchCheck to handle the two [] as prefix 2016-10-17 8:28 [Patch 0/3] BaseTools: PatchCheck to align with wiki and report error for EFI_D_* Yonghong Zhu 2016-10-17 8:28 ` [Patch 1/3] BaseTools: Update PatchCheck for max length of subject and message line Yonghong Zhu @ 2016-10-17 8:28 ` Yonghong Zhu 2016-10-17 16:48 ` Jordan Justen 2016-10-17 8:28 ` [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_* Yonghong Zhu 2 siblings, 1 reply; 12+ messages in thread From: Yonghong Zhu @ 2016-10-17 8:28 UTC (permalink / raw) To: edk2-devel; +Cc: Liming Gao, Jordan Justen The bug is that only remove the first [] when it does the char count, however sometimes we use [edk2][patch] as prefix, this patch fix this bug. Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=113 Cc: Liming Gao <liming.gao@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com> --- BaseTools/Scripts/PatchCheck.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py index 07fca68..05f8f6e 100755 --- a/BaseTools/Scripts/PatchCheck.py +++ b/BaseTools/Scripts/PatchCheck.py @@ -434,10 +434,18 @@ class CheckOnePatch: [\s\S\r\n]+ ) ''', re.IGNORECASE | re.VERBOSE | re.MULTILINE) + subject_prefix_re = \ + re.compile(r'''^ + \s* (\[ + [^\[\]]* # Allow all non-brackets + \])* \s* + ''', + re.VERBOSE) + def find_patch_pieces(self): if sys.version_info < (3, 0): patch = self.patch.encode('ascii', 'ignore') else: patch = self.patch @@ -470,18 +478,11 @@ class CheckOnePatch: self.stat = mo.group('stat') self.commit_msg = mo.group('commit_message') self.commit_subject = pmail['subject'].replace('\r\n', '') self.commit_subject = self.commit_subject.replace('\n', '') - - pfx_start = self.commit_subject.find('[') - if pfx_start >= 0: - pfx_end = self.commit_subject.find(']') - if pfx_end > pfx_start: - self.commit_prefix = self.commit_subject[pfx_start + 1 : pfx_end] - self.commit_subject = self.commit_subject[pfx_end + 1 :].lstrip() - + self.commit_subject = self.subject_prefix_re.sub('', self.commit_subject, 1) class CheckGitCommits: """Reads patches from git based on the specified git revision range. The patches are read from git, and then checked. -- 2.6.1.windows.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Patch 2/3] BaseTools: Update PatchCheck to handle the two [] as prefix 2016-10-17 8:28 ` [Patch 2/3] BaseTools: Update PatchCheck to handle the two [] as prefix Yonghong Zhu @ 2016-10-17 16:48 ` Jordan Justen 2016-10-18 0:53 ` Zhu, Yonghong 0 siblings, 1 reply; 12+ messages in thread From: Jordan Justen @ 2016-10-17 16:48 UTC (permalink / raw) To: Yonghong Zhu, edk2-devel; +Cc: Liming Gao A small (not too important) suggestion for the patch subject might be: BaseTools/PatchCheck.py: Update to handle the two [] as prefix On 2016-10-17 01:28:37, Yonghong Zhu wrote: > The bug is that only remove the first [] when it does the char count, > however sometimes we use [edk2][patch] as prefix, this patch fix this bug. > > Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=113 > Remove blank line after 'Fixes:'. > Cc: Liming Gao <liming.gao@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com> > --- > BaseTools/Scripts/PatchCheck.py | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py > index 07fca68..05f8f6e 100755 > --- a/BaseTools/Scripts/PatchCheck.py > +++ b/BaseTools/Scripts/PatchCheck.py > @@ -434,10 +434,18 @@ class CheckOnePatch: > [\s\S\r\n]+ > ) > ''', > re.IGNORECASE | re.VERBOSE | re.MULTILINE) > > + subject_prefix_re = \ > + re.compile(r'''^ > + \s* (\[ > + [^\[\]]* # Allow all non-brackets > + \])* \s* > + ''', > + re.VERBOSE) > + > def find_patch_pieces(self): > if sys.version_info < (3, 0): > patch = self.patch.encode('ascii', 'ignore') > else: > patch = self.patch > @@ -470,18 +478,11 @@ class CheckOnePatch: > self.stat = mo.group('stat') > self.commit_msg = mo.group('commit_message') > > self.commit_subject = pmail['subject'].replace('\r\n', '') > self.commit_subject = self.commit_subject.replace('\n', '') > - > - pfx_start = self.commit_subject.find('[') > - if pfx_start >= 0: > - pfx_end = self.commit_subject.find(']') > - if pfx_end > pfx_start: > - self.commit_prefix = self.commit_subject[pfx_start + 1 : pfx_end] Since we no longer set self.commit_prefix, and you remove the other references to it in the script? Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> > - self.commit_subject = self.commit_subject[pfx_end + 1 :].lstrip() > - > + self.commit_subject = self.subject_prefix_re.sub('', self.commit_subject, 1) > > class CheckGitCommits: > """Reads patches from git based on the specified git revision range. > > The patches are read from git, and then checked. > -- > 2.6.1.windows.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/3] BaseTools: Update PatchCheck to handle the two [] as prefix 2016-10-17 16:48 ` Jordan Justen @ 2016-10-18 0:53 ` Zhu, Yonghong 0 siblings, 0 replies; 12+ messages in thread From: Zhu, Yonghong @ 2016-10-18 0:53 UTC (permalink / raw) To: Justen, Jordan L, edk2-devel@lists.01.org; +Cc: Gao, Liming, Zhu, Yonghong Thanks for the comment, when I push the patch I will do the update. Best Regards, Zhu Yonghong -----Original Message----- From: Justen, Jordan L Sent: Tuesday, October 18, 2016 12:49 AM To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org Cc: Gao, Liming <liming.gao@intel.com> Subject: Re: [Patch 2/3] BaseTools: Update PatchCheck to handle the two [] as prefix A small (not too important) suggestion for the patch subject might be: BaseTools/PatchCheck.py: Update to handle the two [] as prefix On 2016-10-17 01:28:37, Yonghong Zhu wrote: > The bug is that only remove the first [] when it does the char count, > however sometimes we use [edk2][patch] as prefix, this patch fix this bug. > > Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=113 > Remove blank line after 'Fixes:'. > Cc: Liming Gao <liming.gao@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com> > --- > BaseTools/Scripts/PatchCheck.py | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/BaseTools/Scripts/PatchCheck.py > b/BaseTools/Scripts/PatchCheck.py index 07fca68..05f8f6e 100755 > --- a/BaseTools/Scripts/PatchCheck.py > +++ b/BaseTools/Scripts/PatchCheck.py > @@ -434,10 +434,18 @@ class CheckOnePatch: > [\s\S\r\n]+ > ) > ''', > re.IGNORECASE | re.VERBOSE | re.MULTILINE) > > + subject_prefix_re = \ > + re.compile(r'''^ > + \s* (\[ > + [^\[\]]* # Allow all non-brackets > + \])* \s* > + ''', > + re.VERBOSE) > + > def find_patch_pieces(self): > if sys.version_info < (3, 0): > patch = self.patch.encode('ascii', 'ignore') > else: > patch = self.patch > @@ -470,18 +478,11 @@ class CheckOnePatch: > self.stat = mo.group('stat') > self.commit_msg = mo.group('commit_message') > > self.commit_subject = pmail['subject'].replace('\r\n', '') > self.commit_subject = self.commit_subject.replace('\n', '') > - > - pfx_start = self.commit_subject.find('[') > - if pfx_start >= 0: > - pfx_end = self.commit_subject.find(']') > - if pfx_end > pfx_start: > - self.commit_prefix = self.commit_subject[pfx_start + 1 : pfx_end] Since we no longer set self.commit_prefix, and you remove the other references to it in the script? Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> > - self.commit_subject = self.commit_subject[pfx_end + 1 :].lstrip() > - > + self.commit_subject = self.subject_prefix_re.sub('', > + self.commit_subject, 1) > > class CheckGitCommits: > """Reads patches from git based on the specified git revision range. > > The patches are read from git, and then checked. > -- > 2.6.1.windows.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_* 2016-10-17 8:28 [Patch 0/3] BaseTools: PatchCheck to align with wiki and report error for EFI_D_* Yonghong Zhu 2016-10-17 8:28 ` [Patch 1/3] BaseTools: Update PatchCheck for max length of subject and message line Yonghong Zhu 2016-10-17 8:28 ` [Patch 2/3] BaseTools: Update PatchCheck to handle the two [] as prefix Yonghong Zhu @ 2016-10-17 8:28 ` Yonghong Zhu 2016-10-17 16:58 ` Jordan Justen 2 siblings, 1 reply; 12+ messages in thread From: Yonghong Zhu @ 2016-10-17 8:28 UTC (permalink / raw) To: edk2-devel; +Cc: Liming Gao, Jordan Justen In EDK2, DEBUG_* is recommended to be used instead of EFI_D_*. For new code, they should use DEBUG_* macro. Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=145 Cc: Liming Gao <liming.gao@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com> --- BaseTools/Scripts/PatchCheck.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py index 05f8f6e..3ac0769 100755 --- a/BaseTools/Scripts/PatchCheck.py +++ b/BaseTools/Scripts/PatchCheck.py @@ -337,10 +337,15 @@ class GitDiffCheck: if self.hunk_filename is not None: lines.append('File: ' + self.hunk_filename) lines.append('Line: ' + line) self.error(*lines) + + DEBUG_macro_re = re.compile(r'''^ + \s*DEBUG\s*\(\( + ''', + re.VERBOSE) def check_added_line(self, line): eol = '' for an_eol in self.line_endings: if line.endswith(an_eol): @@ -354,10 +359,13 @@ class GitDiffCheck: line) if '\t' in line: self.added_line_error('Tab character used', line) if len(stripped) < len(line): self.added_line_error('Trailing whitespace found', line) + if self.DEBUG_macro_re.match(line): + if 'EFI_D_' in line: + self.added_line_error('EFI_D_* format is used, recommend to use DEBUG_* format', line) split_diff_re = re.compile(r''' (?P<cmd> ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $ ) -- 2.6.1.windows.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_* 2016-10-17 8:28 ` [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_* Yonghong Zhu @ 2016-10-17 16:58 ` Jordan Justen 2016-10-18 2:02 ` Zhu, Yonghong 0 siblings, 1 reply; 12+ messages in thread From: Jordan Justen @ 2016-10-17 16:58 UTC (permalink / raw) To: Yonghong Zhu, edk2-devel; +Cc: Liming Gao On 2016-10-17 01:28:38, Yonghong Zhu wrote: > In EDK2, DEBUG_* is recommended to be used instead of EFI_D_*. For new > code, they should use DEBUG_* macro. > > Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=145 > Remove blank line. I think you meant 143, not 145. Instead of looking for DEBUG and EFI_D_, I think we should just look for EFI_D_. It is possible that the user might put the EFI_D_ on a separate line. What do you think? Should we add 'warning' support to the script first, and then flag this as a warning rather than an error? (I don't think this is too important, because getting an error from the script will not currently prevent the patch from being merged.) -Jordan > Cc: Liming Gao <liming.gao@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com> > --- > BaseTools/Scripts/PatchCheck.py | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py > index 05f8f6e..3ac0769 100755 > --- a/BaseTools/Scripts/PatchCheck.py > +++ b/BaseTools/Scripts/PatchCheck.py > @@ -337,10 +337,15 @@ class GitDiffCheck: > if self.hunk_filename is not None: > lines.append('File: ' + self.hunk_filename) > lines.append('Line: ' + line) > > self.error(*lines) > + > + DEBUG_macro_re = re.compile(r'''^ > + \s*DEBUG\s*\(\( > + ''', > + re.VERBOSE) > > def check_added_line(self, line): > eol = '' > for an_eol in self.line_endings: > if line.endswith(an_eol): > @@ -354,10 +359,13 @@ class GitDiffCheck: > line) > if ' ' in line: > self.added_line_error('Tab character used', line) > if len(stripped) < len(line): > self.added_line_error('Trailing whitespace found', line) > + if self.DEBUG_macro_re.match(line): > + if 'EFI_D_' in line: > + self.added_line_error('EFI_D_* format is used, recommend to use DEBUG_* format', line) > > split_diff_re = re.compile(r''' > (?P<cmd> > ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $ > ) > -- > 2.6.1.windows.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_* 2016-10-17 16:58 ` Jordan Justen @ 2016-10-18 2:02 ` Zhu, Yonghong 2016-10-18 6:43 ` Jordan Justen 0 siblings, 1 reply; 12+ messages in thread From: Zhu, Yonghong @ 2016-10-18 2:02 UTC (permalink / raw) To: Justen, Jordan L, edk2-devel@lists.01.org; +Cc: Gao, Liming, Zhu, Yonghong Hi Jordan, We request to report error since the DEBUG_* is our recommend coding style and EFI_D_ is obsolete. Looking for DEBUG and EFI_D_ since only look for EFI_D_ may cause some False error be reported. And I agree that user might put EFI_D_ on a separate line, however I scanned our current edk2 code, no such usage. So I'd like to add this support after we meet the real case. Best Regards, Zhu Yonghong -----Original Message----- From: Justen, Jordan L Sent: Tuesday, October 18, 2016 12:58 AM To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org Cc: Gao, Liming <liming.gao@intel.com> Subject: Re: [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_* On 2016-10-17 01:28:38, Yonghong Zhu wrote: > In EDK2, DEBUG_* is recommended to be used instead of EFI_D_*. For new > code, they should use DEBUG_* macro. > > Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=145 > Remove blank line. I think you meant 143, not 145. Instead of looking for DEBUG and EFI_D_, I think we should just look for EFI_D_. It is possible that the user might put the EFI_D_ on a separate line. What do you think? Should we add 'warning' support to the script first, and then flag this as a warning rather than an error? (I don't think this is too important, because getting an error from the script will not currently prevent the patch from being merged.) -Jordan > Cc: Liming Gao <liming.gao@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com> > --- > BaseTools/Scripts/PatchCheck.py | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/BaseTools/Scripts/PatchCheck.py > b/BaseTools/Scripts/PatchCheck.py index 05f8f6e..3ac0769 100755 > --- a/BaseTools/Scripts/PatchCheck.py > +++ b/BaseTools/Scripts/PatchCheck.py > @@ -337,10 +337,15 @@ class GitDiffCheck: > if self.hunk_filename is not None: > lines.append('File: ' + self.hunk_filename) > lines.append('Line: ' + line) > > self.error(*lines) > + > + DEBUG_macro_re = re.compile(r'''^ > + \s*DEBUG\s*\(\( > + ''', > + re.VERBOSE) > > def check_added_line(self, line): > eol = '' > for an_eol in self.line_endings: > if line.endswith(an_eol): > @@ -354,10 +359,13 @@ class GitDiffCheck: > line) > if ' ' in line: > self.added_line_error('Tab character used', line) > if len(stripped) < len(line): > self.added_line_error('Trailing whitespace found', line) > + if self.DEBUG_macro_re.match(line): > + if 'EFI_D_' in line: > + self.added_line_error('EFI_D_* format is used, > + recommend to use DEBUG_* format', line) > > split_diff_re = re.compile(r''' > (?P<cmd> > ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $ > ) > -- > 2.6.1.windows.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_* 2016-10-18 2:02 ` Zhu, Yonghong @ 2016-10-18 6:43 ` Jordan Justen 2016-10-18 10:22 ` Zhu, Yonghong 0 siblings, 1 reply; 12+ messages in thread From: Jordan Justen @ 2016-10-18 6:43 UTC (permalink / raw) To: Zhu, Yonghong, edk2-devel@lists.01.org; +Cc: Gao, Liming, Zhu, Yonghong On 2016-10-17 19:02:57, Zhu, Yonghong wrote: > Hi Jordan, > > We request to report error since the DEBUG_* is our recommend coding > style and EFI_D_ is obsolete. Looking for DEBUG and EFI_D_ since > only look for EFI_D_ may cause some False error be reported. And I > agree that user might put EFI_D_ on a separate line, however I > scanned our current edk2 code, no such usage. So I'd like to add > this support after we meet the real case. > In that case, how about making the regex detect both? old_debug_re = \ re.compile(r''' DEBUG \s? \( \s? \( \s* EFI_D_ ([A-Z_]+) ''', re.VERBOSE) Then you can use something like: mo = self.old_debug_re.search(...) if mo is not None: self.added_line_error('EFI_D_' + mo.group(1) + ' was used, ' 'but DEBUG_' + mo.group(1) + ' is now recommended', line) -Jordan > > -----Original Message----- > From: Justen, Jordan L > Sent: Tuesday, October 18, 2016 12:58 AM > To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org > Cc: Gao, Liming <liming.gao@intel.com> > Subject: Re: [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_* > > On 2016-10-17 01:28:38, Yonghong Zhu wrote: > > In EDK2, DEBUG_* is recommended to be used instead of EFI_D_*. For new > > code, they should use DEBUG_* macro. > > > > Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=145 > > > > Remove blank line. I think you meant 143, not 145. > > Instead of looking for DEBUG and EFI_D_, I think we should just look > for EFI_D_. It is possible that the user might put the EFI_D_ on a > separate line. What do you think? > > Should we add 'warning' support to the script first, and then flag > this as a warning rather than an error? (I don't think this is too > important, because getting an error from the script will not > currently prevent the patch from being merged.) > > -Jordan > > > Cc: Liming Gao <liming.gao@intel.com> > > Cc: Jordan Justen <jordan.l.justen@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com> > > --- > > BaseTools/Scripts/PatchCheck.py | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/BaseTools/Scripts/PatchCheck.py > > b/BaseTools/Scripts/PatchCheck.py index 05f8f6e..3ac0769 100755 > > --- a/BaseTools/Scripts/PatchCheck.py > > +++ b/BaseTools/Scripts/PatchCheck.py > > @@ -337,10 +337,15 @@ class GitDiffCheck: > > if self.hunk_filename is not None: > > lines.append('File: ' + self.hunk_filename) > > lines.append('Line: ' + line) > > > > self.error(*lines) > > + > > + DEBUG_macro_re = re.compile(r'''^ > > + \s*DEBUG\s*\(\( > > + ''', > > + re.VERBOSE) > > > > def check_added_line(self, line): > > eol = '' > > for an_eol in self.line_endings: > > if line.endswith(an_eol): > > @@ -354,10 +359,13 @@ class GitDiffCheck: > > line) > > if ' ' in line: > > self.added_line_error('Tab character used', line) > > if len(stripped) < len(line): > > self.added_line_error('Trailing whitespace found', line) > > + if self.DEBUG_macro_re.match(line): > > + if 'EFI_D_' in line: > > + self.added_line_error('EFI_D_* format is used, > > + recommend to use DEBUG_* format', line) > > > > split_diff_re = re.compile(r''' > > (?P<cmd> > > ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $ > > ) > > -- > > 2.6.1.windows.1 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_* 2016-10-18 6:43 ` Jordan Justen @ 2016-10-18 10:22 ` Zhu, Yonghong 0 siblings, 0 replies; 12+ messages in thread From: Zhu, Yonghong @ 2016-10-18 10:22 UTC (permalink / raw) To: Justen, Jordan L, edk2-devel@lists.01.org; +Cc: Gao, Liming, Zhu, Yonghong Thanks Jordan, I sent a V2. I update a little on the re.compile to cover the case that EFI_D_* in the middle of the DEBUG string, and may have multiple space between DEBUG and ( character. Best Regards, Zhu Yonghong -----Original Message----- From: Justen, Jordan L Sent: Tuesday, October 18, 2016 2:43 PM To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org Cc: Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com> Subject: RE: [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_* On 2016-10-17 19:02:57, Zhu, Yonghong wrote: > Hi Jordan, > > We request to report error since the DEBUG_* is our recommend coding > style and EFI_D_ is obsolete. Looking for DEBUG and EFI_D_ since only > look for EFI_D_ may cause some False error be reported. And I agree > that user might put EFI_D_ on a separate line, however I scanned our > current edk2 code, no such usage. So I'd like to add this support > after we meet the real case. > In that case, how about making the regex detect both? old_debug_re = \ re.compile(r''' DEBUG \s? \( \s? \( \s* EFI_D_ ([A-Z_]+) ''', re.VERBOSE) Then you can use something like: mo = self.old_debug_re.search(...) if mo is not None: self.added_line_error('EFI_D_' + mo.group(1) + ' was used, ' 'but DEBUG_' + mo.group(1) + ' is now recommended', line) -Jordan > > -----Original Message----- > From: Justen, Jordan L > Sent: Tuesday, October 18, 2016 12:58 AM > To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org > Cc: Gao, Liming <liming.gao@intel.com> > Subject: Re: [Patch 3/3] BaseTools: Update PatchCheck to report error > for EFI_D_* > > On 2016-10-17 01:28:38, Yonghong Zhu wrote: > > In EDK2, DEBUG_* is recommended to be used instead of EFI_D_*. For > > new code, they should use DEBUG_* macro. > > > > Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=145 > > > > Remove blank line. I think you meant 143, not 145. > > Instead of looking for DEBUG and EFI_D_, I think we should just look > for EFI_D_. It is possible that the user might put the EFI_D_ on a > separate line. What do you think? > > Should we add 'warning' support to the script first, and then flag > this as a warning rather than an error? (I don't think this is too > important, because getting an error from the script will not currently > prevent the patch from being merged.) > > -Jordan > > > Cc: Liming Gao <liming.gao@intel.com> > > Cc: Jordan Justen <jordan.l.justen@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com> > > --- > > BaseTools/Scripts/PatchCheck.py | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/BaseTools/Scripts/PatchCheck.py > > b/BaseTools/Scripts/PatchCheck.py index 05f8f6e..3ac0769 100755 > > --- a/BaseTools/Scripts/PatchCheck.py > > +++ b/BaseTools/Scripts/PatchCheck.py > > @@ -337,10 +337,15 @@ class GitDiffCheck: > > if self.hunk_filename is not None: > > lines.append('File: ' + self.hunk_filename) > > lines.append('Line: ' + line) > > > > self.error(*lines) > > + > > + DEBUG_macro_re = re.compile(r'''^ > > + \s*DEBUG\s*\(\( > > + ''', > > + re.VERBOSE) > > > > def check_added_line(self, line): > > eol = '' > > for an_eol in self.line_endings: > > if line.endswith(an_eol): > > @@ -354,10 +359,13 @@ class GitDiffCheck: > > line) > > if ' ' in line: > > self.added_line_error('Tab character used', line) > > if len(stripped) < len(line): > > self.added_line_error('Trailing whitespace found', > > line) > > + if self.DEBUG_macro_re.match(line): > > + if 'EFI_D_' in line: > > + self.added_line_error('EFI_D_* format is used, > > + recommend to use DEBUG_* format', line) > > > > split_diff_re = re.compile(r''' > > (?P<cmd> > > ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $ > > ) > > -- > > 2.6.1.windows.1 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-10-18 10:22 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-17 8:28 [Patch 0/3] BaseTools: PatchCheck to align with wiki and report error for EFI_D_* Yonghong Zhu 2016-10-17 8:28 ` [Patch 1/3] BaseTools: Update PatchCheck for max length of subject and message line Yonghong Zhu 2016-10-17 16:40 ` Jordan Justen 2016-10-18 0:49 ` Zhu, Yonghong 2016-10-17 8:28 ` [Patch 2/3] BaseTools: Update PatchCheck to handle the two [] as prefix Yonghong Zhu 2016-10-17 16:48 ` Jordan Justen 2016-10-18 0:53 ` Zhu, Yonghong 2016-10-17 8:28 ` [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_* Yonghong Zhu 2016-10-17 16:58 ` Jordan Justen 2016-10-18 2:02 ` Zhu, Yonghong 2016-10-18 6:43 ` Jordan Justen 2016-10-18 10:22 ` Zhu, Yonghong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox