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

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

[Laszlo Ersek]

Hi Michael,

recently I encountered an uncrustify failure on github.

The reason was that my local uncrustify was *more recent* (73.0.8) than
the one we use in edk2 CI (which is 73.0.3, per the edk2 file
".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml").

Updating the version number in the YAML file (i.e., advancing edk2 to
version 73.0.8) seems easy enough, but:

- Do you think 73.0.8 is mature enough for adoption in edk2?

  This upstream uncrustify release was tagged in April (and I can't see
  any more recent commits), so I assume it should be stable.

- Would the version update require a whole-tree re-uncrustification?

The reason I'm not just ignoring this topic is that 73.0.8 actually
produces *better output* than 73.0.3, at least in the one instance I
encountered. Compare:

> diff --git a/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c b/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
> index 434cdca84b23..3a6f75988220 100644
> --- a/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
> +++ b/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
> @@ -43,12 +43,12 @@ STATIC EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL
>  STATIC CONST EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  mMmio64Configuration = {
>    ACPI_ADDRESS_SPACE_DESCRIPTOR,                   // Desc
>    (UINT16)(                                        // Len
> -                                                   sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
> -                                                   OFFSET_OF (
> -                                                     EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
> -                                                     ResType
> -                                                     )
> -                                                   ),
> +    sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
> +    OFFSET_OF (
> +      EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
> +      ResType
> +      )
> +    ),
>    ACPI_ADDRESS_SPACE_TYPE_MEM,                     // ResType
>    0,                                               // GenFlag
>    0,                                               // SpecificFlag
> @@ -77,12 +77,12 @@ STATIC CONST EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  mMmio64Configuration = {
>  STATIC CONST EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  mOptionRomConfiguration =   {
>    ACPI_ADDRESS_SPACE_DESCRIPTOR,                   // Desc
>    (UINT16)(                                        // Len
> -                                                   sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
> -                                                   OFFSET_OF (
> -                                                     EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
> -                                                     ResType
> -                                                     )
> -                                                   ),
> +    sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
> +    OFFSET_OF (
> +      EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
> +      ResType
> +      )
> +    ),
>    ACPI_ADDRESS_SPACE_TYPE_MEM,                     // ResType
>    0,                                               // GenFlag
>    0,                                               // Disable option roms SpecificFlag

Note that 73.0.3 indents the subexpression to the "//"  comment on the
previous line, while 73.0.8 ignores the comment -- which I think is
justified here.

I believe this improvement may come from uncrustify commit 239c4fad745b
("Prevent endless indentation scenario in struct assignment",
2022-07-29). I think it's worth having in edk2.

CC: stewards, Pedro (commit 6ded9f50c3aa), Marcin (traditionally a big
fan of uncrustify :))

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111147): https://edk2.groups.io/g/devel/message/111147
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Marcin Juszkiewicz]

W dniu 13.11.2023 o 12:58, Laszlo Ersek pisze:
> Note that 73.0.3 indents the subexpression to the "//"  comment on the
> previous line, while 73.0.8 ignores the comment -- which I think is
> justified here.
> 
> I believe this improvement may come from uncrustify commit 239c4fad745b
> ("Prevent endless indentation scenario in struct assignment",
> 2022-07-29). I think it's worth having in edk2.

I like this.

> CC: stewards, Pedro (commit 6ded9f50c3aa), Marcin (traditionally a big
> fan of uncrustify :))

I prefer to have source formatter than not having it. Especially in 
projects which have code style far from mine.

Still a fan of adding edk2-uncrustify to BaseTools. If we are expected 
to use it then let it get installed at same moment as "build" command is.


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

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

[Rebecca Cran via groups.io]

On 11/13/2023 5:29 AM, Marcin Juszkiewicz via groups.io wrote:

> Still a fan of adding edk2-uncrustify to BaseTools. If we are expected 
> to use it then let it get installed at same moment as "build" command is.

