public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci
@ 2020-06-03  8:48 Zhang, Shenglei
  2020-06-03  8:48 ` [PATCH v2 1/5] BaseTools:ECC needs to update the contents of CParser4 Zhang, Shenglei
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Zhang, Shenglei @ 2020-06-03  8:48 UTC (permalink / raw)
  To: devel; +Cc: Bob Feng, Bret Barkelew, Michael D Kinney, Liming Gao,
	Sean Brogan

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2606
As planed we will enable Ecc check for edk2 on open ci. And they are
ready now, but these are V2 series. So I expect that contributors in
edk2 community can try using this script when reviewing. And I appreciate
receiving feedback and comments if someone find errors or false positive
issues.

I created a pipline of EccCheck for my forked edk2.
https://dev.azure.com/shengleizhang/shengleizhang/_build?definitionId=10&_a=summary

The patch series are big, so the commits are also pushed into my forked tree.
https://github.com/shenglei10/edk2/commits/ECC

Patches
1/5: This is a patch to enable python 3.8 for Ecc. It is a tool issues not
     a pipline or script issue. But it is listed here for people willing
     to try this tool.
2/5: EccCheck.py is a tool to report Ecc issues for commits. It can be run
     on azure servers for open ci, or locally. Its usage is like
     PatchCheck.py.
3/5: It's a lib necessary for py3 to run Ecc on azure servers. For local
     use, we need to type command
     "py -3 -m pip install antlr4-python3-runtime" first.
4/5: Windows-EccCheck.yml is a yaml file to configure the newly added
     pipline. The azure uses this to create a pipline.
5/5: We consider some cases that will report out Ecc issues but they won't
     be fixed, like submodule and industry standard related things. So we
     add two configuration fields "Exception" and "IgnoreFiles" for people
     to use. The patch is a example and the contents in the fields will be
     empty in final version.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>

v2: Update 2/5, fix the bug that the script can't hanlde multiple commits.

Fan, Zhiju (1):
  BaseTools:ECC needs to update the contents of CParser4

Shenglei Zhang (4):
  BaseTools/Scripts: Add EccCheck.py
  pip-requirements.txt: Add Ecc required lib
  .azurepiplines: Add a pipline to check ECC issues for commits
  MdeModulePkg/MdeModulePkg.ci.yaml: Add configuration for Ecc check

 .azurepipelines/Windows-EccCheck.yml          |  38 ++
 BaseTools/Scripts/EccCheck.py                 | 433 ++++++++++++++++++
 .../Source/Python/Ecc/CParser4/CLexer.py      |   6 +-
 .../Source/Python/Ecc/CParser4/CListener.py   |   4 +-
 .../Source/Python/Ecc/CParser4/CParser.py     |  38 +-
 MdeModulePkg/MdeModulePkg.ci.yaml             |   8 +
 pip-requirements.txt                          |   1 +
 7 files changed, 505 insertions(+), 23 deletions(-)
 create mode 100644 .azurepipelines/Windows-EccCheck.yml
 create mode 100644 BaseTools/Scripts/EccCheck.py

-- 
2.18.0.windows.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/5] BaseTools:ECC needs to update the contents of CParser4
  2020-06-03  8:48 [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci Zhang, Shenglei
@ 2020-06-03  8:48 ` Zhang, Shenglei
  2020-06-05  4:38   ` [edk2-devel] " Yuwei Chen
  2020-06-03  8:48 ` [PATCH v2 2/5] BaseTools/Scripts: Add EccCheck.py Zhang, Shenglei
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 12+ messages in thread
From: Zhang, Shenglei @ 2020-06-03  8:48 UTC (permalink / raw)
  To: devel; +Cc: Fan, Zhiju, Bob Feng, Liming Gao

From: "Fan, Zhiju" <zhijux.fan@intel.com>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2676

Because Ecc relies on the antlr extension package,
When the antlr version is updated, we need to change the code in it.
Currently, if you use the latest version antlr4.8, Ecc will fail

We will update the version to support the use of antlr4.8 and all
previous versions will have errors.This can be resolved by installing
the antlr4.8 version.
Installation method: pip install antlr4-python3-runtime==4.8

This patch is going to fixed this 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>
---
 .../Source/Python/Ecc/CParser4/CLexer.py      |  6 +--
 .../Source/Python/Ecc/CParser4/CListener.py   |  4 +-
 .../Source/Python/Ecc/CParser4/CParser.py     | 38 ++++++++++---------
 3 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/BaseTools/Source/Python/Ecc/CParser4/CLexer.py b/BaseTools/Source/Python/Ecc/CParser4/CLexer.py
index a2cc5bf56e66..40e2afbf1a1f 100644
--- a/BaseTools/Source/Python/Ecc/CParser4/CLexer.py
+++ b/BaseTools/Source/Python/Ecc/CParser4/CLexer.py
@@ -1,4 +1,4 @@
-# Generated from C.g4 by ANTLR 4.7.1
+# Generated from C.g4 by ANTLR 4.8
 from antlr4 import *
 from io import StringIO
 from typing.io import TextIO
@@ -12,7 +12,7 @@ import sys
 # This file is generated by running:
 # java org.antlr.Tool C.g
 #
-# Copyright (c) 2009 - 2010, Intel Corporation  All rights reserved.
+# Copyright (c) 2009 - 2020, Intel Corporation  All rights reserved.
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -584,7 +584,7 @@ class CLexer(Lexer):
     # @param  output= sys.stdout Type: TextIO
     def __init__(self,input=None,output= sys.stdout):
         super().__init__(input, output)
-        self.checkVersion("4.7.1")
+        self.checkVersion("4.8")
         self._interp = LexerATNSimulator(self, self.atn, self.decisionsToDFA, PredictionContextCache())
         self._actions = None
         self._predicates = None
diff --git a/BaseTools/Source/Python/Ecc/CParser4/CListener.py b/BaseTools/Source/Python/Ecc/CParser4/CListener.py
index bb4351d9249a..ba7e70892680 100644
--- a/BaseTools/Source/Python/Ecc/CParser4/CListener.py
+++ b/BaseTools/Source/Python/Ecc/CParser4/CListener.py
@@ -1,4 +1,4 @@
-# Generated from C.g4 by ANTLR 4.7.1
+# Generated from C.g4 by ANTLR 4.8
 from antlr4 import *
 if __name__ is not None and "." in __name__:
     from .CParser import CParser
@@ -12,7 +12,7 @@ else:
 # This file is generated by running:
 # java org.antlr.Tool C.g
 #
-# Copyright (c) 2009 - 2010, Intel Corporation  All rights reserved.
+# Copyright (c) 2009 - 2020, Intel Corporation  All rights reserved.
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
diff --git a/BaseTools/Source/Python/Ecc/CParser4/CParser.py b/BaseTools/Source/Python/Ecc/CParser4/CParser.py
index 31d23d55aa57..30d70a74e669 100644
--- a/BaseTools/Source/Python/Ecc/CParser4/CParser.py
+++ b/BaseTools/Source/Python/Ecc/CParser4/CParser.py
@@ -1,10 +1,12 @@
-# Generated from C.g4 by ANTLR 4.7.1
+# Generated from C.g4 by ANTLR 4.8
 # encoding: utf-8
 from antlr4 import *
 from io import StringIO
-from typing.io import TextIO
 import sys
-
+if sys.version_info[1] > 5:
+    from typing import TextIO
+else:
+    from typing.io import TextIO
 
 ## @file
 # The file defines the parser for C source files.
@@ -13,7 +15,7 @@ import sys
 # This file is generated by running:
 # java org.antlr.Tool C.g
 #
-# Copyright (c) 2009 - 2010, Intel Corporation  All rights reserved.
+# Copyright (c) 2009 - 2020, Intel Corporation  All rights reserved.
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -739,7 +741,7 @@ class CParser ( Parser ):
     # @param  output= sys.stdout Type: TextIO
     def __init__(self,input,output= sys.stdout):
         super().__init__(input, output)
-        self.checkVersion("4.7.1")
+        self.checkVersion("4.8")
         self._interp = ParserATNSimulator(self, self.atn, self.decisionsToDFA, self.sharedContextCache)
         self._predicates = None
 
@@ -1062,10 +1064,10 @@ class CParser ( Parser ):
 
 
             if localctx.d != None:
-                ModifierText = (None if localctx._declaration_specifiers is None else self._input.getText((localctx._declaration_specifiers.start,localctx._declaration_specifiers.stop)))
+                ModifierText = (None if localctx._declaration_specifiers is None else self._input.getText(localctx._declaration_specifiers.start,localctx._declaration_specifiers.stop))
             else:
                 ModifierText = ''
-            DeclText = (None if localctx._declarator is None else self._input.getText((localctx._declarator.start,localctx._declarator.stop)))
+            DeclText = (None if localctx._declarator is None else self._input.getText(localctx._declarator.start,localctx._declarator.stop))
             DeclLine = (None if localctx._declarator is None else localctx._declarator.start).line
             DeclOffset = (None if localctx._declarator is None else localctx._declarator.start).column
             if localctx.a != None:
@@ -1245,9 +1247,9 @@ class CParser ( Parser ):
                 localctx.d = self.match(CParser.T__1)
 
                 if localctx.b is not None:
-                    self.StoreTypedefDefinition(localctx.a.line, localctx.a.column, (0 if localctx.d is None else localctx.d.line), localctx.d.column, (None if localctx.b is None else self._input.getText((localctx.b.start,localctx.b.stop))), (None if localctx.c is None else self._input.getText((localctx.c.start,localctx.c.stop))))
+                    self.StoreTypedefDefinition(localctx.a.line, localctx.a.column, (0 if localctx.d is None else localctx.d.line), localctx.d.column, (None if localctx.b is None else self._input.getText(localctx.b.start,localctx.b.stop)), (None if localctx.c is None else self._input.getText(localctx.c.start,localctx.c.stop)))
                 else:
-                    self.StoreTypedefDefinition(localctx.a.line, localctx.a.column, (0 if localctx.d is None else localctx.d.line), localctx.d.column, '', (None if localctx.c is None else self._input.getText((localctx.c.start,localctx.c.stop))))
+                    self.StoreTypedefDefinition(localctx.a.line, localctx.a.column, (0 if localctx.d is None else localctx.d.line), localctx.d.column, '', (None if localctx.c is None else self._input.getText(localctx.c.start,localctx.c.stop)))
 
                 pass
             elif token in [CParser.T__5, CParser.T__6, CParser.T__7, CParser.T__8, CParser.T__9, CParser.T__10, CParser.T__11, CParser.T__12, CParser.T__13, CParser.T__14, CParser.T__15, CParser.T__16, CParser.T__17, CParser.T__18, CParser.T__20, CParser.T__21, CParser.T__23, CParser.T__24, CParser.T__25, CParser.T__26, CParser.T__27, CParser.T__28, CParser.T__29, CParser.T__30, CParser.T__31, CParser.T__32, CParser.T__33, CParser.T__34, CParser.T__35, CParser.T__36, CParser.IDENTIFIER]:
@@ -1266,7 +1268,7 @@ class CParser ( Parser ):
                 localctx.e = self.match(CParser.T__1)
 
                 if localctx.t is not None:
-                    self.StoreVariableDeclaration((None if localctx.s is None else localctx.s.start).line, (None if localctx.s is None else localctx.s.start).column, (None if localctx.t is None else localctx.t.start).line, (None if localctx.t is None else localctx.t.start).column, (None if localctx.s is None else self._input.getText((localctx.s.start,localctx.s.stop))), (None if localctx.t is None else self._input.getText((localctx.t.start,localctx.t.stop))))
+                    self.StoreVariableDeclaration((None if localctx.s is None else localctx.s.start).line, (None if localctx.s is None else localctx.s.start).column, (None if localctx.t is None else localctx.t.start).line, (None if localctx.t is None else localctx.t.start).column, (None if localctx.s is None else self._input.getText(localctx.s.start,localctx.s.stop)), (None if localctx.t is None else self._input.getText(localctx.t.start,localctx.t.stop)))
 
                 pass
             else:
