[RFC PATCH] ArmPkg: Enable AuditMode for Uncrustify CI checks

[RFC PATCH] ArmPkg: Enable AuditMode for Uncrustify CI checks

[Ard Biesheuvel]

Uncrustify checks are too rigid, making them counter-productive:

- it leads to code that is arguably harder to parse visually (e.g.,
  the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit
  429309e0c6b74792)
- it forces indentation-only changes to code in the vicinity of actual
  changes, making the code history more bloated than necessary (see
  commit 7f198321eec0f520373 for an example)
- finding out from the web UI what exactly Uncrustify objected to is not
  straight-forward.

So let's enable AuditMode for ArmPkg, so that interested parties can see
the uncrustify recommendations if desired, but without preventing the
changes from being merged. This leaves it at the discretion of the
ArmPkg maintainers to decide which level of conformance is required.

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/ArmPkg.ci.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
index 24db7425051388cf..d3124816118944cb 100644
--- a/ArmPkg/ArmPkg.ci.yaml
+++ b/ArmPkg/ArmPkg.ci.yaml
@@ -239,5 +239,10 @@
         ],
         "AdditionalIncludePaths": [] # Additional paths to spell check
                                      # (wildcards supported)
+    },
+
+    # options defined in .pytool/Plugin/UncrustifyCheck
+    "UncrustifyCheck": {
+        "AuditOnly": True
     }
 }
-- 
2.39.2

Re: [RFC PATCH] ArmPkg: Enable AuditMode for Uncrustify CI checks

[Leif Lindholm]

On Fri, Jun 02, 2023 at 10:51:36 +0200, Ard Biesheuvel wrote:
> Uncrustify checks are too rigid, making them counter-productive:
> 
> - it leads to code that is arguably harder to parse visually (e.g.,
>   the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit
>   429309e0c6b74792)
> - it forces indentation-only changes to code in the vicinity of actual
>   changes, making the code history more bloated than necessary (see
>   commit 7f198321eec0f520373 for an example)
> - finding out from the web UI what exactly Uncrustify objected to is not
>   straight-forward.
> 
> So let's enable AuditMode for ArmPkg, so that interested parties can see
> the uncrustify recommendations if desired, but without preventing the
> changes from being merged. This leaves it at the discretion of the
> ArmPkg maintainers to decide which level of conformance is required.
> 
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

I'm going to merge this - at least for now - since some of the quirks
are holding back merging code that is compliant because of existing
things it takes an issue with elsewhere in the file.

/
    Leif

> ---
>  ArmPkg/ArmPkg.ci.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
> index 24db7425051388cf..d3124816118944cb 100644
> --- a/ArmPkg/ArmPkg.ci.yaml
> +++ b/ArmPkg/ArmPkg.ci.yaml
> @@ -239,5 +239,10 @@
>          ],
>          "AdditionalIncludePaths": [] # Additional paths to spell check
>                                       # (wildcards supported)
> +    },
> +
> +    # options defined in .pytool/Plugin/UncrustifyCheck
> +    "UncrustifyCheck": {
> +        "AuditOnly": True
>      }
>  }
> -- 
> 2.39.2
> 

Re: [edk2-devel] [RFC PATCH] ArmPkg: Enable AuditMode for Uncrustify CI checks

[Michael Kubacki]

Hi Ard,

Are there particular areas that could be improved to make it more usable 
for you? I'm trying to find actionable improvements that can be made, if 
any.

I know it's not perfect but developers can run it with a single keyboard 
shortcut and it's been useful internally for eliminating style minutia 
from distracting design and correctness conversation in code reviews.




 > - finding out from the web UI what exactly Uncrustify objected to is not
 >    straight-forward.
 >
This should not be necessary. It can be run locally to produce the same 
result as in CI.