The issue with doing this is there's a push to remove all C/C++ code 
from BaseTools (including porting existing code to Python), and adding 
edk2-uncrustify would work against that.

-- 
Rebecca Cran




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

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

[Michael Kubacki]

On 11/13/2023 2:14 PM, Rebecca Cran via groups.io wrote:
> On 11/13/2023 5:29 AM, Marcin Juszkiewicz via groups.io wrote:
> 
>> Still a fan of adding edk2-uncrustify to BaseTools. If we are expected 
>> to use it then let it get installed at same moment as "build" command is.
> 
> The issue with doing this is there's a push to remove all C/C++ code 
> from BaseTools (including porting existing code to Python), and adding 
> edk2-uncrustify would work against that.
> 
That was a reason I did not add the code to BaseTools. I personally 
think it would bloat the edk2 repo and complicate its build process for 
something that rarely changes. Given binary dependencies are already 
used and managed for other incoming binaries such as iasl and nasm 
(https://github.com/tianocore/edk2/tree/master/BaseTools/Bin) and the 
effort to eliminate C/C++ code from BaseTools, I think it makes sense to 
keep it in a dedicated repository.

If there is another reason for the move, such as a preference to move 
the repo under the Tianocore organization (for whatever reason) or 
something like that, please let me know.


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

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

[Pedro Falcato]

On Mon, Nov 13, 2023 at 11:58 AM Laszlo Ersek <lersek@redhat.com> wrote:
>
> Hi Michael,
>
> recently I encountered an uncrustify failure on github.
>
> The reason was that my local uncrustify was *more recent* (73.0.8) than
> the one we use in edk2 CI (which is 73.0.3, per the edk2 file
> ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml").

Wait, you can use upstream uncrustify? I'm just using whatever
uncrustify version I took from the project-mu fork...

>
> Updating the version number in the YAML file (i.e., advancing edk2 to
> version 73.0.8) seems easy enough, but:
>
> - Do you think 73.0.8 is mature enough for adoption in edk2?
>
>   This upstream uncrustify release was tagged in April (and I can't see
>   any more recent commits), so I assume it should be stable.
>
> - Would the version update require a whole-tree re-uncrustification?

Please, no. I didn't mind doing an initial reformatting at first, but
doing this continuously is both 1) problem-prone 2) just amazing
amounts of churn.
Let's say I have version N, you have version N+1 - we may never get
any final, formatted output as your version formats it differently
from mine.

I don't know how the CI is doing its thing atm (I haven't merged
anything myself to edk2), but the uncrustify check should be relaxed
to just a warning. There's nothing wrong with what my uncrustify
version is formatting to, there's nothing wrong with yours either, and
CI isn't necessarily wrong either.

And, to be fair, I already find uncrustify a large pain in the butt to
use (requiring a custom fork really does not help), but I find the
benefits worth it *locally*, as my coding style is also quite
different from the NT-esque style.

-- 
Pedro


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

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

[Michael Kubacki]

On 11/13/2023 2:07 PM, Pedro Falcato wrote:
> On Mon, Nov 13, 2023 at 11:58 AM Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> Hi Michael,
>>
>> recently I encountered an uncrustify failure on github.
>>
>> The reason was that my local uncrustify was *more recent* (73.0.8) than
>> the one we use in edk2 CI (which is 73.0.3, per the edk2 file
>> ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml").
> 
> Wait, you can use upstream uncrustify? I'm just using whatever
> uncrustify version I took from the project-mu fork...
> 
The fork version is needed for edk2 specific conventions. More details 
are here - 
https://dev.azure.com/projectmu/uncrustify?anchor=edk-ii-poc-details

>>
>> Updating the version number in the YAML file (i.e., advancing edk2 to
>> version 73.0.8) seems easy enough, but:
>>
>> - Do you think 73.0.8 is mature enough for adoption in edk2?
>>
>>    This upstream uncrustify release was tagged in April (and I can't see
>>    any more recent commits), so I assume it should be stable.
>>
>> - Would the version update require a whole-tree re-uncrustification?
> 
> Please, no. I didn't mind doing an initial reformatting at first, but
> doing this continuously is both 1) problem-prone 2) just amazing
> amounts of churn.
> Let's say I have version N, you have version N+1 - we may never get
> any final, formatted output as your version formats it differently
> from mine.
> 
> I don't know how the CI is doing its thing atm (I haven't merged
> anything myself to edk2), but the uncrustify check should be relaxed
> to just a warning. There's nothing wrong with what my uncrustify
> version is formatting to, there's nothing wrong with yours either, and
> CI isn't necessarily wrong either.
> 
> And, to be fair, I already find uncrustify a large pain in the butt to
> use (requiring a custom fork really does not help), but I find the
> benefits worth it *locally*, as my coding style is also quite
> different from the NT-esque style.
> 
It should be simple to update and ensure everyone is using the same 
version. This requires stuart commands to be used 
(https://github.com/tianocore/edk2-pytool-extensions). I know there's 
aversion to stuart but that's how these extensions plug into the edk2 
build process right now.

If you use it, as an end user, you just run "stuart_update -c 
.pytool/CISettings.py" and it will get the Uncrustify binary for your 
host OS with the version used by the project.

---

The version pulled and the source feed used by stuart are defined in 
edk2 here: 
https://github.com/tianocore/edk2/blob/master/.pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml

That file and command are used locally, in CI, and the file is checked 
into edk2. At any given point in time, a user at a given point in edk2 
history should be using the same version and configuration.

More details, for those interested, are here 
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-Formatting. 
That tries to cover some niche use cases so it may seem more 
overwhelming than it actually is to just get and use the executable.


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

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

[Michael D Kinney]

Hi Michael,

Have you tried to upstream the edk2 specific elements to the
uncrustify project?  That would be a path to remove the fork
and eventually all the distros would catch up.

Mike

> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Monday, November 13, 2023 12:22 PM
> To: Pedro Falcato <pedro.falcato@gmail.com>; devel@edk2.groups.io;
> lersek@redhat.com
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Andrew Fish
> <afish@apple.com>; Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>;
> Leif Lindholm (Quic) <quic_llindhol@quicinc.com>
> Subject: Re: [edk2-devel] edk2 uncrustify update (73.0.8)?
> 
> On 11/13/2023 2:07 PM, Pedro Falcato wrote:
> > On Mon, Nov 13, 2023 at 11:58 AM Laszlo Ersek <lersek@redhat.com>
> wrote:
> >>
> >> Hi Michael,
> >>
> >> recently I encountered an uncrustify failure on github.
> >>
> >> The reason was that my local uncrustify was *more recent* (73.0.8)
> than
> >> the one we use in edk2 CI (which is 73.0.3, per the edk2 file
> >> ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml").
> >
> > Wait, you can use upstream uncrustify? I'm just using whatever
> > uncrustify version I took from the project-mu fork...
> >
> The fork version is needed for edk2 specific conventions. More details
> are here -
> https://dev.azure.com/projectmu/uncrustify?anchor=edk-ii-poc-details
> 
> >>
> >> Updating the version number in the YAML file (i.e., advancing edk2
> to
> >> version 73.0.8) seems easy enough, but:
> >>
> >> - Do you think 73.0.8 is mature enough for adoption in edk2?
> >>
> >>    This upstream uncrustify release was tagged in April (and I
> can't see
> >>    any more recent commits), so I assume it should be stable.
> >>
> >> - Would the version update require a whole-tree re-
> uncrustification?
> >
> > Please, no. I didn't mind doing an initial reformatting at first,
> but
> > doing this continuously is both 1) problem-prone 2) just amazing
> > amounts of churn.
> > Let's say I have version N, you have version N+1 - we may never get
> > any final, formatted output as your version formats it differently
> > from mine.
> >
> > I don't know how the CI is doing its thing atm (I haven't merged
> > anything myself to edk2), but the uncrustify check should be relaxed
> > to just a warning. There's nothing wrong with what my uncrustify
> > version is formatting to, there's nothing wrong with yours either,
> and
> > CI isn't necessarily wrong either.
> >
> > And, to be fair, I already find uncrustify a large pain in the butt
> to
> > use (requiring a custom fork really does not help), but I find the
> > benefits worth it *locally*, as my coding style is also quite
> > different from the NT-esque style.
> >
> It should be simple to update and ensure everyone is using the same
> version. This requires stuart commands to be used
> (https://github.com/tianocore/edk2-pytool-extensions). I know there's
> aversion to stuart but that's how these extensions plug into the edk2
> build process right now.
> 
> If you use it, as an end user, you just run "stuart_update -c
> .pytool/CISettings.py" and it will get the Uncrustify binary for your
> host OS with the version used by the project.
> 
> ---
> 
> The version pulled and the source feed used by stuart are defined in
> edk2 here:
> https://github.com/tianocore/edk2/blob/master/.pytool/Plugin/Uncrustif
> yCheck/uncrustify_ext_dep.yaml
> 
> That file and command are used locally, in CI, and the file is checked
> into edk2. At any given point in time, a user at a given point in edk2
> history should be using the same version and configuration.
> 
> More details, for those interested, are here
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-
> Formatting.
> That tries to cover some niche use cases so it may seem more
> overwhelming than it actually is to just get and use the executable.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111181): https://edk2.groups.io/g/devel/message/111181
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 11/13/23 20:07, Pedro Falcato wrote:
> On Mon, Nov 13, 2023 at 11:58 AM Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> Hi Michael,
>>
>> recently I encountered an uncrustify failure on github.
>>
>> The reason was that my local uncrustify was *more recent* (73.0.8) than
>> the one we use in edk2 CI (which is 73.0.3, per the edk2 file
>> ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml").
> 
> Wait, you can use upstream uncrustify? I'm just using whatever
> uncrustify version I took from the project-mu fork...

Apologies -- for edk2 purposes (and I don't use uncrustify for anything
else), I consider
<https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify>
"upstream".

> 
>>
>> Updating the version number in the YAML file (i.e., advancing edk2 to
>> version 73.0.8) seems easy enough, but:
>>
>> - Do you think 73.0.8 is mature enough for adoption in edk2?
>>
>>   This upstream uncrustify release was tagged in April (and I can't see
>>   any more recent commits), so I assume it should be stable.
>>
>> - Would the version update require a whole-tree re-uncrustification?
> 
> Please, no. I didn't mind doing an initial reformatting at first, but
> doing this continuously is both 1) problem-prone 2) just amazing
> amounts of churn.
> Let's say I have version N, you have version N+1 - we may never get
> any final, formatted output as your version formats it differently
> from mine.

Your argument against a continuously reformatted code base is well
received; just a small correction: we should never have an N vs. N+1
dilemma. What CI uses is the authoritative version.

> 
> I don't know how the CI is doing its thing atm (I haven't merged
> anything myself to edk2), but the uncrustify check should be relaxed
> to just a warning.

I don't think that's going to happen. Everybody ignores warnings when in
a rush, or when the same warnings pop up for the 10th time.

> There's nothing wrong with what my uncrustify
> version is formatting to, there's nothing wrong with yours either, and
> CI isn't necessarily wrong either.
> 
> And, to be fair, I already find uncrustify a large pain in the butt to
> use (requiring a custom fork really does not help), but I find the
> benefits worth it *locally*, as my coding style is also quite
> different from the NT-esque style.

Funnily enough, my stance is quite the opposite. I happen to disagree
with some patterns that uncrustify enforces, but I'm thankful that at
any given state of CI (= using any given version of uncrustify), we
can't have any more debates about patch formatting (that is, it's
especially its central nature that I like). I've found uncrustify
relatively easy to use locally, too.

All in all I'm not trying to upset the status quo, it's just a question
about a version bump, and how we'd deal with any fallout from that.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111200): https://edk2.groups.io/g/devel/message/111200
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Rebecca Cran via groups.io]