@@ -1568,7 +1570,7 @@ class CParser ( Parser ):
                 localctx.s = self.struct_or_union_specifier()
 
                 if localctx.s.stop is not None:
-                    self.StoreStructUnionDefinition((None if localctx.s is None else localctx.s.start).line, (None if localctx.s is None else localctx.s.start).column, (None if localctx.s is None else localctx.s.stop).line, (None if localctx.s is None else localctx.s.stop).column, (None if localctx.s is None else self._input.getText((localctx.s.start,localctx.s.stop))))
+                    self.StoreStructUnionDefinition((None if localctx.s is None else localctx.s.start).line, (None if localctx.s is None else localctx.s.start).column, (None if localctx.s is None else localctx.s.stop).line, (None if localctx.s is None else localctx.s.stop).column, (None if localctx.s is None else self._input.getText(localctx.s.start,localctx.s.stop)))
 
                 pass
 
@@ -1578,7 +1580,7 @@ class CParser ( Parser ):
                 localctx.e = self.enum_specifier()
 
                 if localctx.e.stop is not None:
-                    self.StoreEnumerationDefinition((None if localctx.e is None else localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column, (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is None else localctx.e.stop).column, (None if localctx.e is None else self._input.getText((localctx.e.start,localctx.e.stop))))
+                    self.StoreEnumerationDefinition((None if localctx.e is None else localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column, (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is None else localctx.e.stop).column, (None if localctx.e is None else self._input.getText(localctx.e.start,localctx.e.stop)))
 
                 pass
 
@@ -4022,7 +4024,7 @@ class CParser ( Parser ):
             self.enterOuterAlt(localctx, 1)
             self.state = 569
             localctx.p = self.primary_expression()
-            self.FuncCallText += (None if localctx.p is None else self._input.getText((localctx.p.start,localctx.p.stop)))
+            self.FuncCallText += (None if localctx.p is None else self._input.getText(localctx.p.start,localctx.p.stop))
             self.state = 600
             self._errHandler.sync(self)
             _alt = self._interp.adaptivePredict(self._input,73,self._ctx)
@@ -4055,7 +4057,7 @@ class CParser ( Parser ):
                         localctx.c = self.argument_expression_list()
                         self.state = 580
                         localctx.b = self.match(CParser.T__38)
-                        self.StoreFunctionCalling((None if localctx.p is None else localctx.p.start).line, (None if localctx.p is None else localctx.p.start).column, (0 if localctx.b is None else localctx.b.line), localctx.b.column, self.FuncCallText, (None if localctx.c is None else self._input.getText((localctx.c.start,localctx.c.stop))))
+                        self.StoreFunctionCalling((None if localctx.p is None else localctx.p.start).line, (None if localctx.p is None else localctx.p.start).column, (0 if localctx.b is None else localctx.b.line), localctx.b.column, self.FuncCallText, (None if localctx.c is None else self._input.getText(localctx.c.start,localctx.c.stop)))
                         pass
 
                     elif la_ == 4:
@@ -4770,7 +4772,7 @@ class CParser ( Parser ):
                 self.match(CParser.T__22)
                 self.state = 674
                 self.conditional_expression()
-                self.StorePredicateExpression((None if localctx.e is None else localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column, (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is None else localctx.e.stop).column, (None if localctx.e is None else self._input.getText((localctx.e.start,localctx.e.stop))))
+                self.StorePredicateExpression((None if localctx.e is None else localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column, (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is None else localctx.e.stop).column, (None if localctx.e is None else self._input.getText(localctx.e.start,localctx.e.stop)))
 
 
         except RecognitionException as re:
@@ -6053,7 +6055,7 @@ class CParser ( Parser ):
                 localctx.e = self.expression()
                 self.state = 845
                 self.match(CParser.T__38)
-                self.StorePredicateExpression((None if localctx.e is None else localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column, (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is None else localctx.e.stop).column, (None if localctx.e is None else self._input.getText((localctx.e.start,localctx.e.stop))))
+                self.StorePredicateExpression((None if localctx.e is None else localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column, (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is None else localctx.e.stop).column, (None if localctx.e is None else self._input.getText(localctx.e.start,localctx.e.stop)))
                 self.state = 847
                 self.statement()
                 self.state = 850
@@ -6144,7 +6146,7 @@ class CParser ( Parser ):
                 self.match(CParser.T__38)
                 self.state = 864
                 self.statement()
-                self.StorePredicateExpression((None if localctx.e is None else localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column, (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is None else localctx.e.stop).column, (None if localctx.e is None else self._input.getText((localctx.e.start,localctx.e.stop))))
+                self.StorePredicateExpression((None if localctx.e is None else localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column, (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is None else localctx.e.stop).column, (None if localctx.e is None else self._input.getText(localctx.e.start,localctx.e.stop)))
                 pass
             elif token in [CParser.T__87]:
                 self.enterOuterAlt(localctx, 2)
@@ -6162,7 +6164,7 @@ class CParser ( Parser ):
                 self.match(CParser.T__38)
                 self.state = 873
                 self.match(CParser.T__1)
-                self.StorePredicateExpression((None if localctx.e is None else localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column, (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is None else localctx.e.stop).column, (None if localctx.e is None else self._input.getText((localctx.e.start,localctx.e.stop))))
+                self.StorePredicateExpression((None if localctx.e is None else localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column, (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is None else localctx.e.stop).column, (None if localctx.e is None else self._input.getText(localctx.e.start,localctx.e.stop)))
                 pass
             else:
                 raise NoViableAltException(self)
-- 
2.18.0.windows.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/5] BaseTools/Scripts: Add EccCheck.py
  2020-06-03  8:48 [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci Zhang, Shenglei
  2020-06-03  8:48 ` [PATCH v2 1/5] BaseTools:ECC needs to update the contents of CParser4 Zhang, Shenglei
@ 2020-06-03  8:48 ` Zhang, Shenglei
  2020-06-03  8:48 ` [PATCH v2 3/5] pip-requirements.txt: Add Ecc required lib Zhang, Shenglei
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Zhang, Shenglei @ 2020-06-03  8:48 UTC (permalink / raw)
  To: devel; +Cc: Bob Feng, Liming Gao

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2606
EccCheck.py is a tool to report Ecc issues for commits, which will
be run on open ci.
But note not each kind of issue could be reported out.
It can only handle the issues, whose line number in CSV report
accurately map with their code in source code files. And comment
issues can also be handled.

Its usage is similar to PatchCheck.py. Type EccCheck.py -h and then
learn how to use it.

If a patch passes EccCheck, "Ecc Pass" will show up.
Otherwise, "Ecc error detected" alerts the users and the details
are also presented.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---

v2: Update 2/5, fix the bug that the script can't hanlde multiple commits.

 BaseTools/Scripts/EccCheck.py | 433 ++++++++++++++++++++++++++++++++++
 1 file changed, 433 insertions(+)
 create mode 100644 BaseTools/Scripts/EccCheck.py

diff --git a/BaseTools/Scripts/EccCheck.py b/BaseTools/Scripts/EccCheck.py
new file mode 100644
index 000000000000..034a9965a79b
--- /dev/null
+++ b/BaseTools/Scripts/EccCheck.py
@@ -0,0 +1,433 @@
+## @file
+#  Check a patch for various format issues
+#
+#  Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+
+import os
+import re
+import csv
+import subprocess
+import argparse
+import sys
+import yaml
+import xml.dom.minidom
+
+__copyright__ = "Copyright (c) 2020, Intel Corporation  All rights reserved."
+ReModifyFile = re.compile(r'[B-Q,S-Z]+[\d]*\t(.*?)\n')
+FindModifyFile = re.compile(r'\+\+\+ b\/(.*)')
+LineScopePattern = (r'@@ -\d*\,*\d* \+\d*\,*\d* @@.*')
+LineNumRange = re.compile(r'@@ -\d*\,*\d* \+(\d*)\,*(\d*) @@.*')
+
+EnvList = os.environ
+GlobalSymbol = {}
+
+def AppendException(ExceptionList, ExceptionFile):
+    error_code_list = ExceptionList[::2]
+    keyword_list = ExceptionList[1::2]
+    domTree = xml.dom.minidom.parse(ExceptionFile)
+    rootNode = domTree.documentElement
+    for error_code, keyword in zip(error_code_list, keyword_list):
+        customer_node = domTree.createElement("Exception")
+        keyword_node = domTree.createElement("KeyWord")
+        keyword_node_text_value = domTree.createTextNode(keyword)
+        keyword_node.appendChild(keyword_node_text_value)
+        customer_node.appendChild(keyword_node)
+        error_code_node = domTree.createElement("ErrorID")
+        error_code_text_value = domTree.createTextNode(error_code)
+        error_code_node.appendChild(error_code_text_value)
+        customer_node.appendChild(error_code_node)
+        rootNode.appendChild(customer_node)
+
+    with open(ExceptionFile, 'w') as f:
+        domTree.writexml(f,indent='',addindent='',newl='\n',encoding='UTF-8')
+
+def GetPkgList(EnvList):
+    WORKDIR = EnvList['WORKDIR']
+    dirs = os.listdir(WORKDIR)
+    PkgList = []
+    for directory in dirs:
+        if directory.endswith('Pkg'):
+            PkgList.append(directory)
+    return PkgList
+
+def GenerateEccReport(EnvList, ModifyDirList, EccDiffRange):
+    IsECCNeed = False
+    IsTrue = True
+    PkgList = GetPkgList(EnvList)
+
+    for Line in ModifyDirList:
+        print('echo Run ECC tool for the commit in %s' % Line)
+        GlobalSymbol['GenerateEccReport'] = True
+        for Pkg in PkgList:
+            if Pkg in Line:
+                IsECCNeed = True
+                Ecc_cmd = ["ecc", "-c", "%WORKDIR%\BaseTools\Source\Python\Ecc\config.ini",
+                                  "-e", "%WORKDIR%\BaseTools\Source\Python\Ecc\exception.xml",
+                                  "-t", "%WORKDIR%\{}".format(Line),
+                                  "-r", "%WORKDIR%\Ecc.csv"]
+                _, _, result, return_code = ExecuteScript(Ecc_cmd, EnvList, shell=True)
+                if return_code != 0:
+                    IsTrue = False
+                    break
+
+        if not IsTrue:
+            print('Fail to run ECC tool')
+            GlobalSymbol['SCRIPT_ERROR'] = True
+            EndDelFile(EnvList)
+
+        if GlobalSymbol.get('GenerateEccReport'):
+            ParseEccReport(EnvList, EccDiffRange)
+        else:
+            print("Patch check tool or ECC tool don't detect error")
+
+    if IsECCNeed:
+        revert_cmd = ["git", "checkout", "--", "%WORKDIR%\BaseTools\Source\Python\Ecc\exception.xml"]
+        _, _, result, return_code = ExecuteScript(revert_cmd, EnvList, shell=True)
+    else:
+        print("Doesn't need run ECC check")
+        return
+
+def ParseEccReport(EnvList, EccDiffRange):
+    WorkDir = EnvList['WORKDIR']
+    EccLog = os.path.join(WorkDir, "Ecc.log")
+    EccsCsv = "Ecc.csv"
+    FileList = os.listdir(WorkDir)
+    rowLines = []
+    if EccsCsv in FileList:
+        with open(EccsCsv) as Csv:
+            reader = csv.reader(Csv)
+            for row in reader:
+                for ModifyFile in EccDiffRange:
+                    if ModifyFile in row[3]:
+                        for i in EccDiffRange[ModifyFile]:
+                            LineNo = int(row[4])
+                            if i[0] <= LineNo <= i[1]:
+                                row[0] = '\nEFI coding style error'
+                                row[1] = 'Error code: ' + row[1]
+                                row[3] = 'File: ' + row[3]
+                                row[4] = 'Line number: ' + row[4]
+                                rowLine = '\n  *'.join(row)
+                                rowLines.append(rowLine)
+                                break
+                        break
+    if rowLines:
+        GlobalSymbol['ECC_PASS'] = False
+
+    with open(EccLog, 'a') as EccFile:
+            AllLine = '\n'.join(rowLines)
+            AllLine = AllLine + '\n'
+            EccFile.writelines(AllLine)
+
+def RemoveFile(File):
+    if os.path.exists(File):
+        os.remove(File)
+
+def ExecuteScript(command, env_variables, collect_env=False,
+                  enable_std_pipe=False, shell=True):
+    """launches a process that executes a script/shell command passed to it
+
+        :param command: The command/script with its commandline
+            arguments to be executed
+        :type command:  List:String
+        :param env_variables: Environment variables passed to the process
+        :type env_variables: String
+        :param collect_env: Enables the collection of evironment variables
+            when process execution is done
+        :type collect_env: Boolean
+        :param enable_std_pipe: Enables process out to be piped to
+        :type enable_std_pipe: String
+        :returns: a tuple of std_out, stderr , environment variables,
+            return code
+        :rtype: Tuple: (std_out, stderr , enVar, return_code)
+    """
+
+    env_marker = '-----env-----'
+    env = {}
+    kwarg = {"env": env_variables,
+             "universal_newlines": True,
+             "shell": shell,
+             "cwd": env_variables["WORKSPACE"]}
+
+    if enable_std_pipe or collect_env:
+        kwarg["stdout"] = subprocess.PIPE
+        kwarg["stderr"] = subprocess.PIPE
+
+    if collect_env:
+        # get the binary that prints environment variables based on os
+        if os.name == 'nt':
+            get_var_command = "set"
+        else:
+            get_var_command = "env"
+        # modify the command to print the environment variables
+        if isinstance(command, list):
+            command += ["&&", "echo", env_marker, "&&",
+                        get_var_command, "&&", "echo", env_marker]
+        else:
+            command += " " + " ".join(["&&", "echo", env_marker,
+                                       "&&", get_var_command,
+                                       "&&", "echo", env_marker])
+
+    # execute the command
+    execute = subprocess.Popen(command, **kwarg)
+    std_out, stderr = execute.communicate()
+    code = execute.returncode
+
+    # wait for process to be done
+    execute.wait()
+
+    # if collect enviroment variables
+    if collect_env:
+        # get the new environment variables
+        std_out, env = GetEnvironmentVariables(std_out, env_marker)
+    return (std_out, stderr, env, code)
+
+def GetEnvironmentVariables(std_out_str, marker):
+    """Gets the environment variables from a process
+
+        :param std_out_str: The std_out pipe
+        :type std_out_str: String
+        :param marker: A begining and end mark of environment
+            variables printed to std_out
+        :type marker: String
+        :returns: The environment variables read from the process' std_out pipe
+        :rtype: Tuple
+    """
+    start_env_update = False
+    environment_vars = {}
+    out_put = ""
+    for line in std_out_str.split("\n"):
+        if start_env_update and len(line.split("=")) == 2:
+            key, value = line.split("=")
+            environment_vars[key] = value
+        else:
+            out_put += "\n" + line.replace(marker, "")
+
+        if marker in line:
+            if start_env_update:
+                start_env_update = False
+            else:
+                start_env_update = True
+    return (out_put, environment_vars)
+
+def EndDelFile(EnvList):
+    WORKDIR = EnvList['WORKDIR']
+    ModifyFileListLog = os.path.join(WORKDIR, 'PatchModifyFiles.log')
+    RemoveFile(ModifyFileListLog)
+    PatchLog = os.path.join(WORKDIR, 'PatchFile.log')
+    RemoveFile(PatchLog)
+    FileLog = os.path.join(WORKDIR, "File.log")
+    RemoveFile(FileLog)
+    if GlobalSymbol.get('GenerateEccReport'):
+        FileList = os.listdir(WORKDIR)
+        CsvList = [os.path.join(WORKDIR, File) for File in FileList if File.endswith('.csv')]
+        for Csv in CsvList:
+            RemoveFile(Csv)
+    if GlobalSymbol.get('SCRIPT_ERROR'):
+        print('ECC tool detect error')
+        exit(1)
+
+def EdksetupRebuild(EnvList, WORKDIR):
+    EnvList['WORKDIR'] = WORKDIR
+    EnvList['WORKSPACE'] = WORKDIR
+    edk2_setup_cmd = ["edksetup", "Rebuild"]
+    _, _, result, return_code = ExecuteScript(edk2_setup_cmd, EnvList, collect_env=True, shell=True)
+    if return_code == 0:
+        for Key in result:
+            EnvList[Key] = result[Key]
+        return True
+    return False
+
+def GetDiffrange(EnvList, commit):
+    WORKDIR = EnvList['WORKDIR']
+    ModifyFileListLog = EnvList['ModifyFileListLog']
+    RangeDirectory = {}
+    PatchLog = os.path.join(WORKDIR, 'PatchFile.log')
+    Format_Patch_cmd = ["git", "show", str(commit), "--unified=0", ">", PatchLog]
+    _, _, result, return_code = ExecuteScript(Format_Patch_cmd, EnvList, shell=True)
+    if return_code != 0:
+        print('Fail to run GIT')
+        GlobalSymbol['SCRIPT_ERROR'] = True
+        EndDelFile(EnvList)
+    with open(PatchLog, encoding='utf8') as PatchLogFile:
+        Filelines = PatchLogFile.readlines()
+        IsDelete = True
+        StartCheck = False
+        for line in Filelines:
+            ModifyFile = FindModifyFile.findall(line)
+            if ModifyFile and not StartCheck and os.path.isfile(ModifyFile[0]):
+                ModifyFileCommentDic = GetCommentRange(EnvList, ModifyFile[0])
+                IsDelete = False
+                StartCheck = True
+                ModifyFilieDic = ModifyFile[0]
+                ModifyFilieDic = ModifyFilieDic.replace("/","\\")
+                RangeDirectory[ModifyFilieDic] = []
+            elif line.startswith('--- '):
+                StartCheck = False
+            elif re.match(LineScopePattern, line, re.I) and not IsDelete and StartCheck:
+                startline = LineNumRange.search(line).group(1)
+                linerange = LineNumRange.search(line).group(2)
+                if not linerange:
+                    linerange = '1'
+                RangeDirectory[ModifyFilieDic].append((int(startline), int(startline) + int(linerange) - 1))
+                for i in ModifyFileCommentDic:
+                    if i[0] <= int(startline) <= i[1]:
+                        RangeDirectory[ModifyFilieDic].append(i)
+    return RangeDirectory
+
+def GetCommentRange(EnvList, ModifyFile):
+    WORKDIR = EnvList['WORKDIR']
+    ModifyFilePath = os.path.join(WORKDIR, ModifyFile)
+    with open(ModifyFilePath) as f:
+        LineNo = 1
+        CommentRange = []
+        Start = False
+        for line in f:
+            if line.startswith('/**'):
+                startno = LineNo
+                Start = True
+            if line.startswith('**/') and Start:
+                endno = LineNo
+                Start = False
+                CommentRange.append((int(startno), int(endno)))
+            LineNo += 1
+
+    if CommentRange and CommentRange[0][0] == 1:
+        del CommentRange[0]
+    return CommentRange
+
+def GetModifyDir(EnvList, commit):
+    WORKDIR = EnvList['WORKDIR']
+    ModifyDirList = []
+    ModifyFileListLog = os.path.join(WORKDIR, 'PatchModifyFiles.log')
+    EnvList['ModifyFileListLog'] = ModifyFileListLog
+    Patch_Modify_cmd = ["git", "diff", "--name-status", str(commit), str(commit)+"~1", ">", ModifyFileListLog]
+    _, _, result, return_code = ExecuteScript(Patch_Modify_cmd, EnvList, shell=True)
+    if return_code != 0:
+        print('Fail to run GIT')
+        GlobalSymbol['SCRIPT_ERROR'] = True
+        EndDelFile(EnvList)
+    with open(ModifyFileListLog) as ModifyFile:
+        Filelines = ModifyFile.readlines()
+    for Line in Filelines:
+        FilePath = ReModifyFile.findall(Line)
+        if FilePath:
+            FileDir = os.path.dirname(FilePath[0])
+        else:
+            continue
+        PkgList = GetPkgList(EnvList)
+        if FileDir in PkgList or not FileDir:
+            continue
+        else:
+            ModifyDirList.append('%s'%FileDir)
+
+    ModifyDirList = list(set(ModifyDirList))
+    return ModifyDirList
+
+def ApplyConfig(EnvList, ModifyDirList, EccDiffRange):
+    WORKDIR = EnvList['WORKDIR']
+    ModifyPkgList = []
+    for ModifyDir in ModifyDirList:
+        ModifyPkg = ModifyDir.split("/")[0]
+        ModifyPkgList.append(ModifyPkg)
+    ModifyPkgList = list(set(ModifyPkgList))
+    for ModifyPkg in ModifyPkgList:
+        pkg_config_file = os.path.join(WORKDIR, ModifyPkg, ModifyPkg + ".ci.yaml")
+        if os.path.exists(pkg_config_file):
+            with open(pkg_config_file, 'r') as f:
+                pkg_config = yaml.safe_load(f)
+            if "EccCheck" in pkg_config:
+                EccConfig = pkg_config["EccCheck"]
+                #
+                # Add exceptions
+                #
+                ExceptionList = EccConfig["ExceptionList"]
+                ExceptionFile = os.path.join(WORKDIR, "BaseTools", "Source", "Python", "Ecc", "exception.xml")
+                if os.path.exists(ExceptionFile):
+                    AppendException(ExceptionList, ExceptionFile)
+                #
+                # Exclude ignored files
+                #
+                IgnoreFilesList = EccConfig['IgnoreFiles']
+                for ignore_file in IgnoreFilesList:
+                    ignore_file = ignore_file.replace("/", "\\")
+                    ignore_file = os.path.join(ModifyPkg, ignore_file)
+                    if ignore_file in EccDiffRange:
+                        del EccDiffRange[ignore_file]
+    return
+
+def CheckOneCommit(commit):
+    WORKDIR = os.getcwd()
+    EdksetupRebuild(EnvList, WORKDIR)
+    ModifyDirList = GetModifyDir(EnvList, commit)
+    EccDiffRange = GetDiffrange(EnvList, commit)
+    ApplyConfig(EnvList, ModifyDirList, EccDiffRange)
+    GenerateEccReport(EnvList, ModifyDirList, EccDiffRange)
+    EndDelFile(EnvList)
+
+def parse_options():
+    parser = argparse.ArgumentParser(description=__copyright__)
+    parser.add_argument('commits', nargs='*',
+                        help='[commit(s) ID | number of commits, like "-3" means check first 3 commits]')
+    args = parser.parse_args()
+    return args
+
+def process_one_arg(self, arg):
+        if len(arg) >= 2 and arg[0] == '-':
+            count = int(arg[1:])
+        CheckOneArg(arg, self.count).ok
+
+
+def read_commit_list_from_git(start_commit, count):
+    cmd = ['git', 'rev-list', '--abbrev-commit', '--no-walk' ]
+    if count is not None:
+            cmd.append('--max-count=' + str(count))
+    cmd.append(start_commit)
+    p = subprocess.Popen(cmd,
+                stdout=subprocess.PIPE,
+                stderr=subprocess.STDOUT)
+    result = p.communicate()
+    out = result[0].decode('utf-8', 'ignore') if result[0] and result[0].find(b"fatal")!=0 else None
+    return out.split() if out else []
+
+def ReleaseReport(EnvList):
+        WORKDIR = EnvList['WORKDIR']
+        EccLog = os.path.join(WORKDIR, "ECC.log")
+        if GlobalSymbol['ECC_PASS']:
+            print('\n===================Ecc pass===================')
+            RemoveFile(EccLog)
+            return 0
+        else:
+            print('\n===================Ecc error detected===================')
+            with open(EccLog) as output:
+                print(output.read())
+            RemoveFile(EccLog)
+            return -1
+
+def main():
+    commits = parse_options().commits
+
+    if len(commits) == 0:
+        commits = [ 'HEAD' ]
+
+    if len(commits[0]) >= 2 and commits[0][0] == '-':
+        count = int(commits[0][1:])
+        commits = read_commit_list_from_git('HEAD', count)
+
+    """
+        This 'if' block is only used for creating pull request.
+    """
+    if ".." in commits[0]:
+        commits = read_commit_list_from_git(commits[0], None)
+
+    GlobalSymbol['ECC_PASS'] = True
+    for commit in commits:
+        CheckOneCommit(commit)
+
+    retval = ReleaseReport(EnvList)
+    return retval
+
+if __name__ == "__main__":
+    sys.exit(main())
\ No newline at end of file
-- 
2.18.0.windows.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 3/5] pip-requirements.txt: Add Ecc required lib
  2020-06-03  8:48 [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci Zhang, Shenglei
  2020-06-03  8:48 ` [PATCH v2 1/5] BaseTools:ECC needs to update the contents of CParser4 Zhang, Shenglei
  2020-06-03  8:48 ` [PATCH v2 2/5] BaseTools/Scripts: Add EccCheck.py Zhang, Shenglei
@ 2020-06-03  8:48 ` Zhang, Shenglei
  2020-06-03  8:48 ` [PATCH v2 4/5] .azurepiplines: Add a pipline to check ECC issues for commits Zhang, Shenglei
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Zhang, Shenglei @ 2020-06-03  8:48 UTC (permalink / raw)
  To: devel; +Cc: Sean Brogan, Bret Barkelew, Michael D Kinney, Liming Gao

antlr4-python3-runtime is a lib to support Ecc run with Py3.x.

Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
 pip-requirements.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/pip-requirements.txt b/pip-requirements.txt
index 574dac43b1a6..7e83ddd9b3c9 100644
--- a/pip-requirements.txt
+++ b/pip-requirements.txt
@@ -14,3 +14,4 @@
 
 edk2-pytool-library==0.10.*
 edk2-pytool-extensions~=0.13.3
+antlr4-python3-runtime
\ No newline at end of file
-- 
2.18.0.windows.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 4/5] .azurepiplines: Add a pipline to check ECC issues for commits
  2020-06-03  8:48 [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci Zhang, Shenglei
                   ` (2 preceding siblings ...)
  2020-06-03  8:48 ` [PATCH v2 3/5] pip-requirements.txt: Add Ecc required lib Zhang, Shenglei
@ 2020-06-03  8:48 ` Zhang, Shenglei
  2020-06-03  8:48 ` [PATCH v2 5/5] MdeModulePkg/MdeModulePkg.ci.yaml: Add configuration for Ecc check Zhang, Shenglei
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Zhang, Shenglei @ 2020-06-03  8:48 UTC (permalink / raw)
  To: devel; +Cc: Sean Brogan, Bret Barkelew, Michael D Kinney, Liming Gao

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2606
Add a pipeline to run the ECC checks on Azure Pipelines agents
for edk2 open ci.

Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
 .azurepipelines/Windows-EccCheck.yml | 38 ++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 .azurepipelines/Windows-EccCheck.yml

diff --git a/.azurepipelines/Windows-EccCheck.yml b/.azurepipelines/Windows-EccCheck.yml
new file mode 100644
index 000000000000..0d20551efb5f
--- /dev/null
+++ b/.azurepipelines/Windows-EccCheck.yml
@@ -0,0 +1,38 @@
+## @file
+# Azure Pipielines YML file that evalues the patch series in a PR using the
+# python script BaseTools/Scripts/PatchCheck.py.
+#
+# NOTE: This example monitors pull requests against the edk2-ci branch.  Most
+# environments would replace 'edk2-ci' with 'master'.
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+# https://github.com/tianocore
+#
+##
+
+trigger: none
+
+pr:
+- master
+
+pool:
+  vmImage: 'windows-latest'
+
+steps:
+- checkout: self
+  clean: true
+
+- task: UsePythonVersion@0
+  inputs:
+    versionSpec: '3.8.x'
+    architecture: 'x64'
+
+- script: pip install -r pip-requirements.txt
+  displayName: 'Install/Upgrade pip modules'
+
+- script: |
+    git fetch origin $(System.PullRequest.TargetBranch):$(System.PullRequest.TargetBranch)
+    py -3 BaseTools/Scripts/EccCheck.py $(System.PullRequest.TargetBranch)..$(System.PullRequest.SourceCommitId)
+  displayName: 'Use EccCheck.py to verify patch series in pull request'
-- 
2.18.0.windows.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 5/5] MdeModulePkg/MdeModulePkg.ci.yaml: Add configuration for Ecc check
  2020-06-03  8:48 [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci Zhang, Shenglei
                   ` (3 preceding siblings ...)
  2020-06-03  8:48 ` [PATCH v2 4/5] .azurepiplines: Add a pipline to check ECC issues for commits Zhang, Shenglei
@ 2020-06-03  8:48 ` Zhang, Shenglei
  2020-06-03 14:27 ` [edk2-devel] [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci Laszlo Ersek
  2020-06-09 13:10 ` Liming Gao
  6 siblings, 0 replies; 12+ messages in thread
From: Zhang, Shenglei @ 2020-06-03  8:48 UTC (permalink / raw)
  To: devel; +Cc: Jian J Wang, Hao A Wu

This is a sample for users to add exceptions and ignored files for
their own use.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
---
 MdeModulePkg/MdeModulePkg.ci.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.ci.yaml b/MdeModulePkg/MdeModulePkg.ci.yaml
index 1cfc1328390e..8ccf667d12f3 100644
--- a/MdeModulePkg/MdeModulePkg.ci.yaml
+++ b/MdeModulePkg/MdeModulePkg.ci.yaml
@@ -5,6 +5,14 @@
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 ##
 {
+    "EccCheck": {
+        "ExceptionList": [
+            "8001", "aaabbb"
+        ],
+        "IgnoreFiles": [
+            "Library/BaseBmpSupportLib/BmpSupportLib.c"
+        ]
+    },
     ## options defined ci/Plugin/CompilerPlugin
     "CompilerPlugin": {
         "DscPath": "MdeModulePkg.dsc"
-- 
2.18.0.windows.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [edk2-devel] [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci
  2020-06-03  8:48 [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci Zhang, Shenglei
                   ` (4 preceding siblings ...)
  2020-06-03  8:48 ` [PATCH v2 5/5] MdeModulePkg/MdeModulePkg.ci.yaml: Add configuration for Ecc check Zhang, Shenglei
@ 2020-06-03 14:27 ` Laszlo Ersek
  2020-06-04  5:38   ` Zhang, Shenglei
  2020-06-09 13:10 ` Liming Gao
  6 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2020-06-03 14:27 UTC (permalink / raw)
  To: devel, shenglei.zhang
  Cc: Bob Feng, Bret Barkelew, Michael D Kinney, Liming Gao,
	Sean Brogan

On 06/03/20 10:48, Zhang, Shenglei wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2606
> As planed we will enable Ecc check for edk2 on open ci. And they are
> ready now, but these are V2 series. So I expect that contributors in
> edk2 community can try using this script when reviewing. And I appreciate
> receiving feedback and comments if someone find errors or false positive
> issues.
> 
> I created a pipline of EccCheck for my forked edk2.
> https://dev.azure.com/shengleizhang/shengleizhang/_build?definitionId=10&_a=summary
> 
> The patch series are big, so the commits are also pushed into my forked tree.
> https://github.com/shenglei10/edk2/commits/ECC
> 
> Patches
> 1/5: This is a patch to enable python 3.8 for Ecc. It is a tool issues not
>      a pipline or script issue. But it is listed here for people willing
>      to try this tool.
> 2/5: EccCheck.py is a tool to report Ecc issues for commits. It can be run
>      on azure servers for open ci, or locally. Its usage is like
>      PatchCheck.py.
> 3/5: It's a lib necessary for py3 to run Ecc on azure servers. For local
>      use, we need to type command
>      "py -3 -m pip install antlr4-python3-runtime" first.
> 4/5: Windows-EccCheck.yml is a yaml file to configure the newly added
>      pipline. The azure uses this to create a pipline.
> 5/5: We consider some cases that will report out Ecc issues but they won't
>      be fixed, like submodule and industry standard related things. So we
>      add two configuration fields "Exception" and "IgnoreFiles" for people
>      to use. The patch is a example and the contents in the fields will be
>      empty in final version.
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> 
> v2: Update 2/5, fix the bug that the script can't hanlde multiple commits.

Thanks for the ExceptionList / IgnoreFiles features; I think they are
really important. I've run ECC in the past, and in some cases it is
*way* too strict and opinionated, so I'm sure we'll end up "training"
the ExceptionList entry for OvmfPkg.

Can you please explain how (if?) ECC is restricted to new code added by
a patch series? Patch#2 seems related, but I don't fully understand. It
says,

"It can only handle the issues, whose line number in CSV report
accurately map with their code in source code files."

Does that mean that CI performs a full ECC check, but filters out all
warning / error messages that do not refer to code lines added in the
patch series?

Thanks,
Laszlo


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [edk2-devel] [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci
  2020-06-03 14:27 ` [edk2-devel] [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci Laszlo Ersek
@ 2020-06-04  5:38   ` Zhang, Shenglei
  2020-06-09 12:24     ` Laszlo Ersek
  0 siblings, 1 reply; 12+ messages in thread
From: Zhang, Shenglei @ 2020-06-04  5:38 UTC (permalink / raw)
  To: Laszlo Ersek, devel@edk2.groups.io
  Cc: Feng, Bob C, Bret Barkelew, Kinney, Michael D, Gao, Liming,
	Sean Brogan

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, June 3, 2020 10:27 PM
> To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zhang@intel.com>
> Cc: Feng, Bob C <bob.c.feng@intel.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean
> Brogan <sean.brogan@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH v2 0/5] Add a pipline to check Ecc issues for
> edk2 on open ci
> 
> On 06/03/20 10:48, Zhang, Shenglei wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2606
> > As planed we will enable Ecc check for edk2 on open ci. And they are
> > ready now, but these are V2 series. So I expect that contributors in
> > edk2 community can try using this script when reviewing. And I appreciate
> > receiving feedback and comments if someone find errors or false positive
> > issues.
> >
> > I created a pipline of EccCheck for my forked edk2.
> >
> https://dev.azure.com/shengleizhang/shengleizhang/_build?definitionId=10
> &_a=summary
> >
> > The patch series are big, so the commits are also pushed into my forked
> tree.
> > https://github.com/shenglei10/edk2/commits/ECC
> >
> > Patches
> > 1/5: This is a patch to enable python 3.8 for Ecc. It is a tool issues not
> >      a pipline or script issue. But it is listed here for people willing
> >      to try this tool.
> > 2/5: EccCheck.py is a tool to report Ecc issues for commits. It can be run
> >      on azure servers for open ci, or locally. Its usage is like
> >      PatchCheck.py.
> > 3/5: It's a lib necessary for py3 to run Ecc on azure servers. For local
> >      use, we need to type command
> >      "py -3 -m pip install antlr4-python3-runtime" first.
> > 4/5: Windows-EccCheck.yml is a yaml file to configure the newly added
> >      pipline. The azure uses this to create a pipline.
> > 5/5: We consider some cases that will report out Ecc issues but they won't
> >      be fixed, like submodule and industry standard related things. So we
> >      add two configuration fields "Exception" and "IgnoreFiles" for people
> >      to use. The patch is a example and the contents in the fields will be
> >      empty in final version.
> >
> > Cc: Bob Feng <bob.c.feng@intel.com>
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> >
> > v2: Update 2/5, fix the bug that the script can't hanlde multiple commits.
> 
> Thanks for the ExceptionList / IgnoreFiles features; I think they are
> really important. I've run ECC in the past, and in some cases it is
> *way* too strict and opinionated, so I'm sure we'll end up "training"
> the ExceptionList entry for OvmfPkg.
> 
> Can you please explain how (if?) ECC is restricted to new code added by
> a patch series? Patch#2 seems related, but I don't fully understand. It
> says,
> 
> "It can only handle the issues, whose line number in CSV report
> accurately map with their code in source code files."
> 
> Does that mean that CI performs a full ECC check, but filters out all
> warning / error messages that do not refer to code lines added in the
> patch series?
> 

Yes, I think you get the point.
Since there are plenty of Ecc issues existing in edk2, we need to filter out the
issues that are not caused by the patch to be checked in. The actual implementation
is to create a dictionary for modified files, in which the key word is the modified file and
the content is the line number scope for added code. Then if the issues in CSV report can
be mapped with the dictionary, they are marked as real issues.

And the scanning scope is the folder that the change is located in. For example,
Someone modifies "FmpDevicePkg\Library\FmpDependencyLib\FmpDependencyLib.c".
Then the scan scope is "FmpDevicePkg\Library\FmpDependencyLib". It's not a full check
for edk2, because it will cost much time.

Thanks,
Shenglei

> Thanks,
> Laszlo


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [edk2-devel] [PATCH v2 1/5] BaseTools:ECC needs to update the contents of CParser4
  2020-06-03  8:48 ` [PATCH v2 1/5] BaseTools:ECC needs to update the contents of CParser4 Zhang, Shenglei
@ 2020-06-05  4:38   ` Yuwei Chen
  2020-06-05  5:11     ` Zhang, Shenglei
  0 siblings, 1 reply; 12+ messages in thread
From: Yuwei Chen @ 2020-06-05  4:38 UTC (permalink / raw)
  To: devel@edk2.groups.io, Zhang, Shenglei
  Cc: Fan, ZhijuX, Feng, Bob C, Gao, Liming

Hi, Shenglei

Since the input arguments number of the getText() function changed, the build failed. May be you should check about the getText() function's input.
While how to choose the CParser4 and CParser3, I am not familiar with it, would you like to explain for me?

Thanks,
Yuwei
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Zhang,
> Shenglei
> Sent: Wednesday, June 3, 2020 4:48 PM
> To: devel@edk2.groups.io
> Cc: Fan, ZhijuX <zhijux.fan@intel.com>; Feng, Bob C
> <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2-devel] [PATCH v2 1/5] BaseTools:ECC needs to update the
> contents of CParser4
> 
> From: "Fan, Zhiju" <zhijux.fan@intel.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2676
> 
> Because Ecc relies on the antlr extension package, When the antlr version is
> updated, we need to change the code in it.
> Currently, if you use the latest version antlr4.8, Ecc will fail
> 
> We will update the version to support the use of antlr4.8 and all previous
> versions will have errors.This can be resolved by installing the antlr4.8 version.
> Installation method: pip install antlr4-python3-runtime==4.8
> 
> This patch is going to fixed this 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>
> ---
>  .../Source/Python/Ecc/CParser4/CLexer.py      |  6 +--
>  .../Source/Python/Ecc/CParser4/CListener.py   |  4 +-
>  .../Source/Python/Ecc/CParser4/CParser.py     | 38 ++++++++++---------
>  3 files changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/BaseTools/Source/Python/Ecc/CParser4/CLexer.py
> b/BaseTools/Source/Python/Ecc/CParser4/CLexer.py
> index a2cc5bf56e66..40e2afbf1a1f 100644
> --- a/BaseTools/Source/Python/Ecc/CParser4/CLexer.py
> +++ b/BaseTools/Source/Python/Ecc/CParser4/CLexer.py
> @@ -1,4 +1,4 @@
> -# Generated from C.g4 by ANTLR 4.7.1
> +# Generated from C.g4 by ANTLR 4.8
>  from antlr4 import *
>  from io import StringIO
>  from typing.io import TextIO
> @@ -12,7 +12,7 @@ import sys
>  # This file is generated by running:
>  # java org.antlr.Tool C.g
>  #
> -# Copyright (c) 2009 - 2010, Intel Corporation  All rights reserved.
> +# Copyright (c) 2009 - 2020, Intel Corporation  All rights reserved.
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -584,7 +584,7 @@
> class CLexer(Lexer):
>      # @param  output= sys.stdout Type: TextIO
>      def __init__(self,input=None,output= sys.stdout):
>          super().__init__(input, output)
> -        self.checkVersion("4.7.1")
> +        self.checkVersion("4.8")
>          self._interp = LexerATNSimulator(self, self.atn, self.decisionsToDFA,
> PredictionContextCache())
>          self._actions = None
>          self._predicates = None
> diff --git a/BaseTools/Source/Python/Ecc/CParser4/CListener.py
> b/BaseTools/Source/Python/Ecc/CParser4/CListener.py
> index bb4351d9249a..ba7e70892680 100644
> --- a/BaseTools/Source/Python/Ecc/CParser4/CListener.py
> +++ b/BaseTools/Source/Python/Ecc/CParser4/CListener.py
> @@ -1,4 +1,4 @@
> -# Generated from C.g4 by ANTLR 4.7.1
> +# Generated from C.g4 by ANTLR 4.8
>  from antlr4 import *
>  if __name__ is not None and "." in __name__:
>      from .CParser import CParser
> @@ -12,7 +12,7 @@ else:
>  # This file is generated by running:
>  # java org.antlr.Tool C.g
>  #
> -# Copyright (c) 2009 - 2010, Intel Corporation  All rights reserved.
> +# Copyright (c) 2009 - 2020, Intel Corporation  All rights reserved.
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent  # diff --git
> a/BaseTools/Source/Python/Ecc/CParser4/CParser.py
> b/BaseTools/Source/Python/Ecc/CParser4/CParser.py
> index 31d23d55aa57..30d70a74e669 100644
> --- a/BaseTools/Source/Python/Ecc/CParser4/CParser.py
> +++ b/BaseTools/Source/Python/Ecc/CParser4/CParser.py
> @@ -1,10 +1,12 @@
> -# Generated from C.g4 by ANTLR 4.7.1
> +# Generated from C.g4 by ANTLR 4.8
>  # encoding: utf-8
>  from antlr4 import *
>  from io import StringIO
> -from typing.io import TextIO
>  import sys
> -
> +if sys.version_info[1] > 5:
> +    from typing import TextIO
> +else:
> +    from typing.io import TextIO
> 
>  ## @file
>  # The file defines the parser for C source files.
> @@ -13,7 +15,7 @@ import sys
>  # This file is generated by running:
>  # java org.antlr.Tool C.g
>  #
> -# Copyright (c) 2009 - 2010, Intel Corporation  All rights reserved.
> +# Copyright (c) 2009 - 2020, Intel Corporation  All rights reserved.
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -739,7 +741,7 @@
> class CParser ( Parser ):
>      # @param  output= sys.stdout Type: TextIO
>      def __init__(self,input,output= sys.stdout):
>          super().__init__(input, output)
> -        self.checkVersion("4.7.1")
> +        self.checkVersion("4.8")
>          self._interp = ParserATNSimulator(self, self.atn, self.decisionsToDFA,
> self.sharedContextCache)
>          self._predicates = None
> 
> @@ -1062,10 +1064,10 @@ class CParser ( Parser ):
> 
> 
>              if localctx.d != None:
> -                ModifierText = (None if localctx._declaration_specifiers is None else
> self._input.getText((localctx._declaration_specifiers.start,localctx._declarati
> on_specifiers.stop)))
> +                ModifierText = (None if
> + localctx._declaration_specifiers is None else
> + self._input.getText(localctx._declaration_specifiers.start,localctx._d
> + eclaration_specifiers.stop))
>              else:
>                  ModifierText = ''
> -            DeclText = (None if localctx._declarator is None else
> self._input.getText((localctx._declarator.start,localctx._declarator.stop)))
> +            DeclText = (None if localctx._declarator is None else
> + self._input.getText(localctx._declarator.start,localctx._declarator.st
> + op))
>              DeclLine = (None if localctx._declarator is None else
> localctx._declarator.start).line
>              DeclOffset = (None if localctx._declarator is None else
> localctx._declarator.start).column
>              if localctx.a != None:
> @@ -1245,9 +1247,9 @@ class CParser ( Parser ):
>                  localctx.d = self.match(CParser.T__1)
> 
>                  if localctx.b is not None:
> -                    self.StoreTypedefDefinition(localctx.a.line, localctx.a.column, (0 if
> localctx.d is None else localctx.d.line), localctx.d.column, (None if localctx.b is
> None else self._input.getText((localctx.b.start,localctx.b.stop))), (None if
> localctx.c is None else self._input.getText((localctx.c.start,localctx.c.stop))))
> +                    self.StoreTypedefDefinition(localctx.a.line,
> + localctx.a.column, (0 if localctx.d is None else localctx.d.line),
> + localctx.d.column, (None if localctx.b is None else
> + self._input.getText(localctx.b.start,localctx.b.stop)), (None if
> + localctx.c is None else
> + self._input.getText(localctx.c.start,localctx.c.stop)))
>                  else:
> -                    self.StoreTypedefDefinition(localctx.a.line, localctx.a.column, (0 if
> localctx.d is None else localctx.d.line), localctx.d.column, '', (None if localctx.c
> is None else self._input.getText((localctx.c.start,localctx.c.stop))))
> +                    self.StoreTypedefDefinition(localctx.a.line,
> + localctx.a.column, (0 if localctx.d is None else localctx.d.line),
> + localctx.d.column, '', (None if localctx.c is None else
> + self._input.getText(localctx.c.start,localctx.c.stop)))
> 
>                  pass
>              elif token in [CParser.T__5, CParser.T__6, CParser.T__7, CParser.T__8,
> CParser.T__9, CParser.T__10, CParser.T__11, CParser.T__12, CParser.T__13,
> CParser.T__14, CParser.T__15, CParser.T__16, CParser.T__17, CParser.T__18,
> CParser.T__20, CParser.T__21, CParser.T__23, CParser.T__24, CParser.T__25,
> CParser.T__26, CParser.T__27, CParser.T__28, CParser.T__29, CParser.T__30,
> CParser.T__31, CParser.T__32, CParser.T__33, CParser.T__34, CParser.T__35,
> CParser.T__36, CParser.IDENTIFIER]:
> @@ -1266,7 +1268,7 @@ class CParser ( Parser ):
>                  localctx.e = self.match(CParser.T__1)
> 
>                  if localctx.t is not None:
> -                    self.StoreVariableDeclaration((None if localctx.s is None else
> localctx.s.start).line, (None if localctx.s is None else localctx.s.start).column,
> (None if localctx.t is None else localctx.t.start).line, (None if localctx.t is None
> else localctx.t.start).column, (None if localctx.s is None else
> self._input.getText((localctx.s.start,localctx.s.stop))), (None if localctx.t is
> None else self._input.getText((localctx.t.start,localctx.t.stop))))
> +                    self.StoreVariableDeclaration((None if localctx.s
> + is None else localctx.s.start).line, (None if localctx.s is None else
> + localctx.s.start).column, (None if localctx.t is None else
> + localctx.t.start).line, (None if localctx.t is None else
> + localctx.t.start).column, (None if localctx.s is None else
> + self._input.getText(localctx.s.start,localctx.s.stop)), (None if
> + localctx.t is None else
> + self._input.getText(localctx.t.start,localctx.t.stop)))
> 
>                  pass
>              else:
> @@ -1568,7 +1570,7 @@ class CParser ( Parser ):
>                  localctx.s = self.struct_or_union_specifier()
> 
>                  if localctx.s.stop is not None:
> -                    self.StoreStructUnionDefinition((None if localctx.s is None else
> localctx.s.start).line, (None if localctx.s is None else localctx.s.start).column,
> (None if localctx.s is None else localctx.s.stop).line, (None if localctx.s is None
> else localctx.s.stop).column, (None if localctx.s is None else
> self._input.getText((localctx.s.start,localctx.s.stop))))
> +                    self.StoreStructUnionDefinition((None if localctx.s
> + is None else localctx.s.start).line, (None if localctx.s is None else
> + localctx.s.start).column, (None if localctx.s is None else
> + localctx.s.stop).line, (None if localctx.s is None else
> + localctx.s.stop).column, (None if localctx.s is None else
> + self._input.getText(localctx.s.start,localctx.s.stop)))
> 
>                  pass
> 
> @@ -1578,7 +1580,7 @@ class CParser ( Parser ):
>                  localctx.e = self.enum_specifier()
> 
>                  if localctx.e.stop is not None:
> -                    self.StoreEnumerationDefinition((None if localctx.e is None else
> localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column,
> (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is
> None else localctx.e.stop).column, (None if localctx.e is None else
> self._input.getText((localctx.e.start,localctx.e.stop))))
> +                    self.StoreEnumerationDefinition((None if localctx.e
> + is None else localctx.e.start).line, (None if localctx.e is None else
> + localctx.e.start).column, (None if localctx.e is None else
> + localctx.e.stop).line, (None if localctx.e is None else
> + localctx.e.stop).column, (None if localctx.e is None else
> + self._input.getText(localctx.e.start,localctx.e.stop)))
> 
>                  pass
> 
> @@ -4022,7 +4024,7 @@ class CParser ( Parser ):
>              self.enterOuterAlt(localctx, 1)
>              self.state = 569
>              localctx.p = self.primary_expression()
> -            self.FuncCallText += (None if localctx.p is None else
> self._input.getText((localctx.p.start,localctx.p.stop)))
> +            self.FuncCallText += (None if localctx.p is None else
> + self._input.getText(localctx.p.start,localctx.p.stop))
>              self.state = 600
>              self._errHandler.sync(self)
>              _alt = self._interp.adaptivePredict(self._input,73,self._ctx)
> @@ -4055,7 +4057,7 @@ class CParser ( Parser ):
>                          localctx.c = self.argument_expression_list()
>                          self.state = 580
>                          localctx.b = self.match(CParser.T__38)
> -                        self.StoreFunctionCalling((None if localctx.p is None else
> localctx.p.start).line, (None if localctx.p is None else localctx.p.start).column,
> (0 if localctx.b is None else localctx.b.line), localctx.b.column,
> self.FuncCallText, (None if localctx.c is None else
> self._input.getText((localctx.c.start,localctx.c.stop))))
> +                        self.StoreFunctionCalling((None if localctx.p
> + is None else localctx.p.start).line, (None if localctx.p is None else
> + localctx.p.start).column, (0 if localctx.b is None else
> + localctx.b.line), localctx.b.column, self.FuncCallText, (None if
> + localctx.c is None else
> + self._input.getText(localctx.c.start,localctx.c.stop)))
>                          pass
> 
>                      elif la_ == 4:
> @@ -4770,7 +4772,7 @@ class CParser ( Parser ):
>                  self.match(CParser.T__22)
>                  self.state = 674
>                  self.conditional_expression()
> -                self.StorePredicateExpression((None if localctx.e is None else
> localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column,
> (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is
> None else localctx.e.stop).column, (None if localctx.e is None else
> self._input.getText((localctx.e.start,localctx.e.stop))))
> +                self.StorePredicateExpression((None if localctx.e is
> + None else localctx.e.start).line, (None if localctx.e is None else
> + localctx.e.start).column, (None if localctx.e is None else
> + localctx.e.stop).line, (None if localctx.e is None else
> + localctx.e.stop).column, (None if localctx.e is None else
> + self._input.getText(localctx.e.start,localctx.e.stop)))
> 
> 
>          except RecognitionException as re:
> @@ -6053,7 +6055,7 @@ class CParser ( Parser ):
>                  localctx.e = self.expression()
>                  self.state = 845
>                  self.match(CParser.T__38)
> -                self.StorePredicateExpression((None if localctx.e is None else
> localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column,
> (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is
> None else localctx.e.stop).column, (None if localctx.e is None else
> self._input.getText((localctx.e.start,localctx.e.stop))))
> +                self.StorePredicateExpression((None if localctx.e is
> + None else localctx.e.start).line, (None if localctx.e is None else
> + localctx.e.start).column, (None if localctx.e is None else
> + localctx.e.stop).line, (None if localctx.e is None else
> + localctx.e.stop).column, (None if localctx.e is None else
> + self._input.getText(localctx.e.start,localctx.e.stop)))
>                  self.state = 847
>                  self.statement()
>                  self.state = 850
> @@ -6144,7 +6146,7 @@ class CParser ( Parser ):
>                  self.match(CParser.T__38)
>                  self.state = 864
>                  self.statement()
> -                self.StorePredicateExpression((None if localctx.e is None else
> localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column,
> (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is
> None else localctx.e.stop).column, (None if localctx.e is None else
> self._input.getText((localctx.e.start,localctx.e.stop))))
> +                self.StorePredicateExpression((None if localctx.e is
> + None else localctx.e.start).line, (None if localctx.e is None else
> + localctx.e.start).column, (None if localctx.e is None else
> + localctx.e.stop).line, (None if localctx.e is None else
> + localctx.e.stop).column, (None if localctx.e is None else
> + self._input.getText(localctx.e.start,localctx.e.stop)))
>                  pass
>              elif token in [CParser.T__87]:
>                  self.enterOuterAlt(localctx, 2) @@ -6162,7 +6164,7 @@ class CParser
> ( Parser ):
>                  self.match(CParser.T__38)
>                  self.state = 873
>                  self.match(CParser.T__1)
> -                self.StorePredicateExpression((None if localctx.e is None else
> localctx.e.start).line, (None if localctx.e is None else localctx.e.start).column,
> (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is
> None else localctx.e.stop).column, (None if localctx.e is None else
> self._input.getText((localctx.e.start,localctx.e.stop))))
> +                self.StorePredicateExpression((None if localctx.e is
> + None else localctx.e.start).line, (None if localctx.e is None else
> + localctx.e.start).column, (None if localctx.e is None else
> + localctx.e.stop).line, (None if localctx.e is None else
> + localctx.e.stop).column, (None if localctx.e is None else
> + self._input.getText(localctx.e.start,localctx.e.stop)))
>                  pass
>              else:
>                  raise NoViableAltException(self)
> --
> 2.18.0.windows.1
> 
> 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [edk2-devel] [PATCH v2 1/5] BaseTools:ECC needs to update the contents of CParser4
  2020-06-05  4:38   ` [edk2-devel] " Yuwei Chen
@ 2020-06-05  5:11     ` Zhang, Shenglei
  0 siblings, 0 replies; 12+ messages in thread
From: Zhang, Shenglei @ 2020-06-05  5:11 UTC (permalink / raw)
  To: Chen, Yuwei, devel@edk2.groups.io, Fan, ZhijuX; +Cc: Feng, Bob C, Gao, Liming

Yuwei,

I'm also not familiar with that. I attached this patch in patch series because people can't run Ecc
with py38 if they want to use my script. Zhiju is the original author.

Zhiju,

Could you help answer Yuwei's question?

Thanks,
Shenglei

> -----Original Message-----
> From: Chen, Yuwei
> Sent: Friday, June 5, 2020 12:39 PM
> To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zhang@intel.com>
> Cc: Fan, ZhijuX <zhijux.fan@intel.com>; Feng, Bob C
> <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2-devel] [PATCH v2 1/5] BaseTools:ECC needs to update the
> contents of CParser4
> 
> Hi, Shenglei
> 
> Since the input arguments number of the getText() function changed, the
> build failed. May be you should check about the getText() function's input.
> While how to choose the CParser4 and CParser3, I am not familiar with it,
> would you like to explain for me?
> 
> Thanks,
> Yuwei
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Zhang,
> > Shenglei
> > Sent: Wednesday, June 3, 2020 4:48 PM
> > To: devel@edk2.groups.io
> > Cc: Fan, ZhijuX <zhijux.fan@intel.com>; Feng, Bob C
> > <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>
> > Subject: [edk2-devel] [PATCH v2 1/5] BaseTools:ECC needs to update the
> > contents of CParser4
> >
> > From: "Fan, Zhiju" <zhijux.fan@intel.com>
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2676
> >
> > Because Ecc relies on the antlr extension package, When the antlr version is
> > updated, we need to change the code in it.
> > Currently, if you use the latest version antlr4.8, Ecc will fail
> >
> > We will update the version to support the use of antlr4.8 and all previous
> > versions will have errors.This can be resolved by installing the antlr4.8
> version.
> > Installation method: pip install antlr4-python3-runtime==4.8
> >
> > This patch is going to fixed this 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>
> > ---
> >  .../Source/Python/Ecc/CParser4/CLexer.py      |  6 +--
> >  .../Source/Python/Ecc/CParser4/CListener.py   |  4 +-
> >  .../Source/Python/Ecc/CParser4/CParser.py     | 38 ++++++++++---------
> >  3 files changed, 25 insertions(+), 23 deletions(-)
> >
> > diff --git a/BaseTools/Source/Python/Ecc/CParser4/CLexer.py
> > b/BaseTools/Source/Python/Ecc/CParser4/CLexer.py
> > index a2cc5bf56e66..40e2afbf1a1f 100644
> > --- a/BaseTools/Source/Python/Ecc/CParser4/CLexer.py
> > +++ b/BaseTools/Source/Python/Ecc/CParser4/CLexer.py
> > @@ -1,4 +1,4 @@
> > -# Generated from C.g4 by ANTLR 4.7.1
> > +# Generated from C.g4 by ANTLR 4.8
> >  from antlr4 import *
> >  from io import StringIO
> >  from typing.io import TextIO
> > @@ -12,7 +12,7 @@ import sys
> >  # This file is generated by running:
> >  # java org.antlr.Tool C.g
> >  #
> > -# Copyright (c) 2009 - 2010, Intel Corporation  All rights reserved.
> > +# Copyright (c) 2009 - 2020, Intel Corporation  All rights reserved.
> >  #
> >  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -584,7 +584,7 @@
> > class CLexer(Lexer):
> >      # @param  output= sys.stdout Type: TextIO
> >      def __init__(self,input=None,output= sys.stdout):
> >          super().__init__(input, output)
> > -        self.checkVersion("4.7.1")
> > +        self.checkVersion("4.8")
> >          self._interp = LexerATNSimulator(self, self.atn, self.decisionsToDFA,
> > PredictionContextCache())
> >          self._actions = None
> >          self._predicates = None
> > diff --git a/BaseTools/Source/Python/Ecc/CParser4/CListener.py
> > b/BaseTools/Source/Python/Ecc/CParser4/CListener.py
> > index bb4351d9249a..ba7e70892680 100644
> > --- a/BaseTools/Source/Python/Ecc/CParser4/CListener.py
> > +++ b/BaseTools/Source/Python/Ecc/CParser4/CListener.py
> > @@ -1,4 +1,4 @@
> > -# Generated from C.g4 by ANTLR 4.7.1
> > +# Generated from C.g4 by ANTLR 4.8
> >  from antlr4 import *
> >  if __name__ is not None and "." in __name__:
> >      from .CParser import CParser
> > @@ -12,7 +12,7 @@ else:
> >  # This file is generated by running:
> >  # java org.antlr.Tool C.g
> >  #
> > -# Copyright (c) 2009 - 2010, Intel Corporation  All rights reserved.
> > +# Copyright (c) 2009 - 2020, Intel Corporation  All rights reserved.
> >  #
> >  # SPDX-License-Identifier: BSD-2-Clause-Patent  # diff --git
> > a/BaseTools/Source/Python/Ecc/CParser4/CParser.py
> > b/BaseTools/Source/Python/Ecc/CParser4/CParser.py
> > index 31d23d55aa57..30d70a74e669 100644
> > --- a/BaseTools/Source/Python/Ecc/CParser4/CParser.py
> > +++ b/BaseTools/Source/Python/Ecc/CParser4/CParser.py
> > @@ -1,10 +1,12 @@
> > -# Generated from C.g4 by ANTLR 4.7.1
> > +# Generated from C.g4 by ANTLR 4.8
> >  # encoding: utf-8
> >  from antlr4 import *
> >  from io import StringIO
> > -from typing.io import TextIO
> >  import sys
> > -
> > +if sys.version_info[1] > 5:
> > +    from typing import TextIO
> > +else:
> > +    from typing.io import TextIO
> >
> >  ## @file
> >  # The file defines the parser for C source files.
> > @@ -13,7 +15,7 @@ import sys
> >  # This file is generated by running:
> >  # java org.antlr.Tool C.g
> >  #
> > -# Copyright (c) 2009 - 2010, Intel Corporation  All rights reserved.
> > +# Copyright (c) 2009 - 2020, Intel Corporation  All rights reserved.
> >  #
> >  # SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -739,7 +741,7 @@
> > class CParser ( Parser ):
> >      # @param  output= sys.stdout Type: TextIO
> >      def __init__(self,input,output= sys.stdout):
> >          super().__init__(input, output)
> > -        self.checkVersion("4.7.1")
> > +        self.checkVersion("4.8")
> >          self._interp = ParserATNSimulator(self, self.atn, self.decisionsToDFA,
> > self.sharedContextCache)
> >          self._predicates = None
> >
> > @@ -1062,10 +1064,10 @@ class CParser ( Parser ):
> >
> >
> >              if localctx.d != None:
> > -                ModifierText = (None if localctx._declaration_specifiers is None
> else
> >
> self._input.getText((localctx._declaration_specifiers.start,localctx._declarati
> > on_specifiers.stop)))
> > +                ModifierText = (None if
> > + localctx._declaration_specifiers is None else
> > + self._input.getText(localctx._declaration_specifiers.start,localctx._d
> > + eclaration_specifiers.stop))
> >              else:
> >                  ModifierText = ''
> > -            DeclText = (None if localctx._declarator is None else
> > self._input.getText((localctx._declarator.start,localctx._declarator.stop)))
> > +            DeclText = (None if localctx._declarator is None else
> > + self._input.getText(localctx._declarator.start,localctx._declarator.st
> > + op))
> >              DeclLine = (None if localctx._declarator is None else
> > localctx._declarator.start).line
> >              DeclOffset = (None if localctx._declarator is None else
> > localctx._declarator.start).column
> >              if localctx.a != None:
> > @@ -1245,9 +1247,9 @@ class CParser ( Parser ):
> >                  localctx.d = self.match(CParser.T__1)
> >
> >                  if localctx.b is not None:
> > -                    self.StoreTypedefDefinition(localctx.a.line, localctx.a.column, (0
> if
> > localctx.d is None else localctx.d.line), localctx.d.column, (None if localctx.b
> is
> > None else self._input.getText((localctx.b.start,localctx.b.stop))), (None if
> > localctx.c is None else self._input.getText((localctx.c.start,localctx.c.stop))))
> > +                    self.StoreTypedefDefinition(localctx.a.line,
> > + localctx.a.column, (0 if localctx.d is None else localctx.d.line),
> > + localctx.d.column, (None if localctx.b is None else
> > + self._input.getText(localctx.b.start,localctx.b.stop)), (None if
> > + localctx.c is None else
> > + self._input.getText(localctx.c.start,localctx.c.stop)))
> >                  else:
> > -                    self.StoreTypedefDefinition(localctx.a.line, localctx.a.column, (0
> if
> > localctx.d is None else localctx.d.line), localctx.d.column, '', (None if
> localctx.c
> > is None else self._input.getText((localctx.c.start,localctx.c.stop))))
> > +                    self.StoreTypedefDefinition(localctx.a.line,
> > + localctx.a.column, (0 if localctx.d is None else localctx.d.line),
> > + localctx.d.column, '', (None if localctx.c is None else
> > + self._input.getText(localctx.c.start,localctx.c.stop)))
> >
> >                  pass
> >              elif token in [CParser.T__5, CParser.T__6, CParser.T__7,
> CParser.T__8,
> > CParser.T__9, CParser.T__10, CParser.T__11, CParser.T__12,
> CParser.T__13,
> > CParser.T__14, CParser.T__15, CParser.T__16, CParser.T__17,
> CParser.T__18,
> > CParser.T__20, CParser.T__21, CParser.T__23, CParser.T__24,
> CParser.T__25,
> > CParser.T__26, CParser.T__27, CParser.T__28, CParser.T__29,
> CParser.T__30,
> > CParser.T__31, CParser.T__32, CParser.T__33, CParser.T__34,
> CParser.T__35,
> > CParser.T__36, CParser.IDENTIFIER]:
> > @@ -1266,7 +1268,7 @@ class CParser ( Parser ):
> >                  localctx.e = self.match(CParser.T__1)
> >
> >                  if localctx.t is not None:
> > -                    self.StoreVariableDeclaration((None if localctx.s is None else
> > localctx.s.start).line, (None if localctx.s is None else localctx.s.start).column,
> > (None if localctx.t is None else localctx.t.start).line, (None if localctx.t is
> None
> > else localctx.t.start).column, (None if localctx.s is None else
> > self._input.getText((localctx.s.start,localctx.s.stop))), (None if localctx.t is
> > None else self._input.getText((localctx.t.start,localctx.t.stop))))
> > +                    self.StoreVariableDeclaration((None if localctx.s
> > + is None else localctx.s.start).line, (None if localctx.s is None else
> > + localctx.s.start).column, (None if localctx.t is None else
> > + localctx.t.start).line, (None if localctx.t is None else
> > + localctx.t.start).column, (None if localctx.s is None else
> > + self._input.getText(localctx.s.start,localctx.s.stop)), (None if
> > + localctx.t is None else
> > + self._input.getText(localctx.t.start,localctx.t.stop)))
> >
> >                  pass
> >              else:
> > @@ -1568,7 +1570,7 @@ class CParser ( Parser ):
> >                  localctx.s = self.struct_or_union_specifier()
> >
> >                  if localctx.s.stop is not None:
> > -                    self.StoreStructUnionDefinition((None if localctx.s is None else
> > localctx.s.start).line, (None if localctx.s is None else localctx.s.start).column,
> > (None if localctx.s is None else localctx.s.stop).line, (None if localctx.s is
> None
> > else localctx.s.stop).column, (None if localctx.s is None else
> > self._input.getText((localctx.s.start,localctx.s.stop))))
> > +                    self.StoreStructUnionDefinition((None if localctx.s
> > + is None else localctx.s.start).line, (None if localctx.s is None else
> > + localctx.s.start).column, (None if localctx.s is None else
> > + localctx.s.stop).line, (None if localctx.s is None else
> > + localctx.s.stop).column, (None if localctx.s is None else
> > + self._input.getText(localctx.s.start,localctx.s.stop)))
> >
> >                  pass
> >
> > @@ -1578,7 +1580,7 @@ class CParser ( Parser ):
> >                  localctx.e = self.enum_specifier()
> >
> >                  if localctx.e.stop is not None:
> > -                    self.StoreEnumerationDefinition((None if localctx.e is None else
> > localctx.e.start).line, (None if localctx.e is None else
> localctx.e.start).column,
> > (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is
> > None else localctx.e.stop).column, (None if localctx.e is None else
> > self._input.getText((localctx.e.start,localctx.e.stop))))
> > +                    self.StoreEnumerationDefinition((None if localctx.e
> > + is None else localctx.e.start).line, (None if localctx.e is None else
> > + localctx.e.start).column, (None if localctx.e is None else
> > + localctx.e.stop).line, (None if localctx.e is None else
> > + localctx.e.stop).column, (None if localctx.e is None else
> > + self._input.getText(localctx.e.start,localctx.e.stop)))
> >
> >                  pass
> >
> > @@ -4022,7 +4024,7 @@ class CParser ( Parser ):
> >              self.enterOuterAlt(localctx, 1)
> >              self.state = 569
> >              localctx.p = self.primary_expression()
> > -            self.FuncCallText += (None if localctx.p is None else
> > self._input.getText((localctx.p.start,localctx.p.stop)))
> > +            self.FuncCallText += (None if localctx.p is None else
> > + self._input.getText(localctx.p.start,localctx.p.stop))
> >              self.state = 600
> >              self._errHandler.sync(self)
> >              _alt = self._interp.adaptivePredict(self._input,73,self._ctx)
> > @@ -4055,7 +4057,7 @@ class CParser ( Parser ):
> >                          localctx.c = self.argument_expression_list()
> >                          self.state = 580
> >                          localctx.b = self.match(CParser.T__38)
> > -                        self.StoreFunctionCalling((None if localctx.p is None else
> > localctx.p.start).line, (None if localctx.p is None else
> localctx.p.start).column,
> > (0 if localctx.b is None else localctx.b.line), localctx.b.column,
> > self.FuncCallText, (None if localctx.c is None else
> > self._input.getText((localctx.c.start,localctx.c.stop))))
> > +                        self.StoreFunctionCalling((None if localctx.p
> > + is None else localctx.p.start).line, (None if localctx.p is None else
> > + localctx.p.start).column, (0 if localctx.b is None else
> > + localctx.b.line), localctx.b.column, self.FuncCallText, (None if
> > + localctx.c is None else
> > + self._input.getText(localctx.c.start,localctx.c.stop)))
> >                          pass
> >
> >                      elif la_ == 4:
> > @@ -4770,7 +4772,7 @@ class CParser ( Parser ):
> >                  self.match(CParser.T__22)
> >                  self.state = 674
> >                  self.conditional_expression()
> > -                self.StorePredicateExpression((None if localctx.e is None else
> > localctx.e.start).line, (None if localctx.e is None else
> localctx.e.start).column,
> > (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is
> > None else localctx.e.stop).column, (None if localctx.e is None else
> > self._input.getText((localctx.e.start,localctx.e.stop))))
> > +                self.StorePredicateExpression((None if localctx.e is
> > + None else localctx.e.start).line, (None if localctx.e is None else
> > + localctx.e.start).column, (None if localctx.e is None else
> > + localctx.e.stop).line, (None if localctx.e is None else
> > + localctx.e.stop).column, (None if localctx.e is None else
> > + self._input.getText(localctx.e.start,localctx.e.stop)))
> >
> >
> >          except RecognitionException as re:
> > @@ -6053,7 +6055,7 @@ class CParser ( Parser ):
> >                  localctx.e = self.expression()
> >                  self.state = 845
> >                  self.match(CParser.T__38)
> > -                self.StorePredicateExpression((None if localctx.e is None else
> > localctx.e.start).line, (None if localctx.e is None else
> localctx.e.start).column,
> > (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is
> > None else localctx.e.stop).column, (None if localctx.e is None else
> > self._input.getText((localctx.e.start,localctx.e.stop))))
> > +                self.StorePredicateExpression((None if localctx.e is
> > + None else localctx.e.start).line, (None if localctx.e is None else
> > + localctx.e.start).column, (None if localctx.e is None else
> > + localctx.e.stop).line, (None if localctx.e is None else
> > + localctx.e.stop).column, (None if localctx.e is None else
> > + self._input.getText(localctx.e.start,localctx.e.stop)))
> >                  self.state = 847
> >                  self.statement()
> >                  self.state = 850
> > @@ -6144,7 +6146,7 @@ class CParser ( Parser ):
> >                  self.match(CParser.T__38)
> >                  self.state = 864
> >                  self.statement()
> > -                self.StorePredicateExpression((None if localctx.e is None else
> > localctx.e.start).line, (None if localctx.e is None else
> localctx.e.start).column,
> > (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is
> > None else localctx.e.stop).column, (None if localctx.e is None else
> > self._input.getText((localctx.e.start,localctx.e.stop))))
> > +                self.StorePredicateExpression((None if localctx.e is
> > + None else localctx.e.start).line, (None if localctx.e is None else
> > + localctx.e.start).column, (None if localctx.e is None else
> > + localctx.e.stop).line, (None if localctx.e is None else
> > + localctx.e.stop).column, (None if localctx.e is None else
> > + self._input.getText(localctx.e.start,localctx.e.stop)))
> >                  pass
> >              elif token in [CParser.T__87]:
> >                  self.enterOuterAlt(localctx, 2) @@ -6162,7 +6164,7 @@ class
> CParser
> > ( Parser ):
> >                  self.match(CParser.T__38)
> >                  self.state = 873
> >                  self.match(CParser.T__1)
> > -                self.StorePredicateExpression((None if localctx.e is None else
> > localctx.e.start).line, (None if localctx.e is None else
> localctx.e.start).column,
> > (None if localctx.e is None else localctx.e.stop).line, (None if localctx.e is
> > None else localctx.e.stop).column, (None if localctx.e is None else
> > self._input.getText((localctx.e.start,localctx.e.stop))))
> > +                self.StorePredicateExpression((None if localctx.e is
> > + None else localctx.e.start).line, (None if localctx.e is None else
> > + localctx.e.start).column, (None if localctx.e is None else
> > + localctx.e.stop).line, (None if localctx.e is None else
> > + localctx.e.stop).column, (None if localctx.e is None else
> > + self._input.getText(localctx.e.start,localctx.e.stop)))
> >                  pass
> >              else:
> >                  raise NoViableAltException(self)
> > --
> > 2.18.0.windows.1
> >
> >
> > 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [edk2-devel] [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci
  2020-06-04  5:38   ` Zhang, Shenglei
@ 2020-06-09 12:24     ` Laszlo Ersek
  0 siblings, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2020-06-09 12:24 UTC (permalink / raw)
  To: Zhang, Shenglei, devel@edk2.groups.io
  Cc: Feng, Bob C, Bret Barkelew, Kinney, Michael D, Gao, Liming,
	Sean Brogan

On 06/04/20 07:38, Zhang, Shenglei wrote:
> Hi Laszlo,
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, June 3, 2020 10:27 PM
>> To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zhang@intel.com>
>> Cc: Feng, Bob C <bob.c.feng@intel.com>; Bret Barkelew
>> <Bret.Barkelew@microsoft.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean
>> Brogan <sean.brogan@microsoft.com>
>> Subject: Re: [edk2-devel] [PATCH v2 0/5] Add a pipline to check Ecc issues for
>> edk2 on open ci
>>
>> On 06/03/20 10:48, Zhang, Shenglei wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2606
>>> As planed we will enable Ecc check for edk2 on open ci. And they are
>>> ready now, but these are V2 series. So I expect that contributors in
>>> edk2 community can try using this script when reviewing. And I appreciate
>>> receiving feedback and comments if someone find errors or false positive
>>> issues.
>>>
>>> I created a pipline of EccCheck for my forked edk2.
>>>
>> https://dev.azure.com/shengleizhang/shengleizhang/_build?definitionId=10
>> &_a=summary
>>>
>>> The patch series are big, so the commits are also pushed into my forked
>> tree.
>>> https://github.com/shenglei10/edk2/commits/ECC
>>>
>>> Patches
>>> 1/5: This is a patch to enable python 3.8 for Ecc. It is a tool issues not
>>>      a pipline or script issue. But it is listed here for people willing
>>>      to try this tool.
>>> 2/5: EccCheck.py is a tool to report Ecc issues for commits. It can be run
>>>      on azure servers for open ci, or locally. Its usage is like
>>>      PatchCheck.py.
>>> 3/5: It's a lib necessary for py3 to run Ecc on azure servers. For local
>>>      use, we need to type command
>>>      "py -3 -m pip install antlr4-python3-runtime" first.
>>> 4/5: Windows-EccCheck.yml is a yaml file to configure the newly added
>>>      pipline. The azure uses this to create a pipline.
>>> 5/5: We consider some cases that will report out Ecc issues but they won't
>>>      be fixed, like submodule and industry standard related things. So we
>>>      add two configuration fields "Exception" and "IgnoreFiles" for people
>>>      to use. The patch is a example and the contents in the fields will be
>>>      empty in final version.
>>>
>>> Cc: Bob Feng <bob.c.feng@intel.com>
>>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Cc: Sean Brogan <sean.brogan@microsoft.com>
>>>
>>> v2: Update 2/5, fix the bug that the script can't hanlde multiple commits.
>>
>> Thanks for the ExceptionList / IgnoreFiles features; I think they are
>> really important. I've run ECC in the past, and in some cases it is
>> *way* too strict and opinionated, so I'm sure we'll end up "training"
>> the ExceptionList entry for OvmfPkg.
>>
>> Can you please explain how (if?) ECC is restricted to new code added by
>> a patch series? Patch#2 seems related, but I don't fully understand. It
>> says,
>>
>> "It can only handle the issues, whose line number in CSV report
>> accurately map with their code in source code files."
>>
>> Does that mean that CI performs a full ECC check, but filters out all
>> warning / error messages that do not refer to code lines added in the
>> patch series?
>>
> 
> Yes, I think you get the point.
> Since there are plenty of Ecc issues existing in edk2, we need to filter out the
> issues that are not caused by the patch to be checked in. The actual implementation
> is to create a dictionary for modified files, in which the key word is the modified file and
> the content is the line number scope for added code. Then if the issues in CSV report can
> be mapped with the dictionary, they are marked as real issues.
> 
> And the scanning scope is the folder that the change is located in. For example,
> Someone modifies "FmpDevicePkg\Library\FmpDependencyLib\FmpDependencyLib.c".
> Then the scan scope is "FmpDevicePkg\Library\FmpDependencyLib". It's not a full check
> for edk2, because it will cost much time.

Thank you for the explanation!
Laszlo


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci
  2020-06-03  8:48 [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci Zhang, Shenglei
                   ` (5 preceding siblings ...)
  2020-06-03 14:27 ` [edk2-devel] [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci Laszlo Ersek
@ 2020-06-09 13:10 ` Liming Gao
  6 siblings, 0 replies; 12+ messages in thread
From: Liming Gao @ 2020-06-09 13:10 UTC (permalink / raw)
  To: Zhang, Shenglei, devel@edk2.groups.io
  Cc: Feng, Bob C, Bret Barkelew, Kinney, Michael D, Sean Brogan

Shenglei:
  Seemly, ECC tool update is not related to this patch serial. Can you separate it? 
  Besides, please update package yaml file with new ECC checker. 

Thanks
Liming
> -----Original Message-----
> From: Zhang, Shenglei <shenglei.zhang@intel.com>
> Sent: Wednesday, June 3, 2020 4:48 PM
> To: devel@edk2.groups.io
> Cc: Feng, Bob C <bob.c.feng@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>
> Subject: [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2606
> As planed we will enable Ecc check for edk2 on open ci. And they are
> ready now, but these are V2 series. So I expect that contributors in
> edk2 community can try using this script when reviewing. And I appreciate
> receiving feedback and comments if someone find errors or false positive
> issues.
> 
> I created a pipline of EccCheck for my forked edk2.
> https://dev.azure.com/shengleizhang/shengleizhang/_build?definitionId=10&_a=summary
> 
> The patch series are big, so the commits are also pushed into my forked tree.
> https://github.com/shenglei10/edk2/commits/ECC
> 
> Patches
> 1/5: This is a patch to enable python 3.8 for Ecc. It is a tool issues not
>      a pipline or script issue. But it is listed here for people willing
>      to try this tool.
> 2/5: EccCheck.py is a tool to report Ecc issues for commits. It can be run
>      on azure servers for open ci, or locally. Its usage is like
>      PatchCheck.py.
> 3/5: It's a lib necessary for py3 to run Ecc on azure servers. For local
>      use, we need to type command
>      "py -3 -m pip install antlr4-python3-runtime" first.
> 4/5: Windows-EccCheck.yml is a yaml file to configure the newly added
>      pipline. The azure uses this to create a pipline.
> 5/5: We consider some cases that will report out Ecc issues but they won't
>      be fixed, like submodule and industry standard related things. So we
>      add two configuration fields "Exception" and "IgnoreFiles" for people
>      to use. The patch is a example and the contents in the fields will be
>      empty in final version.
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> 
> v2: Update 2/5, fix the bug that the script can't hanlde multiple commits.
> 
> Fan, Zhiju (1):
>   BaseTools:ECC needs to update the contents of CParser4
> 
> Shenglei Zhang (4):
>   BaseTools/Scripts: Add EccCheck.py
>   pip-requirements.txt: Add Ecc required lib
>   .azurepiplines: Add a pipline to check ECC issues for commits
>   MdeModulePkg/MdeModulePkg.ci.yaml: Add configuration for Ecc check
> 
>  .azurepipelines/Windows-EccCheck.yml          |  38 ++
>  BaseTools/Scripts/EccCheck.py                 | 433 ++++++++++++++++++
>  .../Source/Python/Ecc/CParser4/CLexer.py      |   6 +-
>  .../Source/Python/Ecc/CParser4/CListener.py   |   4 +-
>  .../Source/Python/Ecc/CParser4/CParser.py     |  38 +-
>  MdeModulePkg/MdeModulePkg.ci.yaml             |   8 +
>  pip-requirements.txt                          |   1 +
>  7 files changed, 505 insertions(+), 23 deletions(-)
>  create mode 100644 .azurepipelines/Windows-EccCheck.yml
>  create mode 100644 BaseTools/Scripts/EccCheck.py
> 
> --
> 2.18.0.windows.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-06-09 13:10 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-03  8:48 [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci Zhang, Shenglei
2020-06-03  8:48 ` [PATCH v2 1/5] BaseTools:ECC needs to update the contents of CParser4 Zhang, Shenglei
2020-06-05  4:38   ` [edk2-devel] " Yuwei Chen
2020-06-05  5:11     ` Zhang, Shenglei
2020-06-03  8:48 ` [PATCH v2 2/5] BaseTools/Scripts: Add EccCheck.py Zhang, Shenglei
2020-06-03  8:48 ` [PATCH v2 3/5] pip-requirements.txt: Add Ecc required lib Zhang, Shenglei
2020-06-03  8:48 ` [PATCH v2 4/5] .azurepiplines: Add a pipline to check ECC issues for commits Zhang, Shenglei
2020-06-03  8:48 ` [PATCH v2 5/5] MdeModulePkg/MdeModulePkg.ci.yaml: Add configuration for Ecc check Zhang, Shenglei
2020-06-03 14:27 ` [edk2-devel] [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci Laszlo Ersek
2020-06-04  5:38   ` Zhang, Shenglei
2020-06-09 12:24     ` Laszlo Ersek
2020-06-09 13:10 ` Liming Gao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox