From: Michael Kubacki <michael.kubacki@microsoft.com>

Audit mode was enabled for the spellcheck CI plugin. It is no longer
needed with recent changes. Spelling errors can be checked in the
package moving forward.

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 ArmPkg/ArmPkg.ci.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
index c8dface6821a..a304c7966cf7 100644
--- a/ArmPkg/ArmPkg.ci.yaml
+++ b/ArmPkg/ArmPkg.ci.yaml
@@ -87,7 +87,7 @@
 
     ## options defined .pytool/Plugin/SpellCheck
     "SpellCheck": {
-        "AuditOnly": True,
+        "AuditOnly": False,
         "IgnoreFiles": [
             "Library/ArmSoftFloatLib/berkeley-softfloat-3/**"
         ],                           # use gitignore syntax to ignore errors
@@ -148,6 +148,7 @@
           "fcmplt",
           "ffreestanding",
           "frsub",
+          "hauser",
           "hisilicon",
           "iccabpr",
           "iccbpr",
-- 
2.28.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97394): https://edk2.groups.io/g/devel/message/97394
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, 14 Dec 2022 at 23:53, <mikuback@linux.microsoft.com> wrote:
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> Audit mode was enabled for the spellcheck CI plugin. It is no longer
> needed with recent changes. Spelling errors can be checked in the
> package moving forward.
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Will this patch result in PRs potentially being rejected by pre-merge
CI due to trivial spelling errors?


> ---
>  ArmPkg/ArmPkg.ci.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
> index c8dface6821a..a304c7966cf7 100644
> --- a/ArmPkg/ArmPkg.ci.yaml
> +++ b/ArmPkg/ArmPkg.ci.yaml
> @@ -87,7 +87,7 @@
>
>      ## options defined .pytool/Plugin/SpellCheck
>      "SpellCheck": {
> -        "AuditOnly": True,
> +        "AuditOnly": False,
>          "IgnoreFiles": [
>              "Library/ArmSoftFloatLib/berkeley-softfloat-3/**"
>          ],                           # use gitignore syntax to ignore errors
> @@ -148,6 +148,7 @@
>            "fcmplt",
>            "ffreestanding",
>            "frsub",
> +          "hauser",
>            "hisilicon",
>            "iccabpr",
>            "iccbpr",
> --
> 2.28.0.windows.1
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97400): https://edk2.groups.io/g/devel/message/97400
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Yes. It will also reduce frequency of incoming patches that must be 
reviewed and merged due to people continuously fixing trivial spelling 
errors.

On 12/14/2022 6:07 PM, Ard Biesheuvel wrote:
> On Wed, 14 Dec 2022 at 23:53, <mikuback@linux.microsoft.com> wrote:
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Audit mode was enabled for the spellcheck CI plugin. It is no longer
>> needed with recent changes. Spelling errors can be checked in the
>> package moving forward.
>>
>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Will this patch result in PRs potentially being rejected by pre-merge
> CI due to trivial spelling errors?
> 
> 
>> ---
>>   ArmPkg/ArmPkg.ci.yaml | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
>> index c8dface6821a..a304c7966cf7 100644
>> --- a/ArmPkg/ArmPkg.ci.yaml
>> +++ b/ArmPkg/ArmPkg.ci.yaml
>> @@ -87,7 +87,7 @@
>>
>>       ## options defined .pytool/Plugin/SpellCheck
>>       "SpellCheck": {
>> -        "AuditOnly": True,
>> +        "AuditOnly": False,
>>           "IgnoreFiles": [
>>               "Library/ArmSoftFloatLib/berkeley-softfloat-3/**"
>>           ],                           # use gitignore syntax to ignore errors
>> @@ -148,6 +148,7 @@
>>             "fcmplt",
>>             "ffreestanding",
>>             "frsub",
>> +          "hauser",
>>             "hisilicon",
>>             "iccabpr",
>>             "iccbpr",
>> --
>> 2.28.0.windows.1
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97402): https://edk2.groups.io/g/devel/message/97402
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, 15 Dec 2022 at 00:21, Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> Yes. It will also reduce frequency of incoming patches that must be
> reviewed and merged due to people continuously fixing trivial spelling
> errors.
>

In that case, NAK to this patch (and the ArmVirtPkg one). Unless we
add a button to the GitHub UI that permits me to override a negative
CI result on a PR.

> On 12/14/2022 6:07 PM, Ard Biesheuvel wrote:
> > On Wed, 14 Dec 2022 at 23:53, <mikuback@linux.microsoft.com> wrote:
> >>
> >> From: Michael Kubacki <michael.kubacki@microsoft.com>
> >>
> >> Audit mode was enabled for the spellcheck CI plugin. It is no longer
> >> needed with recent changes. Spelling errors can be checked in the
> >> package moving forward.
> >>
> >> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >> Cc: Sami Mujawar <sami.mujawar@arm.com>
> >> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> >
> > Will this patch result in PRs potentially being rejected by pre-merge
> > CI due to trivial spelling errors?
> >
> >
> >> ---
> >>   ArmPkg/ArmPkg.ci.yaml | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
> >> index c8dface6821a..a304c7966cf7 100644
> >> --- a/ArmPkg/ArmPkg.ci.yaml
> >> +++ b/ArmPkg/ArmPkg.ci.yaml
> >> @@ -87,7 +87,7 @@
> >>
> >>       ## options defined .pytool/Plugin/SpellCheck
> >>       "SpellCheck": {
> >> -        "AuditOnly": True,
> >> +        "AuditOnly": False,
> >>           "IgnoreFiles": [
> >>               "Library/ArmSoftFloatLib/berkeley-softfloat-3/**"
> >>           ],                           # use gitignore syntax to ignore errors
> >> @@ -148,6 +148,7 @@
> >>             "fcmplt",
> >>             "ffreestanding",
> >>             "frsub",
> >> +          "hauser",
> >>             "hisilicon",
> >>             "iccabpr",
> >>             "iccbpr",
> >> --
> >> 2.28.0.windows.1
> >>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97403): https://edk2.groups.io/g/devel/message/97403
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

I'm just trying to understand your position.

