Re: [edk2-devel] edk2 uncrustify update (73.0.8)?

["Laszlo Ersek" <lersek@redhat.com>]

On 11/16/23 09:29, Pedro Falcato wrote:
> On Tue, Nov 14, 2023 at 3:01 PM Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 11/13/23 22:33, Pedro Falcato wrote:
>>> On Mon, Nov 13, 2023 at 8:37 PM Rebecca Cran <rebecca@bsdio.com> wrote:
>>>>
>>>> On 11/13/2023 1:08 PM, Michael Kubacki wrote:
>>>>> Yes. I just did it. It is relatively minor and impacts expected code
>>>>> areas.
>>>>>
>>>>> https://github.com/tianocore/edk2/pull/5043/files
>>>>
>>>>
>>>> Could you update .git-blame-ignore-revs please?
>>>
>>> You can't do that until the merge is done, since we use
>>> rebase-and-merge for tianocore (and rebases do not keep stable commit
>>> hashes).
>>> But I would plead that this should not get merged in general :/
>>
> 
> Laszlo!
> 
> Sorry for the delay.
> 
>> Seeing the cumulative diff in that PR, do you have specific
>> counter-arguments?
> 
> Well, my counter-argument is that formatting is becoming a topic of
> its own. I used to be very pro-formatter but if this leads to either
> 1) eyesore or 2) weird churn every now and then,
> I feel like we should reconsider the current approach. I feel like all
> formatting (manual or automated) is fine as long as it's:
> 1) Visually consistent with the codebase's style
> 2) Not horrendous to look at

That was what we did first, for several years. It wasn't working well.

An edk2 coding style document had always existed, but it had sufficient
corner cases that formatting had *always* been a topic of its own. In
particular, point (2) "not horrendous to look at" is impossible to define.

- edk2 mainly uses a Windows-like coding style, which is immediately
horrendous to anyone coming from a Linux background, at first

- edk2 uses extremely long lines in some modules (200+ characters). I
personally use 80 character lines (for good reason -- not the greatest
eyesight, so I need to use a relatively large font, and I like to place
two columns of source code on my screen side-by-side, for diffing or
comparing otherwise). I've received multiple "buy a larger monitor" or
"use two monitors at the same time", and those don't work for me either.
Some people are super productive with 30" monitors, I'm not. Therefore,
"long lines or not" can never be decided by democracy, only by decree.
uncrustify is decree.

- somewhat as a consequence of my avoidance of long lines, I tended to
break up long function calls earlier / more frequently than other
developers. In order to conserve *vertical* screen real estate, I used a
"condensed" multi-line argument style for function calls, in OvmfPkg and
ArmVirtPkg. (The official multi-line style is more wasteful.) That
wasn't working for other project participants. Uncrustify now does not
permit my preferred (condensed) style, but at least there are no more
debates about it.

Your premise is that those two points form a stable foundation, but they
don't. We tried.


> and switching back and forth because 'magic indentation tool' says so
> just seems silly to me.

I don't perceive this update as switching back and forth; it seems like
a minor tweak.


>> The diff is trivial, IMO. You mentioned "error prone" and "much churn",
>> which are very valid concerns, but they don't seem to apply here. We can
>> review a diff of this size (especially if it's split up on Pkg
>> boundaries), and the github view indicates the change is only in
>> whitespace amount.
>>
>> The change in
>> "OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c"
>> is a net win; the current formatting is really distracting.
>>
>> Furthermore, this diff actually highlights some inexplicable syntax in
>> "EmulatorPkg/FvbServicesRuntimeDxe/FvbInfo.c": those backslashes (not
>> parts of any macro definition) are an eyesore. They should be fixed
>> regardless of re-uncrustification.
>>
>> The version N vs. N+1 concern shouldn't be one; the authoritative
>> version is what the YAML file in edk2 says.
> 
> Well, I fear it's not that simple. EDK2's uncrustify has historically
> been a PITA, I've had to convince people to give it a try, I myself
> don't even know how I installed it (IIRC, some weird random
> combination of unzip + the NuGet...).

(1) The following file in edk2:

  .pytool/Plugin/UncrustifyCheck/Readme.md

highlights the "Project Mu Uncrustify fork source code and documentation":

  https://dev.azure.com/projectmu/Uncrustify

(This file is easy to find in edk2 just by grepping for "uncrustify".)

(2) The "Repos" link in the sidebar of that page leads to:

  https://dev.azure.com/projectmu/_git/Uncrustify

A Clone button appears then, with the project URL being

  https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify

(3) This project can be cloned, and built on Linux very easily. It has a
top-level README.md file with instructions. It needs "cmake" and "make".

(4) The actual version to check out and build -- in order to match the
github CI -- is captured in edk2's file

  .pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml

(5) A bit of digging in edk2's

  .pytool/Plugin/UncrustifyCheck/UncrustifyCheck.py

explains that the edk2 uncrustify config file to use with the binary
from (3) and (4) is

  .pytool/Plugin/UncrustifyCheck/uncrustify.cfg

plus further useful command line options.

(6) Based on all that, I run uncrustify locally as follows:

  uncrustify \
    -c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg \
    -F file-list \
    --replace \
    --no-backup \
    --if-changed

I dislike downloading binaries and/or containers just like many others.
Luckily, in this case, it's easy to build a local binary from source,
and to run it.

> Getting everyone on the same version would already be hard even if the
> software was easy to install.

I agree the documentation does not target a native, local build.

Most users (i.e., most developers) seem to prefer something that "just
works", be that a remotely build binary, or a container. That's probably
why the documentation explains that kind of usage.

I've not suggested extending the documentation with something like the
above bullet list because I consider my preference (i.e., for a local
build) to be "niche".

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111343): https://edk2.groups.io/g/devel/message/111343
Mute This Topic: https://groups.io/mt/102559740/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-