Re: [edk2-devel] [PATCH v7 00/16] Add a plugin to check Ecc issues for edk2 on open ci

["Leif Lindholm" <leif@nuviainc.com>]

On Tue, Jul 07, 2020 at 02:24:14 +0000, Zhang, Shenglei wrote:
> Comments below.
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif
> > Lindholm
> > Sent: Monday, July 6, 2020 4:55 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 v7 00/16] Add a plugin to check Ecc issues
> > for edk2 on open ci
> > 
> > On Mon, Jul 06, 2020 at 16:48:30 +0800, 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. I appreciate receiving feedback and comments if someone
> > > find errors or false positive issues.
> > >
> > > I created a pipline of EccCheck for my forked edk2. Welcome everyone to
> > > create pull request to test the quality of this plugin.
> > > My forked tree: https://github.com/shenglei10/edk2
> > >
> > > And I also created some test cases for ECC plugin. Below are test cases.
> > > https://github.com/shenglei10/edk2/tree/ECC
> > > Results can be view in below azure server.
> > >
> > https://dev.azure.com/shengleizhang/shengleizhang/_build?definitionId=12
> > &_a=summary
> > >
> > > Patches
> > > 1/16: It's a lib necessary for py3 to run Ecc on azure servers.
> > >
> > > 2/16: EccCheck.py is a plugin to report Ecc issues for commits. It can be run
> > >      on azure servers for open ci, or a local virtual environment.
> > >
> > > 3/16~16/16: 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. These patches add configuration in yaml files for Ecc check.
> > >
> > > 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 1/17, fix the bug that the script can't hanlde multiple commits.
> > >
> > > v3: Update 1/17, set the only workalbe workspace is edk2 root directory.
> > >     Update 2/17, designate the version of antlr4 is 4.7.1.
> > >     Add 4/17~17/17.
> > >
> > > v4. Update 1/17, remove the function EdksetupRebuild(), instead add
> > >     function SetupEnvironment(). Update variables' format and type hints
> > >     to pass flake8 and mypy.
> > >
> > > v5. Conver the former method to plugin solution, to align with
> > >     other check points on open ci.
> > >
> > > v6. The 1/16 patch is missed in v5 series. Now add it in v6.
> > >
> > > v7. Fix a bug that Ecc plugin can not be run correctly under Linux OS.
> > 
> > What is the bug? Where is it fixed?
> 
> V6 plugin pipeline link:
> https://dev.azure.com/shengleizhang/shengleizhang/_build/results?buildId=282&view=logs&j=9b8d87a5-bd93-5358-8ffa-588e312c4376&t=282a77bb-da29-5801-6299-2bbe15d5aabb
> v7 plugin pipeline link:
> https://dev.azure.com/shengleizhang/shengleizhang/_build/results?buildId=316&view=logs&j=5e3952f2-9c8a-5394-82c1-a803bcab3ca3&t=7ea9e85e-5c3d-5fcf-865e-8449f16cc745
> 
> For the same pull request, v6 plugin can't report out the Ecc issues under Linux OS.

Right. So *what* was the bug and *where* (which patch or patches) was
it fixed? This is information that should (in an abbreviated form) be
provided in the cover letter.

"Fix a bug" is data, not information. The same is true for "can not be
run correctly". And neither tells me which patch I should look at.
If this has been covered in a specific discussion thread, a link to
the thread *and* mentioning which patch(es) it affects is sufficient.

Regards,

Leif

> 
> Thanks,
> Shenglei
> 
> > 
> > /
> >     Leif
> > 
> > > Shenglei Zhang (16):
> > >   pip-requirements.txt: Add Ecc required lib
> > >   .pytool/Plugin: Add a plugin EccCheck
> > >   MdeModulePkg/MdeModulePkg.ci.yaml: Add configuration for Ecc check
> > >   ArmVirtPkg/ArmVirtPkg.ci.yaml: Add configuration for Ecc check
> > >   CryptoPkg/CryptoPkg.ci.yaml: Add configuration for Ecc check
> > >   EmulatorPkg/EmulatorPkg.ci.yaml: Add configuration for Ecc check
> > >   FatPkg/FatPkg.ci.yaml: Add configuration for Ecc check
> > >   FmpDevicePkg/FmpDevicePkg.ci.yaml: Add configuration for Ecc check
> > >   MdePkg/MdePkg.ci.yaml: Add configuration for Ecc check
> > >   NetworkPkg/NetworkPkg.ci.yaml: Add configuration for Ecc check
> > >   OvmfPkg/OvmfPkg.ci.yaml: Add configuration for Ecc check
> > >   PcAtChipsetPkg/PcAtChipsetPkg.ci.yaml: Add configuration for Ecc check
> > >   SecurityPkg/SecurityPkg.ci.yaml: Add configuration for Ecc check
> > >   ShellPkg/ShellPkg.ci.yaml: Add configuration for Ecc check
> > >   UefiCpuPkg/UefiCpuPkg.ci.yaml: Add configuration for Ecc check
> > >   UnitTestFrameworkPkg: Add configuration for Ecc check in yaml file
> > >
> > >  .pytool/Plugin/EccCheck/EccCheck.py           | 267 ++++++++++++++++++
> > >  .pytool/Plugin/EccCheck/EccCheck_plug_in.yaml |  11 +
> > >  .pytool/Plugin/EccCheck/Readme.md             |  15 +
> > >  ArmVirtPkg/ArmVirtPkg.ci.yaml                 |  11 +
> > >  CryptoPkg/CryptoPkg.ci.yaml                   |  11 +
> > >  EmulatorPkg/EmulatorPkg.ci.yaml               |  11 +
> > >  FatPkg/FatPkg.ci.yaml                         |  11 +
> > >  FmpDevicePkg/FmpDevicePkg.ci.yaml             |  11 +
> > >  MdeModulePkg/MdeModulePkg.ci.yaml             |  11 +
> > >  MdePkg/MdePkg.ci.yaml                         |  11 +
> > >  NetworkPkg/NetworkPkg.ci.yaml                 |  11 +
> > >  OvmfPkg/OvmfPkg.ci.yaml                       |  11 +
> > >  PcAtChipsetPkg/PcAtChipsetPkg.ci.yaml         |  11 +
> > >  SecurityPkg/SecurityPkg.ci.yaml               |  11 +
> > >  ShellPkg/ShellPkg.ci.yaml                     |  11 +
> > >  UefiCpuPkg/UefiCpuPkg.ci.yaml                 |  11 +
> > >  .../UnitTestFrameworkPkg.ci.yaml              |  10 +
> > >  pip-requirements.txt                          |   1 +
> > >  18 files changed, 447 insertions(+)
> > >  create mode 100644 .pytool/Plugin/EccCheck/EccCheck.py
> > >  create mode 100644 .pytool/Plugin/EccCheck/EccCheck_plug_in.yaml
> > >  create mode 100644 .pytool/Plugin/EccCheck/Readme.md
> > >
> > > --
> > > 2.18.0.windows.1
> > >
> > >
> > >
> > >
> > 
> > 
>