Are you saying you would rather people check in typos and then later 
have patches come into the package to fix them?

For example, like these:

- ArmVirtPkg: https://edk2.groups.io/g/devel/message/97021
- ArmPkg: https://edk2.groups.io/g/devel/message/97022

Why not just have the code checked in without typos in the first place?

Checking in typos creates more review work, makes the code history have 
typo fix patches that could be avoided, and impacts accessibility.

I spend far more time with edk2 overhead such as email formatting 
problems, keeping track of maintainer email address changes when 
updating patches, mapping email replies back to code, and so on that 
does not improve the code. A spell checker only takes seconds and is 
built into edk2 CI.

On 12/14/2022 6:24 PM, Ard Biesheuvel wrote:
> On Thu, 15 Dec 2022 at 00:21, Michael Kubacki
> <mikuback@linux.microsoft.com> wrote:
>>
>> Yes. It will also reduce frequency of incoming patches that must be
>> reviewed and merged due to people continuously fixing trivial spelling
>> errors.
>>
> 
> In that case, NAK to this patch (and the ArmVirtPkg one). Unless we
> add a button to the GitHub UI that permits me to override a negative
> CI result on a PR.
> 
>> On 12/14/2022 6:07 PM, Ard Biesheuvel wrote:
>>> On Wed, 14 Dec 2022 at 23:53, <mikuback@linux.microsoft.com> wrote:
>>>>
>>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>
>>>> Audit mode was enabled for the spellcheck CI plugin. It is no longer
>>>> needed with recent changes. Spelling errors can be checked in the
>>>> package moving forward.
>>>>
>>>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>>
>>> Will this patch result in PRs potentially being rejected by pre-merge
>>> CI due to trivial spelling errors?
>>>
>>>
>>>> ---
>>>>    ArmPkg/ArmPkg.ci.yaml | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
>>>> index c8dface6821a..a304c7966cf7 100644
>>>> --- a/ArmPkg/ArmPkg.ci.yaml
>>>> +++ b/ArmPkg/ArmPkg.ci.yaml
>>>> @@ -87,7 +87,7 @@
>>>>
>>>>        ## options defined .pytool/Plugin/SpellCheck
>>>>        "SpellCheck": {
>>>> -        "AuditOnly": True,
>>>> +        "AuditOnly": False,
>>>>            "IgnoreFiles": [
>>>>                "Library/ArmSoftFloatLib/berkeley-softfloat-3/**"
>>>>            ],                           # use gitignore syntax to ignore errors
>>>> @@ -148,6 +148,7 @@
>>>>              "fcmplt",
>>>>              "ffreestanding",
>>>>              "frsub",
>>>> +          "hauser",
>>>>              "hisilicon",
>>>>              "iccabpr",
>>>>              "iccbpr",
>>>> --
>>>> 2.28.0.windows.1
>>>>
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97405): https://edk2.groups.io/g/devel/message/97405
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, Dec 14, 2022 at 19:04:06 -0500, Michael Kubacki wrote:
> I'm just trying to understand your position.
> 
> Are you saying you would rather people check in typos and then later have
> patches come into the package to fix them?
> 
> For example, like these:
> 
> - ArmVirtPkg: https://edk2.groups.io/g/devel/message/97021
> - ArmPkg: https://edk2.groups.io/g/devel/message/97022
> 
> Why not just have the code checked in without typos in the first place?

A little fairy once whispered in my ear that if I stopped myself and
tried to rephrase whenever I found myself using the work "just", I
would meet less friction in context-stripping communication mediums
such as email. They weren't wrong.

> Checking in typos creates more review work, makes the code history have typo
> fix patches that could be avoided, and impacts accessibility.
> 
> I spend far more time with edk2 overhead such as email formatting problems,
> keeping track of maintainer email address changes when updating patches,
> mapping email replies back to code, and so on that does not improve the
> code. A spell checker only takes seconds and is built into edk2 CI.

We didn't disable the spellchecker for the ARM* packages (only)
because we hate correct spelling. We disabled it because it throws
false positives left right and centre. So we end up needing to update
the .ci.yaml every time we mention an architectural concept we haven't
mentioned before. Or add a copyright line mentioning a new
organisation. Or correctly use apostrophes in ways the spellchecker
doesn't expect.

Here is the current (and very incomplete, oh - and package specific)
exception list for ArmPkg:

https://github.com/tianocore/edk2/blob/master/ArmPkg/ArmPkg.ci.yaml#L95

If it had a aggressiveness knob and we could turn it down from 11 to
3 and work our way up from there, that would be another story.

Failing that, a push-through-anyway-this-isn't-a-typo label would be a
reasonable compromise.

But for now, I agree with Ard.

/
    Leif

