[PATCH 0/2] OvmfPkg: CI tweaks

[PATCH 0/2] OvmfPkg: CI tweaks

[Laszlo Ersek]

Repo:   https://pagure.io/lersek/edk2.git
Branch: ovmf_ci_tweaks

Please see the individual commit messages.

Thanks
Laszlo

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>

Laszlo Ersek (2):
  OvmfPkg: start using the ECC plugin exception list
  OvmfPkg: add "gGrubFileGuid=Grub" to GuidCheck.IgnoreDuplicates

 OvmfPkg/OvmfPkg.ci.yaml | 51 +++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

-- 
2.19.1.3.g30247aa5d201

[PATCH 1/2] OvmfPkg: start using the ECC plugin exception list

[Laszlo Ersek]

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

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
-- 
2.19.1.3.g30247aa5d201

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

[Sean]

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.

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



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.


> 
> 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

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

[Laszlo Ersek]

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
> 

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

[Laszlo Ersek]

Hi Sean,

On 12/04/20 16:22, Laszlo Ersek wrote:
> On 12/04/20 05:05, Sean Brogan wrote:

>> 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.

If you have a suggestion to improve the wording here, I'd like to hear
that. I'd really like to go ahead with this patch set in one way or
another, as it's blocking James's work from being merged. I don't want
to merge a commit message here that you find offensive or just plain
wrong though, so please suggest an improvement.

Ard, do you have any comments please?

Thanks
Laszlo

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

[Ard Biesheuvel]

On 12/4/20 4:36 PM, Laszlo Ersek wrote:
> Hi Sean,
> 
> On 12/04/20 16:22, Laszlo Ersek wrote:
>> On 12/04/20 05:05, Sean Brogan wrote:
> 
>>> 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.
> 
> If you have a suggestion to improve the wording here, I'd like to hear
> that. I'd really like to go ahead with this patch set in one way or
> another, as it's blocking James's work from being merged. I don't want
> to merge a commit message here that you find offensive or just plain
> wrong though, so please suggest an improvement.
> 
> Ard, do you have any comments please?
> 

I appreciate your tendency to document things profusely, but in this
case, I think it is sufficient to simply mention that ECC is overly
strict, and that it should not be left up to 'the machine' to decide
whether an exception can be made. We are all bandwidth constrained, and
reviewing is enough of an effort as it is without having to obsess about
details that some of us may not even notice.

So for for the changes

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

but obviously, we need a way for maintainers to overrule this behavior
without being forced to check in metadata files left and right.

Could we perhaps use a special tag? Or simply overrule ECC if the patch
in question has a Reviewed-by from the maintainer (*not* from a
reviewer) of the package in question?

As for the 'risky' - I agree that it is likely to misunderstood, so
better find a different word to describe this.

-- 
Ard.

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

[Laszlo Ersek]

On 12/04/20 17:05, Ard Biesheuvel wrote:
> On 12/4/20 4:36 PM, Laszlo Ersek wrote:
>> Hi Sean,
>>
>> On 12/04/20 16:22, Laszlo Ersek wrote:
>>> On 12/04/20 05:05, Sean Brogan wrote:
>>
>>>> 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.
>>
>> If you have a suggestion to improve the wording here, I'd like to hear
>> that. I'd really like to go ahead with this patch set in one way or
>> another, as it's blocking James's work from being merged. I don't want
>> to merge a commit message here that you find offensive or just plain
>> wrong though, so please suggest an improvement.
>>
>> Ard, do you have any comments please?
>>
> 
> I appreciate your tendency to document things profusely,

I haven't forgotten that you don't like my overlong commit messages. In
this case, I diverged because I expected fierce resistance from
contributors that like ECC, and figured I'd bring the evidence in advance.

> but in this
> case, I think it is sufficient to simply mention that ECC is overly
> strict, and that it should not be left up to 'the machine' to decide
> whether an exception can be made. We are all bandwidth constrained, and
> reviewing is enough of an effort as it is without having to obsess about
> details that some of us may not even notice.
> 
> So for for the changes
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> but obviously, we need a way for maintainers to overrule this behavior
> without being forced to check in metadata files left and right.

100% this!

Worse -- if I understand correctly! -- such CI config changes don't even
take effect for a patch series if they are themselves part of the
series. So it's not like I can just prepend such a patch to a series
that I'm about to merge but ECC doesn't like -- I need to get the CI
config changes reviewed and merged *separately*. Tremendous waste of time.

> 
> Could we perhaps use a special tag? Or simply overrule ECC if the patch
> in question has a Reviewed-by from the maintainer (*not* from a
> reviewer) of the package in question?
> 
> As for the 'risky' - I agree that it is likely to misunderstood, so
> better find a different word to describe this.
> 

