[edk2-devel] Progress on getting Uncrustify working for EDK2?
Michael D Kinney
michael.d.kinney at intel.com
Tue Nov 9 16:33:31 UTC 2021
Hi Marvin,
> -----Original Message-----
> From: Marvin Häuser <mhaeuser at posteo.de>
> Sent: Tuesday, November 9, 2021 8:13 AM
> To: Kinney, Michael D <michael.d.kinney at intel.com>; devel at edk2.groups.io
> Cc: Andrew Fish <afish at apple.com>; Michael Kubacki <Michael.Kubacki at microsoft.com>; Leif Lindholm <leif at nuviainc.com>;
> mikuback at linux.microsoft.com; rebecca at nuviainc.com; Bret Barkelew <Bret.Barkelew at microsoft.com>
> Subject: Re: [edk2-devel] Progress on getting Uncrustify working for EDK2?
>
> Hey Mike,
>
> On 09.11.21 16:43, Kinney, Michael D wrote:
> > Hi Marvin,
> >
> > Comments below.
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Marvin Häuser
> >> Sent: Monday, November 8, 2021 11:33 PM
> >> To: Kinney, Michael D <michael.d.kinney at intel.com>
> >> Cc: Andrew Fish <afish at apple.com>; devel at edk2.groups.io; Michael Kubacki <Michael.Kubacki at microsoft.com>; Leif Lindholm
> >> <leif at nuviainc.com>; mikuback at linux.microsoft.com; rebecca at nuviainc.com; Bret Barkelew <Bret.Barkelew at microsoft.com>
> >> Subject: Re: [edk2-devel] Progress on getting Uncrustify working for EDK2?
> >>
> >> Hey all,
> >>
> >> Thanks for the effort!
> >>
> >> 1. If virtually everyone will need Uncrustify, why cannot it be built along with BaseTools from a submodule? Especially
> >> with the fork that makes sense, after that it depends on the upstream (it does not look too nice to me).
> > No matter where uncrustify sources are hosted, developers can always choose to build the uncrustify tool locally.
> > Providing release binaries for the tool may be simpler for some customers.
> > Using release binaries from EDK II CI agents will reduce CI execution time.
>
> My point was it'd be nice if it (optionally) "just worked", so
> Uncrustify could be be built as part of the edk2 BaseTools build
> process, or release binaries could be downloaded by some script, or
> whatever really. I guess it could be the same logic as for the CI?
Setting up a development work environment also requires the installation of tools such as
compilers, NASM, IASL, etc. These are not handled as part of BaseTools today.
I agree we want to make setting of the EDK II development environment as easy as possible.
>
> > The goal is to upstream all changes to uncrustify project. However, we need to get the EDK II community to review
> > and accept the style of the source code produced by this fork of uncrustify. We do not want to do this type of
> > change more than once, so getting everyone to agree to a specific format is critical.
> >
> > Once the content is upstreamed, then the EDK II communities maintenance of a fork can end and the EDK II
> > community can work with the uncrustify community for any future changes and developers and CI agents can
> > use the release binaries and/or sources from the uncrustify project.
>
> Thanks!
>
> >> 2. Does this cover CRLF->LF?
> > Are you referring to updating the GitHub repo line endings on the GitHub server side? That is a different
> > topic and will require a different tool and developer process changes.
>
> OK, thanks.
>
> >> 3. Will there be a list of implicit changes from the current code style?
> >
> > I believe the uncrustify tool being discussed here is conformant with the current EDK II C Coding Style Specification.
> > I am not aware of any changes that are required to that spec.
>
> Nice, thanks!
>
> >> 4. I feel like if the other code style changes are not tackled now, they will never be, because who wants to deal with
> >> continuous cosmetic commits. But as long as there is a tool now to enforce the current one, that's not a dealbreaker at
> >> all. :)
> > The additional discussion topics I have seen that are not addressed by source format tools:
> >
> > 1) Allowing local variables to be declared in scope of if/while/case.
> > * Current style only allows locals to be declared at top of function.
> > 2) Allow local variables to be assigned to a value in their declaration.
> > 3) Allow local variables that are struct/array to be assigned a value in their declaration
> > * Depends on support for memcpy()/memset() intrinsics for all supported tool chains.
> > 4) Use of STATIC vs static.
>
> There were more proposals:
>
> 5) Indent relative to the last indentation, not to symbol names, i.e.
>
> Status = Function (
> A
> );
>
> instead of
>
> Status = Function (
> A
> );>
> Uncrustify might support this now, but I have never seen even a single
> IDE that does. It'd be kind of awkward to always write "style-broken"
> code and rely on the external tool to fix it.
The EDK II C Coding Style Spec and the fork of uncrustify are aligned
to support indent of 2 spaces from the start column of the function name.
This would be in the following format from your example:
Status = Function (
A
);
Since many of the APIs in EDK II are called through a services table, PPI,
or Protocol, the indent is 2 spaces from the start of the member function.
Examples:
Status = gBS->OpenProtocol (
ControllerHandle,
&gEfiBlockIoProtocolGuid,
(VOID **)&BlockIo,
This->DriverBindingHandle,
ControllerHandle,
EFI_OPEN_PROTOCOL_BY_DRIVER
);
Status = BlockIo2->ReadBlocksEx (
BlockIo2,
MediaId,
Subtask->Lba,
&Subtask->BlockIo2Token,
(Subtask->Length % Media->BlockSize == 0) ? Subtask->Length : Media->BlockSize,
(Subtask->WorkingBuffer != NULL) ? Subtask->WorkingBuffer : Subtask->Buffer
);
Uncrustify can be installed as a plugin to Visual Studio Code, so the
style the EDK II Community agrees to can be supported by that IDE.
Supporting this indent style is one of the enhancements in the fork. If we want to align
to one of the indent styles supported by a wider array of source editors/IDEs, then that
would require a change to the EDK II Coding Style Specification and approval from the
EDK II community.
>
> 6) Allow static function declarations.
I agree that static functions should be allowed. Please add any comments you have to
the following Bugzilla. If you have ideas on the specific spec updates required, then
please provide a patch against the spec markdown.
https://bugzilla.tianocore.org/show_bug.cgi?id=1766
>
> Best regards,
> Marvin
>
> >
> > Please let me know if I missed any of the additional topics.
> >
> >> Best regards,
> >> Marvin
> >>
> >> 09.11.2021 04:03:06 Kinney, Michael D <michael.d.kinney at intel.com>:
> >>
> >>> Andrew,
> >>>
> >>> I think Michael Kubacki started with a nuget feed because that can be easily used by EDK II CI agents.
> >>>
> >>> However, that does not work as easily for all development environments using Windows, Linux, and MacOS. Creating
> >> releases that can be easily installed by developers is critical for success.
> >>> For MacOS, there is a homebrew recipe. You should just have to update the URL to the uncrustify fork.
> >>>
> >>> https://macappstore.org/uncrustify/
> >>>
> >>> Adding the installation information to the EDK II Getting Started page would be a good place to capture the developer
> >> install.
> >>> Mike
> >>>
> >>> *From:* Andrew Fish <afish at apple.com>
> >>> *Sent:* Monday, November 8, 2021 6:47 PM
> >>> *To:* Kinney, Michael D <michael.d.kinney at intel.com>
> >>> *Cc:* devel at edk2.groups.io; Marvin Häuser <mhaeuser at posteo.de>; Michael Kubacki <Michael.Kubacki at microsoft.com>; Leif
> >> Lindholm <leif at nuviainc.com>; mikuback at linux.microsoft.com; rebecca at nuviainc.com; Bret Barkelew
> >> <Bret.Barkelew at microsoft.com>
> >>> *Subject:* Re: [edk2-devel] Progress on getting Uncrustify working for EDK2?
> >>>
> >>>
> >>>
> >>>
> >>> On Nov 8, 2021, at 5:13 PM, Kinney, Michael D <michael.d.kinney at intel.com> wrote:
> >>>
> >>> HI Andrew,
> >>>
> >>> Great feedback.
> >>>
> >>> What your preferred way to install a tool like this? If we collect that data from the community, we can make sure the
> >> pipeline generates the right type of installers.
> >>>
> >>> I could not figure out how to download an installer from the links.
> >>>
> >>> If I go to http://uncrustify.sourceforge.net I see https://sourceforge.net/projects/uncrustify/files/ and a button to
> >> download binaries. Not ideal that it is only for Windows but at least the workflow was obvious. Looks like I need to
> build
> >> my own version for macOS, not ideal but I can at least figure that out.
> >>> Worst case we can have a Tianocore.org[http://Tianocore.org] page to describe a simple recipe to install the tool.
> With
> >> step by step instructions some one can just type in without thinking too much.
> >>> Thanks,
> >>>
> >>> Andrew Fish
> >>>
> >>>
> >>> Thanks,
> >>>
> >>> Mike
> >>>
> >>> *From:* Andrew Fish <afish at apple.com>
> >>> *Sent:* Monday, November 8, 2021 5:09 PM
> >>> *To:* devel at edk2.groups.io; Kinney, Michael D <michael.d.kinney at intel.com>
> >>> *Cc:* Marvin Häuser <mhaeuser at posteo.de>; Michael Kubacki <Michael.Kubacki at microsoft.com>; Leif Lindholm
> >> <leif at nuviainc.com>; mikuback at linux.microsoft.com; rebecca at nuviainc.com; Bret Barkelew <Bret.Barkelew at microsoft.com>
> >>> *Subject:* Re: [edk2-devel] Progress on getting Uncrustify working for EDK2?
> >>>
> >>> MIke,
> >>>
> >>> I could not figure out how to download uncrustify tool from the provided link. 99% of the people are just going to
> want
> >> to install the tool, not be a developer of the fork. We should have some simple instructions on how to download the
> tool.
> >>> The link points to something git web view looking Azure DevOps page and talks about this Nuget thing I know nothing
> >> about. I ran out of time and had to give up trying to download the tool.
> >>> Thanks,
> >>>
> >>> Andrew Fish
> >>>
> >>>
> >>>
> >>> On Nov 8, 2021, at 4:23 PM, Michael D Kinney <michael.d.kinney at intel.com> wrote:
> >>>
> >>> Hello,
> >>>
> >>> Good information in this thread on code style.
> >>>
> >>> Some of the topics apply to uncrustify and some are out of scope for what uncrustify can fix on its own.
> >>>
> >>> I would like to focus on a date to convert all source code in edk2 repo using the uncrustify tool and to capture the
> >> other code style topics into their own thread andbugzillas.
> >>> I would like to propose a conversion date for uncrustify immediately after the edk2-stable202111 release on 2021-11-
> 26.
> >>>
> >>> I have been working with Michael Kubacki on a build comparison tool that verifies that the build generate the same
> >> obj/lib/dll/efi/fv/fd files before and after the uncrustify changes. We would run and publish the results from this
> tool
> >> before committing the changes.
> >>> We need TianoCore community approval of the following:
> >>>
> >> 1. > Approve format of C source generated by the uncrustify.
> >> 2. > Approve uncrustify changes right after edk2-stable-202111 release.
> >> 1. > Extend code freeze until these changes are committed.
> >> 3. > Require use of uncrustify tool before submitting patch review emails or PRs.
> >> 1. > The required version would be a formally released version from the fork maintained by Michael Kubacki until
> the
> >> changes can be upstreamed.
> >> 2. > https://dev.azure.com/projectmu/Uncrustify
> >> 4. > Add EDK II CI check to verify that all PRs submitted exactly match uncrustified version. Reject PRs that do not
> >> match exactly.
> >> 5. > Implement a git hook available that would automatically run uncristufy before committing changes to a local branch
> of
> >> an edk2 repo.
> >>> Thanks,
> >>>
> >>> Mike
> >>>
> >>> *From:* Andrew Fish <afish at apple.com>
> >>> *Sent:* Thursday, October 7, 2021 2:09 PM
> >>> *To:* Marvin Häuser <mhaeuser at posteo.de>
> >>> *Cc:* edk2-devel-groups-io <devel at edk2.groups.io>; Kinney, Michael D <michael.d.kinney at intel.com>; Leif Lindholm
> >> <leif at nuviainc.com>; mikuback at linux.microsoft.com; rebecca at nuviainc.com; Michael Kubacki
> <Michael.Kubacki at microsoft.com>;
> >> Bret Barkelew <Bret.Barkelew at microsoft.com>
> >>> *Subject:* Re: [edk2-devel] Progress on getting Uncrustify working for EDK2?
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> On Oct 7, 2021, at 1:43 PM, Marvin Häuser <mhaeuser at posteo.de> wrote:
> >>>
> >>> Hey Mike,
> >>> Hey Andrew,
> >>>
> >>> I'll just reply to both mails at once :)
> >>>
> >>> On 07/10/2021 19:36, Andrew Fish wrote:
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> …
> >>>
> >>> Thanks! I'd keep STATIC actually just for the sake of not doing no-op changes that do not really do anything and for
> >> consistency with CONST, but whatever works really.
> >>>
> >>>
> >>>
> >>> …
> >>>
> >>> Yes.
> >>>
> >>>
> >>>
> >>>
> >>> …
> >>>
> >>> This issue is independent of CONST. I’m not sure a coding style tool is smart enough to catch this generically? You
> need
> >> an understanding of C types to know if the local variable assignment is going to trigger a memcpy().
> >>> What I’ve seen in the real world is the firmware compiles with -Os or LTO to fit int he ROM for DEBUG and RELEASE, and
> >> the optimizer optimizes away the call to memcpy. Then if you try to build NOOPT (or over ride the compiler flags on an
> >> individual driver/lib) you fail to link as only the NOOPT build injects the memcpy.
> >>> +1
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> Thus I think the best way to enforce this rule is to compile a project NOOPT. I’m trying to remember are there flags
> to
> >> built to tell it to compile and skip the FD construction? Maybe we should advocate platforms add a NOOPT build target
> that
> >> just compiles the code, but does not create the FD?
> >>> I know there were stability concerns with intrinsics in the past, but memcpy() is in the standard, and the rest
> remained
> >> stable to my knowledge. Maybe it's time to fix the issues at the root? Works for us:
> >>> https://github.com/acidanthera/OpenCorePkg/tree/master/Library/OcCompilerIntrinsicsLib
> >>>
> >>>
> >>>
> >>> Marvin,
> >>>
> >>> Good point. This would make the rule moot. So maybe just removing the requirement would be the easiest long term fix.
> >>>
> >>> Other embedded projects I know of do this too, and as you point out the compilers keep these APIs standard for folks
> the
> >> provide their own runtimes.
> >>> Thanks,
> >>>
> >>> Andrew Fish
> >>>
> >>>
> >>>
> >>>
> >>> Best regards,
> >>> Marvin
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> Thanks,
> >>>
> >>> Andrew Fish
> >>>
> >>>
> >>>
> >>>
> >>> …
> >>>
> >>>
> >>>
> >>
> >>
> >>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83520): https://edk2.groups.io/g/devel/message/83520
Mute This Topic: https://groups.io/mt/84932137/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list