> On 12/14/2022 6:24 PM, Ard Biesheuvel wrote:
> > On Thu, 15 Dec 2022 at 00:21, Michael Kubacki
> > <mikuback@linux.microsoft.com> wrote:
> > > 
> > > Yes. It will also reduce frequency of incoming patches that must be
> > > reviewed and merged due to people continuously fixing trivial spelling
> > > errors.
> > > 
> > 
> > In that case, NAK to this patch (and the ArmVirtPkg one). Unless we
> > add a button to the GitHub UI that permits me to override a negative
> > CI result on a PR.
> > 
> > > On 12/14/2022 6:07 PM, Ard Biesheuvel wrote:
> > > > On Wed, 14 Dec 2022 at 23:53, <mikuback@linux.microsoft.com> wrote:
> > > > > 
> > > > > From: Michael Kubacki <michael.kubacki@microsoft.com>
> > > > > 
> > > > > Audit mode was enabled for the spellcheck CI plugin. It is no longer
> > > > > needed with recent changes. Spelling errors can be checked in the
> > > > > package moving forward.
> > > > > 
> > > > > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > > > > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > > > > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > > > > Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> > > > 
> > > > Will this patch result in PRs potentially being rejected by pre-merge
> > > > CI due to trivial spelling errors?
> > > > 
> > > > 
> > > > > ---
> > > > >    ArmPkg/ArmPkg.ci.yaml | 3 ++-
> > > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
> > > > > index c8dface6821a..a304c7966cf7 100644
> > > > > --- a/ArmPkg/ArmPkg.ci.yaml
> > > > > +++ b/ArmPkg/ArmPkg.ci.yaml
> > > > > @@ -87,7 +87,7 @@
> > > > > 
> > > > >        ## options defined .pytool/Plugin/SpellCheck
> > > > >        "SpellCheck": {
> > > > > -        "AuditOnly": True,
> > > > > +        "AuditOnly": False,
> > > > >            "IgnoreFiles": [
> > > > >                "Library/ArmSoftFloatLib/berkeley-softfloat-3/**"
> > > > >            ],                           # use gitignore syntax to ignore errors
> > > > > @@ -148,6 +148,7 @@
> > > > >              "fcmplt",
> > > > >              "ffreestanding",
> > > > >              "frsub",
> > > > > +          "hauser",
> > > > >              "hisilicon",
> > > > >              "iccabpr",
> > > > >              "iccbpr",
> > > > > --
> > > > > 2.28.0.windows.1
> > > > > 
> > 
> > 
> > 
> > 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97436): https://edk2.groups.io/g/devel/message/97436
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 12/15/2022 5:42 AM, Leif Lindholm wrote:
> On Wed, Dec 14, 2022 at 19:04:06 -0500, Michael Kubacki wrote:
>> I'm just trying to understand your position.
>>
>> Are you saying you would rather people check in typos and then later have
>> patches come into the package to fix them?
>>
>> For example, like these:
>>
>> - ArmVirtPkg: https://edk2.groups.io/g/devel/message/97021
>> - ArmPkg: https://edk2.groups.io/g/devel/message/97022
>>
>> Why not just have the code checked in without typos in the first place?
> 
> A little fairy once whispered in my ear that if I stopped myself and
> tried to rephrase whenever I found myself using the work "just", I
> would meet less friction in context-stripping communication mediums
> such as email. They weren't wrong.
> 

This topic is met with friction regardless of how it is phrased.

The friction exists because the community chose to enable spell checking 
in CI and it is not wanted here.

The mechanism chosen to ignore words was through YAML files rather than 
a button. A common YAML file can store words for the whole project and 
packages can add package-specific words. The community was expected to 
make the contributions necessary in the common file to minimize impact 
on package maintainers.

The spellcheck log outputs the exact code that needs to be copied/pasted 
to the exception list - whether the global list or package list. If you 
run CI locally, you can copy/paste the exact lines needed.

This is a patch series I work on in my spare time to try to improve the 
project. I am tired of Ard's dismissive and passive aggressive responses 
such as those in https://edk2.groups.io/g/devel/message/97433 so I 
revoke the series and will let others decide what they want to do.

>> Checking in typos creates more review work, makes the code history have typo
>> fix patches that could be avoided, and impacts accessibility.
>>
>> I spend far more time with edk2 overhead such as email formatting problems,
>> keeping track of maintainer email address changes when updating patches,
>> mapping email replies back to code, and so on that does not improve the
>> code. A spell checker only takes seconds and is built into edk2 CI.
> 
> We didn't disable the spellchecker for the ARM* packages (only)
> because we hate correct spelling. We disabled it because it throws
> false positives left right and centre. So we end up needing to update
> the .ci.yaml every time we mention an architectural concept we haven't
> mentioned before. Or add a copyright line mentioning a new
> organisation. Or correctly use apostrophes in ways the spellchecker
> doesn't expect.
> 
> Here is the current (and very incomplete, oh - and package specific)
> exception list for ArmPkg:
> 
> https://github.com/tianocore/edk2/blob/master/ArmPkg/ArmPkg.ci.yaml#L95
> 
> If it had a aggressiveness knob and we could turn it down from 11 to
> 3 and work our way up from there, that would be another story.
> 
> Failing that, a push-through-anyway-this-isn't-a-typo label would be a
> reasonable compromise.
> 
> But for now, I agree with Ard.
> 
> /
>      Leif
> 
>> On 12/14/2022 6:24 PM, Ard Biesheuvel wrote:
>>> On Thu, 15 Dec 2022 at 00:21, Michael Kubacki
>>> <mikuback@linux.microsoft.com> wrote:
>>>>
>>>> Yes. It will also reduce frequency of incoming patches that must be
>>>> reviewed and merged due to people continuously fixing trivial spelling
>>>> errors.
>>>>
>>>
>>> In that case, NAK to this patch (and the ArmVirtPkg one). Unless we
>>> add a button to the GitHub UI that permits me to override a negative
>>> CI result on a PR.
>>>
>>>> On 12/14/2022 6:07 PM, Ard Biesheuvel wrote:
>>>>> On Wed, 14 Dec 2022 at 23:53, <mikuback@linux.microsoft.com> wrote:
>>>>>>
>>>>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>>>
>>>>>> Audit mode was enabled for the spellcheck CI plugin. It is no longer
>>>>>> needed with recent changes. Spelling errors can be checked in the
>>>>>> package moving forward.
>>>>>>
>>>>>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>>>>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>>>>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>>>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>>
>>>>> Will this patch result in PRs potentially being rejected by pre-merge
>>>>> CI due to trivial spelling errors?
>>>>>
>>>>>
>>>>>> ---
>>>>>>     ArmPkg/ArmPkg.ci.yaml | 3 ++-
>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
>>>>>> index c8dface6821a..a304c7966cf7 100644
>>>>>> --- a/ArmPkg/ArmPkg.ci.yaml
>>>>>> +++ b/ArmPkg/ArmPkg.ci.yaml
>>>>>> @@ -87,7 +87,7 @@
>>>>>>
>>>>>>         ## options defined .pytool/Plugin/SpellCheck
>>>>>>         "SpellCheck": {
>>>>>> -        "AuditOnly": True,
>>>>>> +        "AuditOnly": False,
>>>>>>             "IgnoreFiles": [
>>>>>>                 "Library/ArmSoftFloatLib/berkeley-softfloat-3/**"
>>>>>>             ],                           # use gitignore syntax to ignore errors
>>>>>> @@ -148,6 +148,7 @@
>>>>>>               "fcmplt",
>>>>>>               "ffreestanding",
>>>>>>               "frsub",
>>>>>> +          "hauser",
>>>>>>               "hisilicon",
>>>>>>               "iccabpr",
>>>>>>               "iccbpr",
>>>>>> --
>>>>>> 2.28.0.windows.1
>>>>>>
>>>
>>>
>>>
>>>
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97473): https://edk2.groups.io/g/devel/message/97473
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, 15 Dec 2022 at 17:38, Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> On 12/15/2022 5:42 AM, Leif Lindholm wrote:
> > On Wed, Dec 14, 2022 at 19:04:06 -0500, Michael Kubacki wrote:
> >> I'm just trying to understand your position.
> >>
> >> Are you saying you would rather people check in typos and then later have
> >> patches come into the package to fix them?
> >>
> >> For example, like these:
> >>
> >> - ArmVirtPkg: https://edk2.groups.io/g/devel/message/97021
> >> - ArmPkg: https://edk2.groups.io/g/devel/message/97022
> >>
> >> Why not just have the code checked in without typos in the first place?
> >
> > A little fairy once whispered in my ear that if I stopped myself and
> > tried to rephrase whenever I found myself using the work "just", I
> > would meet less friction in context-stripping communication mediums
> > such as email. They weren't wrong.
> >
>
> This topic is met with friction regardless of how it is phrased.
>
> The friction exists because the community chose to enable spell checking
> in CI and it is not wanted here.
>
> The mechanism chosen to ignore words was through YAML files rather than
> a button. A common YAML file can store words for the whole project and
> packages can add package-specific words. The community was expected to
> make the contributions necessary in the common file to minimize impact
> on package maintainers.
>
> The spellcheck log outputs the exact code that needs to be copied/pasted
> to the exception list - whether the global list or package list. If you
> run CI locally, you can copy/paste the exact lines needed.
>
> This is a patch series I work on in my spare time to try to improve the
> project. I am tired of Ard's dismissive and passive aggressive responses
> such as those in https://edk2.groups.io/g/devel/message/97433 so I
> revoke the series and will let others decide what they want to do.
>