IDE-specific, but a lot of people use VS Code with the Uncrustify 
extension 
(https://marketplace.visualstudio.com/items?itemName=zachflower.uncrustify) 
and that allows Uncrustify to be run against the open source file by 
simply pressing the "code formatter" command.

I usually write the code stage or commit it and then run this command on 
the file. The code is formatted and it gives a normal local diff of 
exactly what Uncrustify changed.


On 6/2/2023 4:51 AM, Ard Biesheuvel wrote:
> Uncrustify checks are too rigid, making them counter-productive:
> 
> - it leads to code that is arguably harder to parse visually (e.g.,
>    the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit
>    429309e0c6b74792)

Looking at commit 7f198321eec0f520373, I see positive changes like in 
ArmCrashDumpDxe.c spacing in the ASSERT calls and treatment of 
multi-line parameter formatting in the mCpu->RegisterInterruptHandler() 
call are consistent.

That carries on for a number of C calls with inconsistent spacing before 
opening parentheses and calls such as those to the DEBUG macro which I 
find easier to read separating each parameter on a dedicated line.

In AArch64Mmu.h, I agree that preserving the (mostly) global column as 
opposed to block-specific columns would be easier to vertically scan. Is 
that the main issue in the file?

> - it forces indentation-only changes to code in the vicinity of actual
>    changes, making the code history more bloated than necessary (see
>    commit 7f198321eec0f520373 for an example)

> - finding out from the web UI what exactly Uncrustify objected to is not
>    straight-forward.
> 
> So let's enable AuditMode for ArmPkg, so that interested parties can see
> the uncrustify recommendations if desired, but without preventing the
> changes from being merged. This leaves it at the discretion of the
> ArmPkg maintainers to decide which level of conformance is required.
> 
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   ArmPkg/ArmPkg.ci.yaml | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
> index 24db7425051388cf..d3124816118944cb 100644
> --- a/ArmPkg/ArmPkg.ci.yaml
> +++ b/ArmPkg/ArmPkg.ci.yaml
> @@ -239,5 +239,10 @@
>           ],
> 
>           "AdditionalIncludePaths": [] # Additional paths to spell check
> 
>                                        # (wildcards supported)
> 
> +    },
> 
> +
> 
> +    # options defined in .pytool/Plugin/UncrustifyCheck
> 
> +    "UncrustifyCheck": {
> 
> +        "AuditOnly": True
> 
>       }
> 
>   }
> 

Re: [edk2-devel] [RFC PATCH] ArmPkg: Enable AuditMode for Uncrustify CI checks

[Michael Kubacki]

Sorry, this message was accidentally sent while I was adjusting 
formatting. I reply to the original mail with a clean reply.

On 6/2/2023 11:18 AM, Michael Kubacki wrote:
> Hi Ard,
> 
> Are there particular areas that could be improved to make it more usable 
> for you? I'm trying to find actionable improvements that can be made, if 
> any.
> 
> I know it's not perfect but developers can run it with a single keyboard 
> shortcut and it's been useful internally for eliminating style minutia 
> from distracting design and correctness conversation in code reviews.
> 
> 
> 
> 
>  > - finding out from the web UI what exactly Uncrustify objected to is not
>  >    straight-forward.
>  >
> This should not be necessary. It can be run locally to produce the same 
> result as in CI.
> 
> IDE-specific, but a lot of people use VS Code with the Uncrustify 
> extension 
> (https://marketplace.visualstudio.com/items?itemName=zachflower.uncrustify) and that allows Uncrustify to be run against the open source file by simply pressing the "code formatter" command.
> 
> I usually write the code stage or commit it and then run this command on 
> the file. The code is formatted and it gives a normal local diff of 
> exactly what Uncrustify changed.
> 
> 
> On 6/2/2023 4:51 AM, Ard Biesheuvel wrote:
>> Uncrustify checks are too rigid, making them counter-productive:
>>
>> - it leads to code that is arguably harder to parse visually (e.g.,
>>    the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit
>>    429309e0c6b74792)
> 
> Looking at commit 7f198321eec0f520373, I see positive changes like in 
> ArmCrashDumpDxe.c spacing in the ASSERT calls and treatment of 
> multi-line parameter formatting in the mCpu->RegisterInterruptHandler() 
> call are consistent.
> 
> That carries on for a number of C calls with inconsistent spacing before 
> opening parentheses and calls such as those to the DEBUG macro which I 
> find easier to read separating each parameter on a dedicated line.
> 
> In AArch64Mmu.h, I agree that preserving the (mostly) global column as 
> opposed to block-specific columns would be easier to vertically scan. Is 
> that the main issue in the file?
> 
>> - it forces indentation-only changes to code in the vicinity of actual
>>    changes, making the code history more bloated than necessary (see
>>    commit 7f198321eec0f520373 for an example)
> 
>> - finding out from the web UI what exactly Uncrustify objected to is not
>>    straight-forward.
>>
>> So let's enable AuditMode for ArmPkg, so that interested parties can see
>> the uncrustify recommendations if desired, but without preventing the
>> changes from being merged. This leaves it at the discretion of the
>> ArmPkg maintainers to decide which level of conformance is required.
>>
>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>> Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>>   ArmPkg/ArmPkg.ci.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
>> index 24db7425051388cf..d3124816118944cb 100644
>> --- a/ArmPkg/ArmPkg.ci.yaml
>> +++ b/ArmPkg/ArmPkg.ci.yaml
>> @@ -239,5 +239,10 @@
>>           ],
>>
>>           "AdditionalIncludePaths": [] # Additional paths to spell check
>>
>>                                        # (wildcards supported)
>>
>> +    },
>>
>> +
>>
>> +    # options defined in .pytool/Plugin/UncrustifyCheck
>>
>> +    "UncrustifyCheck": {
>>
>> +        "AuditOnly": True
>>
>>       }
>>
>>   }
>>
> 
> 
> 
> 