On 11/14/2023 7:51 AM, Laszlo Ersek via groups.io wrote:

> Funnily enough, my stance is quite the opposite. I happen to disagree
> with some patterns that uncrustify enforces, but I'm thankful that at
> any given state of CI (= using any given version of uncrustify), we
> can't have any more debates about patch formatting (that is, it's
> especially its central nature that I like). I've found uncrustify
> relatively easy to use locally, too.

There's _one_ place we can still have a debate, but I'm hoping we can 
easily agree, and update CI to enforce it.

I'd like to scrub the tree of all the NT-style function documentation 
blocks and replace them with Doxygen style.

As an example of the NT style, see OvmfPkg/QemuVideoDxe/Gop.c:

EFI_STATUS
EFIAPI
QemuVideoGraphicsOutputQueryMode (
   IN  EFI_GRAPHICS_OUTPUT_PROTOCOL          *This,
   IN  UINT32                                ModeNumber,
   OUT UINTN                                 *SizeOfInfo,
   OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  **Info
   )

/*++

Routine Description:

   Graphics Output protocol interface to query video mode

   Arguments:
     This                  - Protocol instance pointer.
     ModeNumber            - The mode number to return information on.
     Info                  - Caller allocated buffer that returns 
information about ModeNumber.
     SizeOfInfo            - A pointer to the size, in bytes, of the 
Info buffer.

   Returns:
     EFI_SUCCESS           - Mode information returned.
     EFI_BUFFER_TOO_SMALL  - The Info buffer was too small.
     EFI_DEVICE_ERROR      - A hardware error occurred trying to 
retrieve the video mode.
     EFI_NOT_STARTED       - Video display is not initialized. Call 
SetMode ()
     EFI_INVALID_PARAMETER - One of the input args was NULL.

--*/
{
   QEMU_VIDEO_PRIVATE_DATA  *Private;
   QEMU_VIDEO_MODE_DATA     *ModeData;
...


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

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

[Laszlo Ersek]

On 11/14/23 16:12, Rebecca Cran wrote:
> On 11/14/2023 7:51 AM, Laszlo Ersek via groups.io wrote:
> 
>> Funnily enough, my stance is quite the opposite. I happen to disagree
>> with some patterns that uncrustify enforces, but I'm thankful that at
>> any given state of CI (= using any given version of uncrustify), we
>> can't have any more debates about patch formatting (that is, it's
>> especially its central nature that I like). I've found uncrustify
>> relatively easy to use locally, too.
> 
> There's _one_ place we can still have a debate, but I'm hoping we can
> easily agree, and update CI to enforce it.
> 
> I'd like to scrub the tree of all the NT-style function documentation
> blocks and replace them with Doxygen style.
> 
> As an example of the NT style, see OvmfPkg/QemuVideoDxe/Gop.c:
> 
> EFI_STATUS
> EFIAPI
> QemuVideoGraphicsOutputQueryMode (
>   IN  EFI_GRAPHICS_OUTPUT_PROTOCOL          *This,
>   IN  UINT32                                ModeNumber,
>   OUT UINTN                                 *SizeOfInfo,
>   OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  **Info
>   )
> 
> /*++
> 
> Routine Description:
> 
>   Graphics Output protocol interface to query video mode
> 
>   Arguments:
>     This                  - Protocol instance pointer.
>     ModeNumber            - The mode number to return information on.
>     Info                  - Caller allocated buffer that returns
> information about ModeNumber.
>     SizeOfInfo            - A pointer to the size, in bytes, of the Info
> buffer.
> 
>   Returns:
>     EFI_SUCCESS           - Mode information returned.
>     EFI_BUFFER_TOO_SMALL  - The Info buffer was too small.
>     EFI_DEVICE_ERROR      - A hardware error occurred trying to retrieve
> the video mode.
>     EFI_NOT_STARTED       - Video display is not initialized. Call
> SetMode ()
>     EFI_INVALID_PARAMETER - One of the input args was NULL.
> 
> --*/
> {
>   QEMU_VIDEO_PRIVATE_DATA  *Private;
>   QEMU_VIDEO_MODE_DATA     *ModeData;
> ...
> 

I strongly support that motion; the cited comment style is an eyesore.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111243): https://edk2.groups.io/g/devel/message/111243
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Pedro Falcato]