I don't think this is a fair characterization of my response. I
explained in detail why I think rigid spellcheck rules are
counter-productive and do not contribute to the quality of what gets
deployed to devices.

Nevertheless, I apologize if my frustration with the recent CI changes
managed to seep through, although I should also mention that I am a
big fan of the pre-merge build and boot tests, as they have been a
huge help in my workflow. Only the rigid enforcement of standards that
are purely cosmetic is what bothers me, especially because finding the
error messages (and suggestions for improvement) in the complex UI is
not as straight-forward as it is made out to be.

That said, it would be helpful if you could respond to the actual
points I made, rather than dismissing them wholesale as a
passive-aggressive and hostile response. You have presented your
contribution as a take-it-or-leave-it style change, rather than
engaging with Leif and me as the package maintainers to converge on
something that we can all agree on.

You may have also missed my question/invitation regarding
collaboration on IBT/BTI enablement in UEFI.

Kind regards,
Ard.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97930): https://edk2.groups.io/g/devel/message/97930
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Ard,

I believe you're referencing the points in this mail, right?
https://edk2.groups.io/g/devel/message/97433

To be clear about why that was considered dismissive and passive 
aggressive, these sentences:

"I would also like to point out that all this focus on code aesthetics
that do not contribute to the object code at all is distracting from
things that would actually improve the safety and robustness of the
code."

"So if you are interested, and want to improve EDK2 at a level that
actually makes the shipping code more robust, we could collaborate on
this (PE spec change, UEFI spec change, EDK2 and Linux changes both
for x86 and arm64[0]) so that firmware runs with indirect branch
protection enabled when the hardware supports it."

Read as "this is unimportant/distracting so do this more important stuff 
instead".

This may not been what you meant to convey, but it doesn't set the best 
foundation for collaboration on the topic.

I understand that there will always be functionally impacting work that 
has tangible value over supporting changes like correctly spelling 
words. Though, it's not purely aesthetic, language translators and 
screen readers function more reliably if words are spelled correctly. In 
any case, improving reading comprehension and functionality are not 
mutually exclusive.

I think we're meeting on the topic with two different sets of 
expectations that naturally conflict.

1. You're not happy with the current process used for spell checking in 
TianoCore
2. I would like to prevent and/or minimize future patches that only fix 
typos (check in CI)
3. We both believe fixing spelling errors should be simple

(1) is a valid opinion to have but because of (1), I cannot enable (2). 
Because of (1), (3) is not true as the infrastructure needs to be 
revamped, which I do not personally have bandwidth to do at the moment.

My initial understanding was that the TianoCore process had further 
discussion and agreement than it seems occurred so I thought we (as a 
community) were in position to simply follow it and reduce the typo 
count. Without (2), the typos fixed can reappear (the next commit could 
reintroduce the same typo without enforcement) so, in my opinion, it's 
not worth the time and effort to manage the patch series in that case. 
That's why I revoked the series.

Your feedback is still valuable to the wider TianoCore Tools & CI group 
that meets weekly. It may be able to be addressed there.

On the topic of IBT/BTI, I agree that's useful. We probably need to have 
a dedicated thread on that to get more background on where you're at and 
then we can try to help where possible.

Would you be open to a meeting to discuss that?

Thanks,
Michael