Re: [edk2-devel] [RFC PATCH] ArmPkg: Enable AuditMode for Uncrustify CI checks

[Michael Kubacki]

Are there particular areas that could be improved to make it more usable 
for you? I'm trying to find actionable improvements that can be made, if 
any.

I know it's not perfect but developers can run it with a single keyboard 
shortcut and it's been useful internally for eliminating style minutia 
from distracting design and correctness conversation in code reviews.

On 6/2/2023 4:51 AM, Ard Biesheuvel wrote:
> Uncrustify checks are too rigid, making them counter-productive:
> 
> - it leads to code that is arguably harder to parse visually (e.g.,
>    the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit
>    429309e0c6b74792)

Looking at commit 7f198321eec0f520373, I see positive changes like in 
ArmCrashDumpDxe.c spacing in the ASSERT calls and treatment of 
multi-line parameter formatting in the mCpu->RegisterInterruptHandler() 
call are consistent.

That carries on for a number of C calls with inconsistent spacing before 
opening parentheses and calls such as those to the DEBUG macro which I 
find easier to read separating each parameter on a dedicated line.

In AArch64Mmu.h, I agree that preserving the (mostly) global column as 
opposed to block-specific columns would be easier to vertically scan. Is 
that the main issue in the file?

> - it forces indentation-only changes to code in the vicinity of actual
>    changes, making the code history more bloated than necessary (see
>    commit 7f198321eec0f520373 for an example)

That's true. People adjusted things like indentation before depending on 
a given change, but it is predetermined now. Perhaps turning off column 
alignment (in general or in a given package) could help reduce noise 
from this.

I don't think alignment enforcement is necessary. That might also help 
address some of thrash in AArch64Mmu.h.

> - finding out from the web UI what exactly Uncrustify objected to is not
>    straight-forward.
> 

This should not be necessary. It can be run locally to produce the same 
result as in CI.

IDE-specific, but a lot of people use VS Code with the Uncrustify 
extension 
(https://marketplace.visualstudio.com/items?itemName=zachflower.uncrustify) 
and that allows Uncrustify to be run against the open source file by 
simply pressing the "code formatter" command.

I usually write the code, stage or commit it and then run this command 
on the file. The code is formatted and it gives a normal local diff of 
exactly what Uncrustify changed.

> So let's enable AuditMode for ArmPkg, so that interested parties can see
> the uncrustify recommendations if desired, but without preventing the
> changes from being merged. This leaves it at the discretion of the
> ArmPkg maintainers to decide which level of conformance is required.
> 
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   ArmPkg/ArmPkg.ci.yaml | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
> index 24db7425051388cf..d3124816118944cb 100644
> --- a/ArmPkg/ArmPkg.ci.yaml
> +++ b/ArmPkg/ArmPkg.ci.yaml
> @@ -239,5 +239,10 @@
>           ],
> 
>           "AdditionalIncludePaths": [] # Additional paths to spell check
> 
>                                        # (wildcards supported)
> 
> +    },
> 
> +
> 
> +    # options defined in .pytool/Plugin/UncrustifyCheck
> 
> +    "UncrustifyCheck": {
> 
> +        "AuditOnly": True
> 
>       }
> 
>   }
> 

Re: [edk2-devel] [RFC PATCH] ArmPkg: Enable AuditMode for Uncrustify CI checks

[Ard Biesheuvel]