Yeah, let me drop this one patch and see if we can disable ECC globally,
or implement a github label to disable it.

James, is it OK if we delay merging of your v3 series a bit?

Ard, does your R-b apply to the second patch (including its commit
message)? GuidCheck is a useful plugin, and the exception is indeed by
design.

... I would still much prefer of course if that patch (= the exception
to GuidCheck) could simply be included in James's series.

Thanks,
Laszlo

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

[James Bottomley]

On Tue, 2020-12-08 at 02:56 +0100, Laszlo Ersek wrote:
> On 12/04/20 17:05, Ard Biesheuvel wrote:
[...]
> > Could we perhaps use a special tag? Or simply overrule ECC if the
> > patch in question has a Reviewed-by from the maintainer (*not* from
> > a reviewer) of the package in question?
> > 
> > As for the 'risky' - I agree that it is likely to misunderstood, so
> > better find a different word to describe this.
> > 
> 
> Yeah, let me drop this one patch and see if we can disable ECC
> globally, or implement a github label to disable it.
> 
> James, is it OK if we delay merging of your v3 series a bit?

Sure, there's nothing tremendously urgent.  I'm about to post the qemu
enabling patch, but as long as the GUIDs don't change (or the format of
the reset block) a delay shouldn't matter.

James

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

[Ard Biesheuvel]

On 12/8/20 2:56 AM, Laszlo Ersek wrote:
> On 12/04/20 17:05, Ard Biesheuvel wrote:
>> On 12/4/20 4:36 PM, Laszlo Ersek wrote:
>>> Hi Sean,
>>>
>>> On 12/04/20 16:22, Laszlo Ersek wrote:
>>>> On 12/04/20 05:05, Sean Brogan wrote:
>>>
>>>>> 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.
>>>
>>> If you have a suggestion to improve the wording here, I'd like to hear
>>> that. I'd really like to go ahead with this patch set in one way or
>>> another, as it's blocking James's work from being merged. I don't want
>>> to merge a commit message here that you find offensive or just plain
>>> wrong though, so please suggest an improvement.
>>>
>>> Ard, do you have any comments please?
>>>
>>
>> I appreciate your tendency to document things profusely,
> 
> I haven't forgotten that you don't like my overlong commit messages. In
> this case, I diverged because I expected fierce resistance from
> contributors that like ECC, and figured I'd bring the evidence in advance.
> 
>> but in this
>> case, I think it is sufficient to simply mention that ECC is overly
>> strict, and that it should not be left up to 'the machine' to decide
>> whether an exception can be made. We are all bandwidth constrained, and
>> reviewing is enough of an effort as it is without having to obsess about
>> details that some of us may not even notice.
>>
>> So for for the changes
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>
>> but obviously, we need a way for maintainers to overrule this behavior
>> without being forced to check in metadata files left and right.
> 
> 100% this!
> 
> Worse -- if I understand correctly! -- such CI config changes don't even
> take effect for a patch series if they are themselves part of the
> series. So it's not like I can just prepend such a patch to a series
> that I'm about to merge but ECC doesn't like -- I need to get the CI
> config changes reviewed and merged *separately*. Tremendous waste of time.
> 
>>
>> Could we perhaps use a special tag? Or simply overrule ECC if the patch
>> in question has a Reviewed-by from the maintainer (*not* from a
>> reviewer) of the package in question?
>>
>> As for the 'risky' - I agree that it is likely to misunderstood, so
>> better find a different word to describe this.
>>
> 
> Yeah, let me drop this one patch and see if we can disable ECC globally,
> or implement a github label to disable it.
> 
> James, is it OK if we delay merging of your v3 series a bit?
> 
> Ard, does your R-b apply to the second patch (including its commit
> message)? GuidCheck is a useful plugin, and the exception is indeed by
> design.
> 

Yes.

-- 
Ard.

> ... I would still much prefer of course if that patch (= the exception
> to GuidCheck) could simply be included in James's series.
> 
> Thanks,
> Laszlo
> 

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

[Sean]

Laszlo,

Trying to understand this.


On 12/7/2020 5:56 PM, Laszlo Ersek wrote:
> ... I would still much prefer of course if that patch (= the exception
> to GuidCheck) could simply be included in James's series.


The PR based CI runs on the entire series.  It does not run on 
individual commits and thus you can add this to the series and in fact i 
would suggest it get added to the series.  Any changes in the series 
will take effect when running the CI.

This is great for how Project Mu uses "PR gates" because we squash merge 
but in Edk2 with a patch series this can mean that commits in the middle 
can break things.  It is on developer and reviewer to catch those types 
of things.

For this case why can't this change be part of the commit that 
introduces the guid/global?