On 1/4/2023 5:37 AM, Ard Biesheuvel wrote:
> On Thu, 15 Dec 2022 at 17:38, Michael Kubacki
> <mikuback@linux.microsoft.com> wrote:
>>
>> On 12/15/2022 5:42 AM, Leif Lindholm wrote:
>>> On Wed, Dec 14, 2022 at 19:04:06 -0500, Michael Kubacki wrote:
>>>> I'm just trying to understand your position.
>>>>
>>>> Are you saying you would rather people check in typos and then later have
>>>> patches come into the package to fix them?
>>>>
>>>> For example, like these:
>>>>
>>>> - ArmVirtPkg: https://edk2.groups.io/g/devel/message/97021
>>>> - ArmPkg: https://edk2.groups.io/g/devel/message/97022
>>>>
>>>> Why not just have the code checked in without typos in the first place?
>>>
>>> A little fairy once whispered in my ear that if I stopped myself and
>>> tried to rephrase whenever I found myself using the work "just", I
>>> would meet less friction in context-stripping communication mediums
>>> such as email. They weren't wrong.
>>>
>>
>> This topic is met with friction regardless of how it is phrased.
>>
>> The friction exists because the community chose to enable spell checking
>> in CI and it is not wanted here.
>>
>> The mechanism chosen to ignore words was through YAML files rather than
>> a button. A common YAML file can store words for the whole project and
>> packages can add package-specific words. The community was expected to
>> make the contributions necessary in the common file to minimize impact
>> on package maintainers.
>>
>> The spellcheck log outputs the exact code that needs to be copied/pasted
>> to the exception list - whether the global list or package list. If you
>> run CI locally, you can copy/paste the exact lines needed.
>>
>> This is a patch series I work on in my spare time to try to improve the
>> project. I am tired of Ard's dismissive and passive aggressive responses
>> such as those in https://edk2.groups.io/g/devel/message/97433 so I
>> revoke the series and will let others decide what they want to do.
>>
> 
> I don't think this is a fair characterization of my response. I
> explained in detail why I think rigid spellcheck rules are
> counter-productive and do not contribute to the quality of what gets
> deployed to devices.
> 
> Nevertheless, I apologize if my frustration with the recent CI changes
> managed to seep through, although I should also mention that I am a
> big fan of the pre-merge build and boot tests, as they have been a
> huge help in my workflow. Only the rigid enforcement of standards that
> are purely cosmetic is what bothers me, especially because finding the
> error messages (and suggestions for improvement) in the complex UI is
> not as straight-forward as it is made out to be.
> 
> That said, it would be helpful if you could respond to the actual
> points I made, rather than dismissing them wholesale as a
> passive-aggressive and hostile response. You have presented your
> contribution as a take-it-or-leave-it style change, rather than
> engaging with Leif and me as the package maintainers to converge on
> something that we can all agree on.
> 
> You may have also missed my question/invitation regarding
> collaboration on IBT/BTI enablement in UEFI.
> 
> Kind regards,
> Ard.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98049): https://edk2.groups.io/g/devel/message/98049
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Fri, 6 Jan 2023 at 03:46, Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> Hi Ard,
>
> I believe you're referencing the points in this mail, right?
> https://edk2.groups.io/g/devel/message/97433
>
> To be clear about why that was considered dismissive and passive
> aggressive, these sentences:
>
> "I would also like to point out that all this focus on code aesthetics
> that do not contribute to the object code at all is distracting from
> things that would actually improve the safety and robustness of the
> code."
>
> "So if you are interested, and want to improve EDK2 at a level that
> actually makes the shipping code more robust, we could collaborate on
> this (PE spec change, UEFI spec change, EDK2 and Linux changes both
> for x86 and arm64[0]) so that firmware runs with indirect branch
> protection enabled when the hardware supports it."
>
> Read as "this is unimportant/distracting so do this more important stuff
> instead".
>

Fair enough. It was never my intent to be disrespectful, though.

> This may not been what you meant to convey, but it doesn't set the best
> foundation for collaboration on the topic.
>

True.

> I understand that there will always be functionally impacting work that
> has tangible value over supporting changes like correctly spelling
> words. Though, it's not purely aesthetic, language translators and
> screen readers function more reliably if words are spelled correctly. In
> any case, improving reading comprehension and functionality are not
> mutually exclusive.
>
> I think we're meeting on the topic with two different sets of
> expectations that naturally conflict.
>
> 1. You're not happy with the current process used for spell checking in
> TianoCore
> 2. I would like to prevent and/or minimize future patches that only fix
> typos (check in CI)
> 3. We both believe fixing spelling errors should be simple
>
> (1) is a valid opinion to have but because of (1), I cannot enable (2).
> Because of (1), (3) is not true as the infrastructure needs to be
> revamped, which I do not personally have bandwidth to do at the moment.
>

None of this would be problematic for me if I had the discretion as a
long time maintainer to override/ignore certain CI errors. I don't see
how this is fundamentally different from adding known typos to the
exception list.

> My initial understanding was that the TianoCore process had further
> discussion and agreement than it seems occurred so I thought we (as a
> community) were in position to simply follow it and reduce the typo
> count. Without (2), the typos fixed can reappear (the next commit could
> reintroduce the same typo without enforcement) so, in my opinion, it's
> not worth the time and effort to manage the patch series in that case.
> That's why I revoked the series.
>

Fair enough.

> Your feedback is still valuable to the wider TianoCore Tools & CI group
> that meets weekly. It may be able to be addressed there.
>

Those tend to take place in the middle of the night for me, which is
why I rarely join those.

> On the topic of IBT/BTI, I agree that's useful. We probably need to have
> a dedicated thread on that to get more background on where you're at and
> then we can try to help where possible.
>
> Would you be open to a meeting to discuss that?
>

Yes, please. I'm in UTC+1, and my calendar is generally fairly empty,
so please feel free to set a time that works for you.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98301): https://edk2.groups.io/g/devel/message/98301
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 1/11/2023 11:23 AM, Ard Biesheuvel wrote:
>> Your feedback is still valuable to the wider TianoCore Tools & CI group
>> that meets weekly. It may be able to be addressed there.
>>
> 
> Those tend to take place in the middle of the night for me, which is
> why I rarely join those.
> 
The meeting time has been an issue for a number of people that have 
expressed interest in joining. There was a discussion around hosting a 
meeting at least once a month that is Europe friendly that hopefully can 
get started soon.

>> On the topic of IBT/BTI, I agree that's useful. We probably need to have
>> a dedicated thread on that to get more background on where you're at and
>> then we can try to help where possible.
>>
>> Would you be open to a meeting to discuss that?
>>
> 
> Yes, please. I'm in UTC+1, and my calendar is generally fairly empty,
> so please feel free to set a time that works for you.

I've learned this was discussed around 2020 and it appears to have been 
dropped. Sorry about that. We are gathering the right contacts and will 
follow up with you likely next week.

