Re: [edk2-devel] [PATCH v2 0/5] Add a pipline to check Ecc issues for edk2 on open ci

["Laszlo Ersek" <lersek@redhat.com>]

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