On Fri, 2 Jun 2023 at 17:26, Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
>
> Are there particular areas that could be improved to make it more usable
> for you? I'm trying to find actionable improvements that can be made, if
> any.
>
> I know it's not perfect but developers can run it with a single keyboard
> shortcut and it's been useful internally for eliminating style minutia
> from distracting design and correctness conversation in code reviews.
>

I don't disagree with that. But I just don't find it as important as
other people seem to feel it is, and combined with the flaky CI infra
(I just had to amend and push another PR [0] due to infra failure), I
am finding that just getting changes merged is taking a substantial
amount of time, which I'd prefer to spend on other things.

The coding style that uncrustify imposes is overly rigid, and leaves
no room at all for any discretion on the part of the maintainer.

[0] https://github.com/tianocore/edk2/pull/4470

>
> On 6/2/2023 4:51 AM, Ard Biesheuvel wrote:
> > Uncrustify checks are too rigid, making them counter-productive:
> >
> > - it leads to code that is arguably harder to parse visually (e.g.,
> >    the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit
> >    429309e0c6b74792)
>
> Looking at commit 7f198321eec0f520373, I see positive changes like in
> ArmCrashDumpDxe.c spacing in the ASSERT calls and treatment of
> multi-line parameter formatting in the mCpu->RegisterInterruptHandler()
> call are consistent.
>
> That carries on for a number of C calls with inconsistent spacing before
> opening parentheses and calls such as those to the DEBUG macro which I
> find easier to read separating each parameter on a dedicated line.
>

That may be true. But I personally don't think it is important enough
to a) rigidly enforce, and b) fix up for existing code (but that ship
sailed when we did the mass conversion)

> In AArch64Mmu.h, I agree that preserving the (mostly) global column as
> opposed to block-specific columns would be easier to vertically scan. Is
> that the main issue in the file?
>

Yes.

> > - it forces indentation-only changes to code in the vicinity of actual
> >    changes, making the code history more bloated than necessary (see
> >    commit 7f198321eec0f520373 for an example)
>
> That's true. People adjusted things like indentation before depending on
> a given change, but it is predetermined now. Perhaps turning off column
> alignment (in general or in a given package) could help reduce noise
> from this.
>

Note that I find this aspect quite important. In my opinion, the git
history *is* the code base. Everything we changed at any point in the
past, including the commit logs, is part of this, and mass whitespace
conversions, or spurious changes just to make the CI happy are
problematic to me. For this reason alone, I should be able to override
CI choices at my discretion as a maintainer.

> I don't think alignment enforcement is necessary. That might also help
> address some of thrash in AArch64Mmu.h.
>

Interesting. If there is room for configuration here, why does the
existing configuration deviate from the coding style guidelines we had
been using for years? AIUI, we use a private fork of uncrustify for
edk2, right? Are there any problematic aspects in the existing
guidelines that was difficult to automate?

> > - finding out from the web UI what exactly Uncrustify objected to is not
> >    straight-forward.
> >
>
> This should not be necessary. It can be run locally to produce the same
> result as in CI.
>
> IDE-specific, but a lot of people use VS Code with the Uncrustify
> extension
> (https://marketplace.visualstudio.com/items?itemName=zachflower.uncrustify)
> and that allows Uncrustify to be run against the open source file by
> simply pressing the "code formatter" command.
>
> I usually write the code, stage or commit it and then run this command
> on the file. The code is formatted and it gives a normal local diff of
> exactly what Uncrustify changed.
>

I'm sure this all seems quite reasonable if you already bought into
using uncrustify. But for a drive-by contributor, or someone like me
who has been contributing code for many years based on the agreed
coding style guidelines, I struggle to understand why uncrustify is a
reasonable solution to the problem of inconsistent coding style.

I'm all for legible code, don't get me wrong. (And Leif is much more
pedantic in that sense than I am). But in my experience, the
uncrustify rules are too rigid, and arbitrary (two spaces indentation
here, four spaces there, etc etc)

I suppose having a button in the GitHub UI that simply lets us
exercise our discretion as maintainers to deviate from these rules
would go a long way in addressing these concerns. But as it stands
now, having my carefully crafted contributions rejected by an
automated system based on guidelines that deviate from the ones that
we agreed to, without any recourse, is simply not acceptable.




> > So let's enable AuditMode for ArmPkg, so that interested parties can see
> > the uncrustify recommendations if desired, but without preventing the
> > changes from being merged. This leaves it at the discretion of the
> > ArmPkg maintainers to decide which level of conformance is required.
> >
> > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
> > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >   ArmPkg/ArmPkg.ci.yaml | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
> > index 24db7425051388cf..d3124816118944cb 100644
> > --- a/ArmPkg/ArmPkg.ci.yaml
> > +++ b/ArmPkg/ArmPkg.ci.yaml
> > @@ -239,5 +239,10 @@
> >           ],
> >
> >           "AdditionalIncludePaths": [] # Additional paths to spell check
> >
> >                                        # (wildcards supported)
> >
> > +    },
> >
> > +
> >
> > +    # options defined in .pytool/Plugin/UncrustifyCheck
> >
> > +    "UncrustifyCheck": {
> >
> > +        "AuditOnly": True
> >
> >       }
> >
> >   }
> >

