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

["Michael Kubacki" <mikuback@linux.microsoft.com>]

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