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

["Ard Biesheuvel" <ardb@kernel.org>]

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
> >
> >       }
> >
> >   }
> >