Re: [edk2-devel] [PATCH 1/2] OvmfPkg: start using the ECC plugin exception list

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

On 12/04/20 05:05, Sean Brogan wrote:
> 3 things
> 
> 1. I would support disabling any ci plugins that can be destructive to
> your local environment.  I think EccCheck needs to be re-evaluated
> regarding how it gets the change set.  What it does now is not a pattern
> I have encouraged and although i understand the desire (for server ci) i
> think it has caused more trouble than it is worth.

Thanks. To be honest, I called ECC "fascist", when I went over the
report it generated. It's insane. It makes me want to quit programming.
And I'm the reviewer guy that forces contributors to conform to the
coding style in minute detail. ECC is unbearable.

> 
> 2. Not sure if license check has same problem or not.

My understanding is that, for a while now, it has been technically
impossible to contribute code under any other license than
BSD-2-Clause-Patent. I think Leif had some serious thoughts that the
limitation wasn't only technical but even legal. I forget the details.

> 
> 
> 
> On 12/3/2020 7:21 PM, Laszlo Ersek wrote:
>> In the recent past, ECC has wreaked havoc at least twice:
>>
>> - rejected Tom Lendacky's perfectly valid and clean code:
>>
>>    https://bugzilla.tianocore.org/show_bug.cgi?id=3008#c5
>>    https://edk2.groups.io/g/devel/message/67097
>>
>> - rejected James Bottomley's series for bogus reasons:
>>
>>    https://bugzilla.tianocore.org/show_bug.cgi?id=3077#c4
>>    https://edk2.groups.io/g/devel/message/68302
>>
>> There isn't capacity to improve ECC:
>>
>> - Liming filed an ECC bug about the first case noted above, but it has
>>    received no feedback in 25 days (as of this writing):
>>
>>    https://bugzilla.tianocore.org/show_bug.cgi?id=3060
>>
>> And running CI or ECC on developer machines is difficult:
>>
>> - I had to set up a separate VM for it,
>>
>> - Shenglei gave Windows-based usage instructions only,
>>
>>    https://edk2.groups.io/g/devel/message/61966
>>
>> - experimenting with a CI outside of a VM is somewhat risky:
>>
>>    https://github.com/tianocore/edk2-pytool-extensions/issues/231
> 
> 3. Running CI locally should not be "somewhat risky".  More work needs
> to be done to identify the root cause of the above behavior but my guess
> is that it has to do with EccCheck and nothing to do with
> pytool-extensions.

Sorry, I guess I mixed up my references a little bit. I consider running
binaries downloaded from the internet risky (except from the official
repos of my Linux distro(s)). But that's indeed a different topic and I
shouldn't have generalized. Sorry about that.

Thanks for commenting!
Laszlo

> 
> 
>>
>> ECC should be considered an experimental tool; I agreed to its enablement
>> in CI specifically because we were offered an exception list:
>>
>>    https://edk2.groups.io/g/devel/message/60961
>>   
>> https://www.redhat.com/archives/edk2-devel-archive/2020-June/msg00473.html
>>
>>
>> The github.com-based pull request process is already very inefficient for
>> both contributors and maintainers; it's time to put the ECC exception
>> list
>> to use.
>>
>> Now, I've tried adding those error codes that were reported against
>> James's series (after evaluating each entry in the report at
>> <https://dev.azure.com/tianocore/11ea4a10-ac9f-4e5f-8b13-7def1f19d478/_apis/build/builds/16071/logs/122>),
>>
>> namely 4001, 4002, 5007, 8001, 8004, 8005. However, to my horror,
>> "EccCheck.ExceptionList" only supports the following format:
>>
>>    "<ErrorID>", "<KeyWord>", "<ErrorID>", "<KeyWord>", ...
>>
>> where each pair binds a particular error ID to a particular "offending"
>> identifier, such as C variable name. There is no wildcard support, so
>> it's
>> impossible to disable entire classes of ECC reports.
>>
>> Therefore, I have to use "EccCheck.IgnoreFiles". It also doesn't support
>> the "." subdirectory or the "*" wildcard. But, with some sweat, I can
>> still use it to disable ECC for all of OvmfPkg.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>   OvmfPkg/OvmfPkg.ci.yaml | 49 ++++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkg.ci.yaml b/OvmfPkg/OvmfPkg.ci.yaml
>> index 3128aefe9ed1..68d2de704d19 100644
>> --- a/OvmfPkg/OvmfPkg.ci.yaml
>> +++ b/OvmfPkg/OvmfPkg.ci.yaml
>> @@ -22,6 +22,55 @@
>>           ],
>>           ## Both file path and directory path are accepted.
>>           "IgnoreFiles": [
>> +            "8254TimerDxe",
>> +            "8259InterruptControllerDxe",
>> +            "AcpiPlatformDxe",
>> +            "AcpiTables",
>> +            "AmdSev",
>> +            "AmdSevDxe",
>> +            "Bhyve",
>> +            "CompatImageLoaderDxe",
>> +            "CpuHotplugSmm",
>> +            "CpuS3DataDxe",
>> +            "Csm",
>> +            "EmuVariableFvbRuntimeDxe",
>> +            "EnrollDefaultKeys",
>> +            "Include",
>> +            "IncompatiblePciDeviceSupportDxe",
>> +            "IoMmuDxe",
>> +            "Library",
>> +            "LinuxInitrdDynamicShellCommand",
>> +            "LsiScsiDxe",
>> +            "MptScsiDxe",
>> +            "OvmfXenElfHeaderGenerator.c",
>> +            "PciHotPlugInitDxe",
>> +            "PlatformDxe",
>> +            "PlatformPei",
>> +            "PvScsiDxe",
>> +            "QemuFlashFvbServicesRuntimeDxe",
>> +            "QemuKernelLoaderFsDxe",
>> +            "QemuRamfbDxe",
>> +            "QemuVideoDxe",
>> +            "SataControllerDxe",
>> +            "Sec",
>> +            "SioBusDxe",
>> +            "SmbiosPlatformDxe",
>> +            "SmmAccess",
>> +            "SmmControl2Dxe",
>> +            "Tcg",
>> +            "Virtio10Dxe",
>> +            "VirtioBlkDxe",
>> +            "VirtioGpuDxe",
>> +            "VirtioNetDxe",
>> +            "VirtioPciDeviceDxe",
>> +            "VirtioRngDxe",
>> +            "VirtioScsiDxe",
>> +            "XenBusDxe",
>> +            "XenIoPciDxe",
>> +            "XenIoPvhDxe",
>> +            "XenPlatformPei",
>> +            "XenPvBlkDxe",
>> +            "XenTimerDxe"
>>           ]
>>       },
>>       ## options defined .pytool/Plugin/CompilerPlugin
>>
> 
> Thanks
> Sean
>