On Mon, Nov 13, 2023 at 7:07 PM Pedro Falcato via groups.io
<pedro.falcato=gmail.com@groups.io> wrote:
>
> On Mon, Nov 13, 2023 at 11:58 AM Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > Hi Michael,
> >
> > recently I encountered an uncrustify failure on github.
> >
> > The reason was that my local uncrustify was *more recent* (73.0.8) than
> > the one we use in edk2 CI (which is 73.0.3, per the edk2 file
> > ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml").
>
> Wait, you can use upstream uncrustify? I'm just using whatever
> uncrustify version I took from the project-mu fork...

Scratch that, I see there's a 73.0.8 in mu's fork.

-- 
Pedro


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

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

[Michael Kubacki]

On 11/13/2023 6:58 AM, Laszlo Ersek wrote:
> Hi Michael,
> 
> recently I encountered an uncrustify failure on github.
> 
> The reason was that my local uncrustify was *more recent* (73.0.8) than
> the one we use in edk2 CI (which is 73.0.3, per the edk2 file
> ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml").
> 
> Updating the version number in the YAML file (i.e., advancing edk2 to
> version 73.0.8) seems easy enough, but:
> 
> - Do you think 73.0.8 is mature enough for adoption in edk2?
> 
>    This upstream uncrustify release was tagged in April (and I can't see
>    any more recent commits), so I assume it should be stable.
> 
Yes, it is stable. We've been using each new Uncrustify release against 
edk2 code in Project Mu during that time. I updated our code to include 
that change in September 2022 - 
https://github.com/microsoft/mu_basecore/commit/6932526bee9a2f5f3af7588923beae5e5d8fd128.

The changes since then have been additional build support for Linux and 
Windows Arm and macOS.

I originally did not bring this to edk2 right away to verify stability 
over time and reduce thrash if any other changes came in to consolidate 
overall disruption to edk2.

> - Would the version update require a whole-tree re-uncrustification?
> 
Yes. I just did it. It is relatively minor and impacts expected code areas.

https://github.com/tianocore/edk2/pull/5043/files

I'm happy to send that to the list if desired.

> The reason I'm not just ignoring this topic is that 73.0.8 actually
> produces *better output* than 73.0.3, at least in the one instance I
> encountered. Compare:
> 
>> diff --git a/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c b/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
>> index 434cdca84b23..3a6f75988220 100644
>> --- a/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
>> +++ b/OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c
>> @@ -43,12 +43,12 @@ STATIC EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL
>>   STATIC CONST EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  mMmio64Configuration = {
>>     ACPI_ADDRESS_SPACE_DESCRIPTOR,                   // Desc
>>     (UINT16)(                                        // Len
>> -                                                   sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
>> -                                                   OFFSET_OF (
>> -                                                     EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
>> -                                                     ResType
>> -                                                     )
>> -                                                   ),
>> +    sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
>> +    OFFSET_OF (
>> +      EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
>> +      ResType
>> +      )
>> +    ),
>>     ACPI_ADDRESS_SPACE_TYPE_MEM,                     // ResType
>>     0,                                               // GenFlag
>>     0,                                               // SpecificFlag
>> @@ -77,12 +77,12 @@ STATIC CONST EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  mMmio64Configuration = {
>>   STATIC CONST EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  mOptionRomConfiguration =   {
>>     ACPI_ADDRESS_SPACE_DESCRIPTOR,                   // Desc
>>     (UINT16)(                                        // Len
>> -                                                   sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
>> -                                                   OFFSET_OF (
>> -                                                     EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
>> -                                                     ResType
>> -                                                     )
>> -                                                   ),
>> +    sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) -
>> +    OFFSET_OF (
>> +      EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR,
>> +      ResType
>> +      )
>> +    ),
>>     ACPI_ADDRESS_SPACE_TYPE_MEM,                     // ResType
>>     0,                                               // GenFlag
>>     0,                                               // Disable option roms SpecificFlag
> 
> Note that 73.0.3 indents the subexpression to the "//"  comment on the
> previous line, while 73.0.8 ignores the comment -- which I think is
> justified here.
> 
> I believe this improvement may come from uncrustify commit 239c4fad745b
> ("Prevent endless indentation scenario in struct assignment",
> 2022-07-29). I think it's worth having in edk2.
> 
> CC: stewards, Pedro (commit 6ded9f50c3aa), Marcin (traditionally a big
> fan of uncrustify :))
> 
> Thanks
> Laszlo


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

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

[Rebecca Cran]

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?

https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html#avoiding-ruining-git-blame


-- 

Rebecca Cran



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

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

[Pedro Falcato]

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 :/

-- 
Pedro


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

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

[Laszlo Ersek]

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 :/

Seeing the cumulative diff in that PR, do you have specific
counter-arguments?

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.

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111201): https://edk2.groups.io/g/devel/message/111201
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Pedro Falcato]

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

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