Thanks,
Michael


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98412): https://edk2.groups.io/g/devel/message/98412
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Michael,

I appreciate your efforts to improve code quality in all aspects.

I think this is the only part of the change that is being asked
to be adjusted (specifically for the ArmPkg and ArmVirtPkg) is
the following.

>>>>>> -        "AuditOnly": True,
>>>>>> +        "AuditOnly": False,

Different packages may choose different levels if quality or 
quality checks based on the maturity of the package and what
is deemed critical to the package maintainers.

There are obvious checks such as compile failures that we can
all agree on.  Checks that are not directly related to 
functionality should perhaps be flexible and the policy be 
allowed to be adjusted by the package maintainers.

I personally would prefer to see as many packages as possible
take advantage of the CI checks that have been enabled, so I
think it is good to start with an approach where everything is
on by default, and if a package maintainer would prefer a
specific package to have one disabled, then defer to that request.

I also think it would be good to add comments to the package
YAML file if a check is disabled that clearly states that
the choice to diverge from the standard CI checks was made
by the package maintainer.

With this approach, I think your series can go forward with
AuditOnly set to True for the ArmPkg and ArtVirtPkg with 
comments that this setting is different than the default
due to requests from the maintainers.

Best regards,

Mike


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
> Sent: Thursday, December 15, 2022 8:39 AM
> To: devel@edk2.groups.io; quic_llindhol@quicinc.com
> Cc: ardb@kernel.org; Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 13/14] ArmPkg: Turn off spellcheck audit mode
> 
> On 12/15/2022 5:42 AM, Leif Lindholm wrote:
> > On Wed, Dec 14, 2022 at 19:04:06 -0500, Michael Kubacki wrote:
> >> I'm just trying to understand your position.
> >>
> >> Are you saying you would rather people check in typos and then later have
> >> patches come into the package to fix them?
> >>
> >> For example, like these:
> >>
> >> - ArmVirtPkg: https://edk2.groups.io/g/devel/message/97021
> >> - ArmPkg: https://edk2.groups.io/g/devel/message/97022
> >>
> >> Why not just have the code checked in without typos in the first place?
> >
> > A little fairy once whispered in my ear that if I stopped myself and
> > tried to rephrase whenever I found myself using the work "just", I
> > would meet less friction in context-stripping communication mediums
> > such as email. They weren't wrong.
> >
> 
> This topic is met with friction regardless of how it is phrased.
> 
> The friction exists because the community chose to enable spell checking
> in CI and it is not wanted here.
> 
> The mechanism chosen to ignore words was through YAML files rather than
> a button. A common YAML file can store words for the whole project and
> packages can add package-specific words. The community was expected to
> make the contributions necessary in the common file to minimize impact
> on package maintainers.
> 
> The spellcheck log outputs the exact code that needs to be copied/pasted
> to the exception list - whether the global list or package list. If you
> run CI locally, you can copy/paste the exact lines needed.
> 
> This is a patch series I work on in my spare time to try to improve the
> project. I am tired of Ard's dismissive and passive aggressive responses
> such as those in https://edk2.groups.io/g/devel/message/97433 so I
> revoke the series and will let others decide what they want to do.
> 
> >> Checking in typos creates more review work, makes the code history have typo
> >> fix patches that could be avoided, and impacts accessibility.
> >>
> >> I spend far more time with edk2 overhead such as email formatting problems,
> >> keeping track of maintainer email address changes when updating patches,
> >> mapping email replies back to code, and so on that does not improve the
> >> code. A spell checker only takes seconds and is built into edk2 CI.
> >
> > We didn't disable the spellchecker for the ARM* packages (only)
> > because we hate correct spelling. We disabled it because it throws
> > false positives left right and centre. So we end up needing to update
> > the .ci.yaml every time we mention an architectural concept we haven't
> > mentioned before. Or add a copyright line mentioning a new
> > organisation. Or correctly use apostrophes in ways the spellchecker
> > doesn't expect.
> >
> > Here is the current (and very incomplete, oh - and package specific)
> > exception list for ArmPkg:
> >
> > https://github.com/tianocore/edk2/blob/master/ArmPkg/ArmPkg.ci.yaml#L95
> >
> > If it had a aggressiveness knob and we could turn it down from 11 to
> > 3 and work our way up from there, that would be another story.
> >
> > Failing that, a push-through-anyway-this-isn't-a-typo label would be a
> > reasonable compromise.
> >
> > But for now, I agree with Ard.
> >
> > /
> >      Leif
> >
> >> On 12/14/2022 6:24 PM, Ard Biesheuvel wrote:
> >>> On Thu, 15 Dec 2022 at 00:21, Michael Kubacki
> >>> <mikuback@linux.microsoft.com> wrote:
> >>>>
> >>>> Yes. It will also reduce frequency of incoming patches that must be
> >>>> reviewed and merged due to people continuously fixing trivial spelling
> >>>> errors.
> >>>>
> >>>
> >>> In that case, NAK to this patch (and the ArmVirtPkg one). Unless we
> >>> add a button to the GitHub UI that permits me to override a negative
> >>> CI result on a PR.
> >>>
> >>>> On 12/14/2022 6:07 PM, Ard Biesheuvel wrote:
> >>>>> On Wed, 14 Dec 2022 at 23:53, <mikuback@linux.microsoft.com> wrote:
> >>>>>>
> >>>>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
> >>>>>>
> >>>>>> Audit mode was enabled for the spellcheck CI plugin. It is no longer
> >>>>>> needed with recent changes. Spelling errors can be checked in the
> >>>>>> package moving forward.
> >>>>>>
> >>>>>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> >>>>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >>>>>> Cc: Sami Mujawar <sami.mujawar@arm.com>
> >>>>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> >>>>>
> >>>>> Will this patch result in PRs potentially being rejected by pre-merge
> >>>>> CI due to trivial spelling errors?
> >>>>>
> >>>>>
> >>>>>> ---
> >>>>>>     ArmPkg/ArmPkg.ci.yaml | 3 ++-
> >>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
> >>>>>> index c8dface6821a..a304c7966cf7 100644
> >>>>>> --- a/ArmPkg/ArmPkg.ci.yaml
> >>>>>> +++ b/ArmPkg/ArmPkg.ci.yaml
> >>>>>> @@ -87,7 +87,7 @@
> >>>>>>
> >>>>>>         ## options defined .pytool/Plugin/SpellCheck
> >>>>>>         "SpellCheck": {
> >>>>>> -        "AuditOnly": True,
> >>>>>> +        "AuditOnly": False,
> >>>>>>             "IgnoreFiles": [
> >>>>>>                 "Library/ArmSoftFloatLib/berkeley-softfloat-3/**"
> >>>>>>             ],                           # use gitignore syntax to ignore errors
> >>>>>> @@ -148,6 +148,7 @@
> >>>>>>               "fcmplt",
> >>>>>>               "ffreestanding",
> >>>>>>               "frsub",
> >>>>>> +          "hauser",
> >>>>>>               "hisilicon",
> >>>>>>               "iccabpr",
> >>>>>>               "iccbpr",
> >>>>>> --
> >>>>>> 2.28.0.windows.1
> >>>>>>
> >>>
> >>>
> >>>
> >>>
> >
> >
> >
> >
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97475): https://edk2.groups.io/g/devel/message/97475
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Mike,