Or if that is undesirable you should be able to add the ignore in a 
commit prior to introduction and then you would never have a break. 
Either way there is no reason this isn't part of a single series.

Thanks
Sean

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

[Laszlo Ersek]

On 12/08/20 19:45, Sean Brogan wrote:
> Laszlo,
> 
> Trying to understand this.
> 
> 
> On 12/7/2020 5:56 PM, Laszlo Ersek wrote:
>> ... I would still much prefer of course if that patch (= the exception
>> to GuidCheck) could simply be included in James's series.
> 
> 
> The PR based CI runs on the entire series.  It does not run on
> individual commits and thus you can add this to the series and in fact i
> would suggest it get added to the series.  Any changes in the series
> will take effect when running the CI.

That sounds great, thank you. I must have misunderstood something in the
past. I distinctly remember coming away from a discussion somewhere with
the lesson that CI tweaking had to be done in a separate PR.

Perhaps I mistook an explanation. I guess my mental image was that the
CI run started on "master", grabbing its config from the files in
"master", and then checking out or otherwise applying the patches for
the subject series. If CI indeed *launches* while standing at the HEAD
of the topic branch, then that's great.

(I know from my local CI experimentation that it just runs on whatever
commit I have checked out, modulo uncommitted changes per
<https://bugzilla.tianocore.org/show_bug.cgi?id=2986> -- but I didn't
know if the CI environment inside github / azure was the same.)

> 
> This is great for how Project Mu uses "PR gates" because we squash merge
> but in Edk2 with a patch series this can mean that commits in the middle
> can break things.  It is on developer and reviewer to catch those types
> of things.
> 
> For this case why can't this change be part of the commit that
> introduces the guid/global?

I agree that it should be.

> 
> Or if that is undesirable you should be able to add the ignore in a
> commit prior to introduction and then you would never have a break.
> Either way there is no reason this isn't part of a single series.

Sounds great, thank you. I actually prefer it to be part of the same commit.

Here's what I'm proposing / requesting:

(1) Sean, could you please (pretty please :) ) submit the second patch

  Disable EccCheck for OvmfPkg CI

from your demo PR at

  https://edk2.groups.io/g/devel/message/68541
  https://github.com/tianocore/edk2/pull/1201

stand-alone to the list, so that Ard or myself can ACK it and merge it
separately?

(2) Subsequently, I'm going to take the 2nd patch of the present series,
to which Ard's R-b applies as well:

  https://edk2.groups.io/g/devel/message/68451

and I'll *squash it* into James's

  [PATCH v3 3/6] OvmfPkg/AmdSev: add Grub Firmware Volume Package

And then I'll merge that v3 series.

Thank you!
Laszlo

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

[Sean]

I would also agree with Ard about shortening and simplifying the commit 
message if this commit is to go forward.

As a FYI the pytools issue you link to for ci comment is closed as "wont 
fix".  That doesn't change the fact that Edk2 CI runs an edk2 plugin 
that does potentially bad things to your local workspace and if your 
environment is configured in unexpected ways the plugin causes even more 
damage.

More importantly instead of this commit i ask if the community should 
have a quick value prop discussion of EccCheck and if in its current 
form needs changes or to be disabled...then that would be the change 
rather than this commit.  I am generally a fan of automation and tools 
based validation for code formatting but there has been a lot of noise 
with this one so it might not yet be ready to be a PR blocker.

Personally, related to code formatting/conventions i would much much 
rather see the community agree to a profile in clang-format or something 
similar and then just run the tool on all files in the tree and commit 
the changes.  This might mean we have to change a few things as i 
haven't been able to get clang-format to match exactly...but in the end 
auto formatting is in my opinion a better path forward than home grown 
tools to "enforce" formatting.  Auto formatting could be easily enforced 
in CI and is easy/nearly free for a contributor to resolve and help the 
community create consistent code.  I know its not perfect but it gets 
you 95% of the way without huge investment.


Thanks
Sean


On 12/4/2020 7:36 AM, Laszlo Ersek wrote:
> Hi Sean,
> 
> On 12/04/20 16:22, Laszlo Ersek wrote:
>> On 12/04/20 05:05, Sean Brogan wrote:
> 
>>> 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.
> 
> If you have a suggestion to improve the wording here, I'd like to hear
> that. I'd really like to go ahead with this patch set in one way or
> another, as it's blocking James's work from being merged. I don't want
> to merge a commit message here that you find offensive or just plain
> wrong though, so please suggest an improvement.
> 
> Ard, do you have any comments please?
> 
> Thanks
> Laszlo
> 

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

[Laszlo Ersek]