>
> 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...).
Getting everyone on the same version would already be hard even if the
software was easy to install.

-- 
Pedro


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

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

[Michael Kubacki]

On 11/16/2023 3:29 AM, 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
> 
> and switching back and forth because 'magic indentation tool' says so
> just seems silly to me.
> 
This is the first time Uncrustify has been updated in edk2 since Dec 7, 
2021.

https://github.com/tianocore/edk2/commits/master/.pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml

Its configuration has also not changed during that time. For this 
update, as Laszlo said, it is a trivial update to a few files.

Can you elaborate on "switching back and forth"?

>>
>> 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...).
> Getting everyone on the same version would already be hard even if the
> software was easy to install.
> 
As I mentioned before, stuart makes installation simple. Simple meaning 
it connects to the NuGet feed, pulls the NuGet package, and 
automatically uses the appropriate build for your host OS when running 
other commands.

The installation instructions are here: 
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-Formatting#installing-uncrustify

If you don't want to run stuart, you just need to click the link in 
manual instructions and unzip to get the executable. This should only 
take a couple minutes as shown here.

https://gist.githubusercontent.com/makubacki/ed07d7fab53b1f68b28742126dd76b55/raw/11310b59f867e217f06fbc11339f4e18f2247fe5/HowToDownloadUncrustifyLinux.gif

---

I'm not a full time tools developer or anything so I don't have a lot of 
time to spend on this but I truly do want to reduce pain points if there 
are small tweaks to the process that can be made.


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

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

[Pedro Falcato]

On Thu, Nov 16, 2023 at 5:36 PM Michael Kubacki
<mikuback@linux.microsoft.com> wrote:
> This is the first time Uncrustify has been updated in edk2 since Dec 7,
> 2021.
>
> https://github.com/tianocore/edk2/commits/master/.pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml
>
> Its configuration has also not changed during that time. For this
> update, as Laszlo said, it is a trivial update to a few files.
>
> Can you elaborate on "switching back and forth"?

I would hope that once everything is straightened out and the fork is
no longer needed, uncrustify will have a much higher churn of versions
coming out.
And expecting everyone to have the same version that some specific
commit in EDK2 uses for uncrustify is simply unrealistic. And
formatters changing the way they format a certain piece of code is not
super rare.