Re: [edk2-devel] [RFC PATCH] ArmPkg: Enable AuditMode for Uncrustify CI checks

[Michael Kubacki]

On 6/2/2023 11:50 AM, Ard Biesheuvel wrote:
> On Fri, 2 Jun 2023 at 17:26, Michael Kubacki
> <mikuback@linux.microsoft.com> wrote:
>>
>> Are there particular areas that could be improved to make it more usable
>> for you? I'm trying to find actionable improvements that can be made, if
>> any.
>>
>> I know it's not perfect but developers can run it with a single keyboard
>> shortcut and it's been useful internally for eliminating style minutia
>> from distracting design and correctness conversation in code reviews.
>>
> 
> I don't disagree with that. But I just don't find it as important as
> other people seem to feel it is, and combined with the flaky CI infra
> (I just had to amend and push another PR [0] due to infra failure), I
> am finding that just getting changes merged is taking a substantial
> amount of time, which I'd prefer to spend on other things.
> 
> The coding style that uncrustify imposes is overly rigid, and leaves
> no room at all for any discretion on the part of the maintainer.
> 
> [0] https://github.com/tianocore/edk2/pull/4470
> 
>>
>> On 6/2/2023 4:51 AM, Ard Biesheuvel wrote:
>>> Uncrustify checks are too rigid, making them counter-productive:
>>>
>>> - it leads to code that is arguably harder to parse visually (e.g.,
>>>     the changes to ArmPkg/Include/Chipset/AArch64Mmu.h in commit
>>>     429309e0c6b74792)
>>
>> Looking at commit 7f198321eec0f520373, I see positive changes like in
>> ArmCrashDumpDxe.c spacing in the ASSERT calls and treatment of
>> multi-line parameter formatting in the mCpu->RegisterInterruptHandler()
>> call are consistent.
>>
>> That carries on for a number of C calls with inconsistent spacing before
>> opening parentheses and calls such as those to the DEBUG macro which I
>> find easier to read separating each parameter on a dedicated line.
>>
> 
> That may be true. But I personally don't think it is important enough
> to a) rigidly enforce, and b) fix up for existing code (but that ship
> sailed when we did the mass conversion)
> 

Yeah, (b) is done and enforcement can be opted by package for this reason.

>> In AArch64Mmu.h, I agree that preserving the (mostly) global column as
>> opposed to block-specific columns would be easier to vertically scan. Is
>> that the main issue in the file?
>>
> 
> Yes.
> 

Thanks for confirming.

>>> - it forces indentation-only changes to code in the vicinity of actual
>>>     changes, making the code history more bloated than necessary (see
>>>     commit 7f198321eec0f520373 for an example)
>>
>> That's true. People adjusted things like indentation before depending on
>> a given change, but it is predetermined now. Perhaps turning off column
>> alignment (in general or in a given package) could help reduce noise
>> from this.
>>
> 
> Note that I find this aspect quite important. In my opinion, the git
> history *is* the code base. Everything we changed at any point in the
> past, including the commit logs, is part of this, and mass whitespace
> conversions, or spurious changes just to make the CI happy are
> problematic to me. For this reason alone, I should be able to override
> CI choices at my discretion as a maintainer.
> 

I think there's a balance to not have wildly inconsistent style (reason 
the guidelines were introduced in the first place), reduce maintainer 
overhead for style only suggestions, and keep a clean history. But I 
completely agree with the importance of git history.

>> I don't think alignment enforcement is necessary. That might also help
>> address some of thrash in AArch64Mmu.h.
>>
> 
> Interesting. If there is room for configuration here, why does the
> existing configuration deviate from the coding style guidelines we had
> been using for years? AIUI, we use a private fork of uncrustify for
> edk2, right? Are there any problematic aspects in the existing
> guidelines that was difficult to automate?
> 