On 12/04/20 19:28, Sean Brogan wrote:
> I would also agree with Ard about shortening and simplifying the commit
> message if this commit is to go forward.
> 
> As a FYI the pytools issue you link to for ci comment is closed as "wont
> fix".  That doesn't change the fact that Edk2 CI runs an edk2 plugin
> that does potentially bad things to your local workspace and if your
> environment is configured in unexpected ways the plugin causes even more
> damage.
> 
> More importantly instead of this commit i ask if the community should
> have a quick value prop discussion of EccCheck and if in its current
> form needs changes or to be disabled...then that would be the change
> rather than this commit.

- Disabling ECC globally: yes please. Much better idea than this patch.

- "quick" value prop discussion: my fear was (and is) that it would not
be "quick"; I foresee either a heated debate or a drawn-out indecisive
process. Meanwhile good patches would still be held back in either case,
which is the only thing I really care about here. So if "quick" is
indeed quick, then I'm OK; I just figured the really quick way would be
this patch.


> I am generally a fan of automation and tools
> based validation for code formatting but there has been a lot of noise
> with this one so it might not yet be ready to be a PR blocker.

Agreed.

> 
> Personally, related to code formatting/conventions i would much much
> rather see the community agree to a profile in clang-format or something
> similar and then just run the tool on all files in the tree and commit
> the changes.  This might mean we have to change a few things as i
> haven't been able to get clang-format to match exactly...but in the end
> auto formatting is in my opinion a better path forward than home grown
> tools to "enforce" formatting.  Auto formatting could be easily enforced
> in CI and is easy/nearly free for a contributor to resolve and help the
> community create consistent code.  I know its not perfect but it gets
> you 95% of the way without huge investment.

I have no experience with auto-formatting. I'm not a fan of sweeping
changes (huge conversion commits). It's unclear how the above would
differ from ECC "policy wise" -- it would be a different set of
formatting opinions, yes, but it would still be a tool to enforce them,
with unclear options for exceptions.

I'd suggest splitting the two: first let's disable ECC (or make it
opt-in -- and not by metafile commits, but by github pull request
labels!), and second investigate auto-formatting (also as an opt-in, so
we could get a taste).

Thanks!
Laszlo

[PATCH 2/2] OvmfPkg: add "gGrubFileGuid=Grub" to GuidCheck.IgnoreDuplicates

[Laszlo Ersek]

The patch series linked at

  https://bugzilla.tianocore.org/show_bug.cgi?id=3077#c4

is rejected by GuidCheck with the following error message:

> GUID: B5AE312C-BC8A-43B1-9C62-EBB826DD5D07
> NAME: gGrubFileGuid FILE: /home/vsts/work/1/s/OvmfPkg/OvmfPkg.dec
> GUID: B5AE312C-BC8A-43B1-9C62-EBB826DD5D07
> NAME: Grub FILE: /home/vsts/work/1/s/OvmfPkg/AmdSev/Grub/Grub.inf

This GUID identity is by design in the TianoCore#3077 feature; add a CI
exception for it.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/OvmfPkg.ci.yaml b/OvmfPkg/OvmfPkg.ci.yaml
index 68d2de704d19..8e2da2803b30 100644
--- a/OvmfPkg/OvmfPkg.ci.yaml
+++ b/OvmfPkg/OvmfPkg.ci.yaml
@@ -127,7 +127,7 @@
         "IgnoreGuidName": ["ResetVector", "XenResetVector"], # Expected duplication for gEfiFirmwareVolumeTopFileGuid
         "IgnoreGuidValue": [],
         "IgnoreFoldersAndFiles": [],
-        "IgnoreDuplicates": [],
+        "IgnoreDuplicates": ["gGrubFileGuid=Grub"],
     },
 
     ## options defined .pytool/Plugin/LibraryClassCheck
-- 
2.19.1.3.g30247aa5d201

Re: [PATCH 2/2] OvmfPkg: add "gGrubFileGuid=Grub" to GuidCheck.IgnoreDuplicates

[Philippe Mathieu-Daudé]

On 12/4/20 4:21 AM, Laszlo Ersek wrote:
> The patch series linked at
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=3077#c4
> 
> is rejected by GuidCheck with the following error message:
> 
>> GUID: B5AE312C-BC8A-43B1-9C62-EBB826DD5D07
>> NAME: gGrubFileGuid FILE: /home/vsts/work/1/s/OvmfPkg/OvmfPkg.dec
>> GUID: B5AE312C-BC8A-43B1-9C62-EBB826DD5D07
>> NAME: Grub FILE: /home/vsts/work/1/s/OvmfPkg/AmdSev/Grub/Grub.inf
> 
> This GUID identity is by design in the TianoCore#3077 feature; add a CI
> exception for it.
> 
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>