[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 at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-