Yes, there were several. Some not entirely related to the guidelines but 
common implementation patterns. Multi-line parameter indentation in 
function prototypes was not supported. Special handling of double 
parentheses in DEBUG macro calls was formatted in very weird ways. 
Inconsistent use of OPTIONAL before and after commas in parameter lists 
led to varying outcomes. Inline assembly formatting just didn't work 
well at all (compared to what was already in the code base) and was 
disabled. The edk2 configuration is in [1].

[1] - 
https://github.com/tianocore/edk2/blob/master/.pytool/Plugin/UncrustifyCheck/uncrustify.cfg

>>> - finding out from the web UI what exactly Uncrustify objected to is not
>>>     straight-forward.
>>>
>>
>> This should not be necessary. It can be run locally to produce the same
>> result as in CI.
>>
>> IDE-specific, but a lot of people use VS Code with the Uncrustify
>> extension
>> (https://marketplace.visualstudio.com/items?itemName=zachflower.uncrustify)
>> and that allows Uncrustify to be run against the open source file by
>> simply pressing the "code formatter" command.
>>
>> I usually write the code, stage or commit it and then run this command
>> on the file. The code is formatted and it gives a normal local diff of
>> exactly what Uncrustify changed.
>>
> 
> I'm sure this all seems quite reasonable if you already bought into
> using uncrustify. But for a drive-by contributor, or someone like me
> who has been contributing code for many years based on the agreed
> coding style guidelines, I struggle to understand why uncrustify is a
> reasonable solution to the problem of inconsistent coding style.
> 
> I'm all for legible code, don't get me wrong. (And Leif is much more
> pedantic in that sense than I am). But in my experience, the
> uncrustify rules are too rigid, and arbitrary (two spaces indentation
> here, four spaces there, etc etc)
> 
> I suppose having a button in the GitHub UI that simply lets us
> exercise our discretion as maintainers to deviate from these rules
> would go a long way in addressing these concerns. But as it stands
> now, having my carefully crafted contributions rejected by an
> automated system based on guidelines that deviate from the ones that
> we agreed to, without any recourse, is simply not acceptable.
> 

I understand and long time contributors generally done a good job 
maintaining the style. It is difficult to suggest delta to contributor 
only changes in future commits if specific code does not meet the rules 
in prior commits though.

> 
> 
> 
>>> So let's enable AuditMode for ArmPkg, so that interested parties can see
>>> the uncrustify recommendations if desired, but without preventing the
>>> changes from being merged. This leaves it at the discretion of the
>>> ArmPkg maintainers to decide which level of conformance is required.
>>>
>>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>>> Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
>>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>>    ArmPkg/ArmPkg.ci.yaml | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
>>> index 24db7425051388cf..d3124816118944cb 100644
>>> --- a/ArmPkg/ArmPkg.ci.yaml
>>> +++ b/ArmPkg/ArmPkg.ci.yaml
>>> @@ -239,5 +239,10 @@
>>>            ],
>>>
>>>            "AdditionalIncludePaths": [] # Additional paths to spell check
>>>
>>>                                         # (wildcards supported)
>>>
>>> +    },
>>>
>>> +
>>>
>>> +    # options defined in .pytool/Plugin/UncrustifyCheck
>>>
>>> +    "UncrustifyCheck": {
>>>
>>> +        "AuditOnly": True
>>>
>>>        }
>>>
>>>    }
>>>
> 
> 
> 
> 

Re: [edk2-devel] [RFC PATCH] ArmPkg: Enable AuditMode for Uncrustify CI checks

[Gerd Hoffmann]

  Hi,

> > In AArch64Mmu.h, I agree that preserving the (mostly) global column as
> > opposed to block-specific columns would be easier to vertically scan. Is
> > that the main issue in the file?
> 
> Yes.

It is possible to exclude specific files from checks, see CryptoPkg for
example which does this for some files generated by openssl configure.

> I'm sure this all seems quite reasonable if you already bought into
> using uncrustify. But for a drive-by contributor, or someone like me
> who has been contributing code for many years based on the agreed
> coding style guidelines, I struggle to understand why uncrustify is a
> reasonable solution to the problem of inconsistent coding style.

I find it quite nice.  Yes, you have to invest the time to set things up
once.  But then you can just delegate all code formating to uncrustify
instead of doing it manually.

take care,
  Gerd