Thanks for the suggestion but I think I'm just going to avoid making 
these changes in edk2, at least for now.

If package maintainers don't appreciate the work and we have to figure 
out who wants what every time a patch is submitted it truly does consume 
more of my time than it should.

Hopefully, a productive result from this series can be a realization 
that senior members of this community set the culture and less hostility 
toward contributors would encourage more contribution.

I agree the comment in the CI YAML file would be helpful or a section in 
a package readme that states the maintainer's preferences.

Thanks,
Michael

On 12/15/2022 11:57 AM, Michael D Kinney wrote:
> Michael,
> 
> I appreciate your efforts to improve code quality in all aspects.
> 
> I think this is the only part of the change that is being asked
> to be adjusted (specifically for the ArmPkg and ArmVirtPkg) is
> the following.
> 
>>>>>>> -        "AuditOnly": True,
>>>>>>> +        "AuditOnly": False,
> 
> Different packages may choose different levels if quality or
> quality checks based on the maturity of the package and what
> is deemed critical to the package maintainers.
> 
> There are obvious checks such as compile failures that we can
> all agree on.  Checks that are not directly related to
> functionality should perhaps be flexible and the policy be
> allowed to be adjusted by the package maintainers.
> 
> I personally would prefer to see as many packages as possible
> take advantage of the CI checks that have been enabled, so I
> think it is good to start with an approach where everything is
> on by default, and if a package maintainer would prefer a
> specific package to have one disabled, then defer to that request.
> 
> I also think it would be good to add comments to the package
> YAML file if a check is disabled that clearly states that
> the choice to diverge from the standard CI checks was made
> by the package maintainer.
> 
> With this approach, I think your series can go forward with
> AuditOnly set to True for the ArmPkg and ArtVirtPkg with
> comments that this setting is different than the default
> due to requests from the maintainers.
> 
> Best regards,
> 
> Mike
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
>> Sent: Thursday, December 15, 2022 8:39 AM
>> To: devel@edk2.groups.io; quic_llindhol@quicinc.com
>> Cc: ardb@kernel.org; Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v3 13/14] ArmPkg: Turn off spellcheck audit mode
>>
>> On 12/15/2022 5:42 AM, Leif Lindholm wrote:
>>> On Wed, Dec 14, 2022 at 19:04:06 -0500, Michael Kubacki wrote:
>>>> I'm just trying to understand your position.
>>>>
>>>> Are you saying you would rather people check in typos and then later have
>>>> patches come into the package to fix them?
>>>>
>>>> For example, like these:
>>>>
>>>> - ArmVirtPkg: https://edk2.groups.io/g/devel/message/97021
>>>> - ArmPkg: https://edk2.groups.io/g/devel/message/97022
>>>>
>>>> Why not just have the code checked in without typos in the first place?
>>>
>>> A little fairy once whispered in my ear that if I stopped myself and
>>> tried to rephrase whenever I found myself using the work "just", I
>>> would meet less friction in context-stripping communication mediums
>>> such as email. They weren't wrong.
>>>
>>
>> This topic is met with friction regardless of how it is phrased.
>>
>> The friction exists because the community chose to enable spell checking
>> in CI and it is not wanted here.
>>
>> The mechanism chosen to ignore words was through YAML files rather than
>> a button. A common YAML file can store words for the whole project and
>> packages can add package-specific words. The community was expected to
>> make the contributions necessary in the common file to minimize impact
>> on package maintainers.
>>
>> The spellcheck log outputs the exact code that needs to be copied/pasted
>> to the exception list - whether the global list or package list. If you
>> run CI locally, you can copy/paste the exact lines needed.
>>
>> This is a patch series I work on in my spare time to try to improve the
>> project. I am tired of Ard's dismissive and passive aggressive responses
>> such as those in https://edk2.groups.io/g/devel/message/97433 so I
>> revoke the series and will let others decide what they want to do.
>>
>>>> Checking in typos creates more review work, makes the code history have typo
>>>> fix patches that could be avoided, and impacts accessibility.
>>>>
>>>> I spend far more time with edk2 overhead such as email formatting problems,
>>>> keeping track of maintainer email address changes when updating patches,
>>>> mapping email replies back to code, and so on that does not improve the
>>>> code. A spell checker only takes seconds and is built into edk2 CI.
>>>
>>> We didn't disable the spellchecker for the ARM* packages (only)
>>> because we hate correct spelling. We disabled it because it throws
>>> false positives left right and centre. So we end up needing to update
>>> the .ci.yaml every time we mention an architectural concept we haven't
>>> mentioned before. Or add a copyright line mentioning a new
>>> organisation. Or correctly use apostrophes in ways the spellchecker
>>> doesn't expect.
>>>
>>> Here is the current (and very incomplete, oh - and package specific)
>>> exception list for ArmPkg:
>>>
>>> https://github.com/tianocore/edk2/blob/master/ArmPkg/ArmPkg.ci.yaml#L95
>>>
>>> If it had a aggressiveness knob and we could turn it down from 11 to
>>> 3 and work our way up from there, that would be another story.
>>>
>>> Failing that, a push-through-anyway-this-isn't-a-typo label would be a
>>> reasonable compromise.
>>>
>>> But for now, I agree with Ard.
>>>
>>> /
>>>       Leif
>>>
>>>> On 12/14/2022 6:24 PM, Ard Biesheuvel wrote:
>>>>> On Thu, 15 Dec 2022 at 00:21, Michael Kubacki
>>>>> <mikuback@linux.microsoft.com> wrote:
>>>>>>
>>>>>> Yes. It will also reduce frequency of incoming patches that must be
>>>>>> reviewed and merged due to people continuously fixing trivial spelling
>>>>>> errors.
>>>>>>
>>>>>
>>>>> In that case, NAK to this patch (and the ArmVirtPkg one). Unless we
>>>>> add a button to the GitHub UI that permits me to override a negative
>>>>> CI result on a PR.
>>>>>
>>>>>> On 12/14/2022 6:07 PM, Ard Biesheuvel wrote:
>>>>>>> On Wed, 14 Dec 2022 at 23:53, <mikuback@linux.microsoft.com> wrote:
>>>>>>>>
>>>>>>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>>>>>
>>>>>>>> Audit mode was enabled for the spellcheck CI plugin. It is no longer
>>>>>>>> needed with recent changes. Spelling errors can be checked in the
>>>>>>>> package moving forward.
>>>>>>>>
>>>>>>>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>>>>>>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>>>>>>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>>>>>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>>>>
>>>>>>> Will this patch result in PRs potentially being rejected by pre-merge
>>>>>>> CI due to trivial spelling errors?
>>>>>>>
>>>>>>>
>>>>>>>> ---
>>>>>>>>      ArmPkg/ArmPkg.ci.yaml | 3 ++-
>>>>>>>>      1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
>>>>>>>> index c8dface6821a..a304c7966cf7 100644
>>>>>>>> --- a/ArmPkg/ArmPkg.ci.yaml
>>>>>>>> +++ b/ArmPkg/ArmPkg.ci.yaml
>>>>>>>> @@ -87,7 +87,7 @@
>>>>>>>>
>>>>>>>>          ## options defined .pytool/Plugin/SpellCheck
>>>>>>>>          "SpellCheck": {
>>>>>>>> -        "AuditOnly": True,
>>>>>>>> +        "AuditOnly": False,
>>>>>>>>              "IgnoreFiles": [
>>>>>>>>                  "Library/ArmSoftFloatLib/berkeley-softfloat-3/**"
>>>>>>>>              ],                           # use gitignore syntax to ignore errors
>>>>>>>> @@ -148,6 +148,7 @@
>>>>>>>>                "fcmplt",
>>>>>>>>                "ffreestanding",
>>>>>>>>                "frsub",
>>>>>>>> +          "hauser",
>>>>>>>>                "hisilicon",
>>>>>>>>                "iccabpr",
>>>>>>>>                "iccbpr",
>>>>>>>> --
>>>>>>>> 2.28.0.windows.1
>>>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97511): https://edk2.groups.io/g/devel/message/97511
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, 15 Dec 2022 at 01:04, Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> I'm just trying to understand your position.
>
> Are you saying you would rather people check in typos and then later
> have patches come into the package to fix them?
>
> For example, like these:
>
> - ArmVirtPkg: https://edk2.groups.io/g/devel/message/97021
> - ArmPkg: https://edk2.groups.io/g/devel/message/97022
>
> Why not just have the code checked in without typos in the first place?
>
> Checking in typos creates more review work, makes the code history have
> typo fix patches that could be avoided, and impacts accessibility.
>
> I spend far more time with edk2 overhead such as email formatting
> problems, keeping track of maintainer email address changes when
> updating patches, mapping email replies back to code, and so on that
> does not improve the code. A spell checker only takes seconds and is
> built into edk2 CI.
>