So, to pass CI, you'll have to make sure that:

1) All code is formatted by the same uncrustify version - this
requires some churn from time to time to gratuitously change code that
was at least deemed readable before
2) So patches go through without a hitch, everyone needs to be on the
same version - this requires a back and forth between the maintainer
and contributor ("Hey, CI's failing, have you formatted it using
uncrustify?" "Yes I have" "What version do you have? It might not be
upstream's!"). In the case of something like edk2-platforms, where
there's not even CI, this will result in silent "misformatting" and
people's formatters may silently re-format random parts of the code as
they work on it (and this one IS a personal pain point, because all my
platforms packages are formatted using uncrustify, as I cannot write
NT-style code without its help).

and both of these points are way more painful than just installing
uncrustify using my package manager and whatever comes out of it is at
least deemed "okay".

> As I mentioned before, stuart makes installation simple. Simple meaning
> it connects to the NuGet feed, pulls the NuGet package, and
> automatically uses the appropriate build for your host OS when running
> other commands.
>
> The installation instructions are here:
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-Formatting#installing-uncrustify
>
> If you don't want to run stuart, you just need to click the link in
> manual instructions and unzip to get the executable. This should only
> take a couple minutes as shown here.
>
> https://gist.githubusercontent.com/makubacki/ed07d7fab53b1f68b28742126dd76b55/raw/11310b59f867e217f06fbc11339f4e18f2247fe5/HowToDownloadUncrustifyLinux.gif
>
> ---
>
> I'm not a full time tools developer or anything so I don't have a lot of
> time to spend on this but I truly do want to reduce pain points if there
> are small tweaks to the process that can be made.

The smallest improvement I can think of is just to provide
Linux-friendly packaging. A .deb or a .rpm (heck, even a tarball)
should work wonders when it comes to installing this on Linux.

And, to be clear, I appreciate the effort you folks are making to get
some sort of tooling into tianocore. The status quo now is
unquestionably better than before. I early adopted this back in 2021
when it wasn't even upstreamed into EDK2, and it's great at hammering
whatever comes out of my fingers into NT-style code :)

-- 
Pedro


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

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

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Pedro Falcato]

On Fri, Nov 17, 2023 at 9:08 AM Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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

FWIW, this is a point so annoying that normal formatting tools don't
work. It makes you need a fork of an obscure formatter (I know 1
project that uses uncrustify - EDK2...), instead of something more
conventional such as clang-format.
And, FWIW, I've seen a couple of approximations of the NT kernel style
for clang-format.

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

I may be too naive, but with a *well defined* coding style and a
formatter to *help*, I don't see how things would end up poorly.
I think a formatter's job should be to *aid*, to not enforce. These
things are fallible. For instance, clang-format eyesore out of one of
my personal projects:

#define ONES ((size_t) -1 / UCHAR_MAX)
#define HIGHS (ONES * (UCHAR_MAX / 2 + 1))
#define HASZERO(x) (((x) -ONES) & ~(x) &HIGHS)

(may be hard to see in email, but note how clang-format has no idea
how any macro works, so (x) - ONES becomes (x) -ONES and <expr> &
HIGHS becomes <expr> &HIGHS...)

For instance, I think Linux manages this quite nicely. They provide a
.clang-format you can use, and the formatting generally ends up fine.
But you're not expected to re-format every couple of changes, not
everyone is at the same formatter version (if at all!) and "human
thinks looks fine" has higher priority over "*formatter* thinks it
looks fine".

-- 
Pedro


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

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

[Michael Kubacki]

On 11/13/2023 3:37 PM, Rebecca Cran 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?
> 
> https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html#avoiding-ruining-git-blame
> 
> 
Yes. This was discussed this in the Tianocore Tools & CI meeting. I will 
submit the series to the mailing list as this only impacts 5 files in 2 
packages.

I will follow up with a commmit to propose adding that file with all 
formatting changes including the original Uncrustify changes.


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