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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68316): https://edk2.groups.io/g/devel/message/68316
Mute This Topic: https://groups.io/mt/78702238/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68319): https://edk2.groups.io/g/devel/message/68319 Mute This Topic: https://groups.io/mt/78702238/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68348): https://edk2.groups.io/g/devel/message/68348 Mute This Topic: https://groups.io/mt/78702238/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68349): https://edk2.groups.io/g/devel/message/68349 Mute This Topic: https://groups.io/mt/78702238/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68352): https://edk2.groups.io/g/devel/message/68352 Mute This Topic: https://groups.io/mt/78702238/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68412): https://edk2.groups.io/g/devel/message/68412 Mute This Topic: https://groups.io/mt/78702238/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68481): https://edk2.groups.io/g/devel/message/68481 Mute This Topic: https://groups.io/mt/78702238/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68638): https://edk2.groups.io/g/devel/message/68638 Mute This Topic: https://groups.io/mt/78702238/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68451): https://edk2.groups.io/g/devel/message/68451 Mute This Topic: https://groups.io/mt/78702238/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68416): https://edk2.groups.io/g/devel/message/68416 Mute This Topic: https://groups.io/mt/78702238/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68354): https://edk2.groups.io/g/devel/message/68354 Mute This Topic: https://groups.io/mt/78702238/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68411): https://edk2.groups.io/g/devel/message/68411 Mute This Topic: https://groups.io/mt/78702238/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.