SpellCheck cannot distinguish between typos and valid uses of non-english words.

ArmPkg is not made up of english prose, it is unidiomatic even for
EDK2 as its code contains many of the hundreds of mnemonics and
acronyms that are defined in the ARM Architecture Reference Manual.
With SpellCheck enabled, we need to extend the exception list every
time the use of such a word gets introduced into the code, which is
tedious and pointless, given that the exception list is already
hundreds of entries long, and you are even adding *known typos* to it.

Whether or not a patch passes SpellCheck is therefore becoming a poor
measure of quality. And fundamentally, given that the resulting object
code is identical, SpellCheck does not contribute *at all* to making
the code that actually gets shipped any better.

I do care about typos, and if this wasn't so disruptive to my normal
development process, I wouldn't object to it, but getting a simple PR
merged has already become a massive waste of time due to the rigid
uncrustify rules (which I am not as excited about either, tbh). So
rejecting PRs simply because some word does not appear in the list is
not acceptable to me.

Unless I get a button in the GitHub UI that permits me to force-merge
a PR if it failed the SpellCheck pass. Or adds it to the exception
list automatically.

I would also like to point out that all this focus on code aesthetics
that do not contribute to the object code at all is distracting from
things that would actually improve the safety and robustness of the
code. During my time at ARM, I implemented a prototype for IBT/BTI in
EFI, which -given how much EFI relies on function pointers and
indirect calls- would be a very useful thing to have, both on x86 and
arm64.

Unfortunately, this requires a PE/COFF spec change, so that .text
sections can be marked as having been built with brach-tracking
landing pads. At the time, we tried various contacts in Microsoft to
find the people that could help author such a change to the PE/COFF
spec, but I never got anywhere.

So if you are interested, and want to improve EDK2 at a level that
actually makes the shipping code more robust, we could collaborate on
this (PE spec change, UEFI spec change, EDK2 and Linux changes both
for x86 and arm64[0]) so that firmware runs with indirect branch
protection enabled when the hardware supports it.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=efi-bti


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97433): https://edk2.groups.io/g/devel/message/97433
Mute This Topic: https://groups.io/mt/95678218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-