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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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
>