From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web11.14161.1636474369860102154 for ; Tue, 09 Nov 2021 08:12:50 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=V5U2lf5k; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout01.posteo.de (Postfix) with ESMTPS id C1729240026 for ; Tue, 9 Nov 2021 17:12:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1636474366; bh=P6SGqTwMRKb8DcGKDV1HCMpjk7LZfPXHCncf1kzRZ1c=; h=Date:Subject:To:Cc:From:From; b=V5U2lf5knK30SGsNLLJq0y9J2wqPSOYHBDwlkRI3u/d+Lch+Tdaj58LZgqjy2EoRl +36U3PCg4MmeeeJ5//gtl1wC7ddqcCSnzbzGpHtJDvi4pktDUHlpE9AR0NQ/l8tc+i DFghliVDkHgUT+S/ZBF2axRmIfGO1I9Q4nyYWNuJslT0KErk7pNnhj1JoczbrPjpEr tGYDfPqeIt4pptALHBxlxCNWk+P23u+vjS3827HCA9BtOmKPRIHwoc/6wIo7bO5noA UIAHIsWOriBjegMlfXPybkVTRpQidr3kEHM+5y2xiCvIIsqa1RMzOtRQ8w+cx77uCn FMPXPRLGJqFzw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4HpY0q2FPNz6tmJ; Tue, 9 Nov 2021 17:12:43 +0100 (CET) Message-ID: <7e579772-2a21-aaf5-26cc-faed0a1950e1@posteo.de> Date: Tue, 9 Nov 2021 16:12:42 +0000 MIME-Version: 1.0 Subject: Re: [edk2-devel] Progress on getting Uncrustify working for EDK2? To: "Kinney, Michael D" , "devel@edk2.groups.io" Cc: Andrew Fish , Michael Kubacki , Leif Lindholm , "mikuback@linux.microsoft.com" , "rebecca@nuviainc.com" , Bret Barkelew References: <2679bfa3-b4ec-d8e9-7e56-54ebe42d9001@posteo.de> <9fe0f984-db9d-9aec-0b44-5d30791a2855@linux.microsoft.com> <20211007104813.wa4rmfsqgcpvnzwt@leviathan> <07d5c8bc-40b2-4e99-3b3d-4c8ac4e14220@posteo.de> <438B4D66-2CFB-45E3-AF75-42342F0B1E67@apple.com> <07F39C4E-DA8E-4650-A48B-66DA2E08314B@apple.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= In-Reply-To: Content-Language: en-GB Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Hey Mike, On 09.11.21 16:43, Kinney, Michael D wrote: > Hi Marvin, > > Comments below. > > Mike > >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Marvin H= =C3=A4user >> Sent: Monday, November 8, 2021 11:33 PM >> To: Kinney, Michael D >> Cc: Andrew Fish ; devel@edk2.groups.io; Michael Kubacki= ; Leif Lindholm >> ; mikuback@linux.microsoft.com; rebecca@nuviainc.com;= Bret Barkelew >> Subject: Re: [edk2-devel] Progress on getting Uncrustify working for EDK= 2? >> >> Hey all, >> >> Thanks for the effort! >> >> 1. If virtually everyone will need Uncrustify, why cannot it be built al= ong with BaseTools from a submodule? Especially >> with the fork that makes sense, after that it depends on the upstream (i= t does not look too nice to me). > No matter where uncrustify sources are hosted, developers can always choo= se 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 tim= e. My point was it'd be nice if it (optionally) "just worked", so=20 Uncrustify could be be built as part of the edk2 BaseTools build=20 process, or release binaries could be downloaded by some script, or=20 whatever really. I guess it could be the same logic as for the CI? > The goal is to upstream all changes to uncrustify project. However, we n= eed to get the EDK II community to review > and accept the style of the source code produced by this fork of uncrusti= fy. 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 o= f a fork can end and the EDK II > community can work with the uncrustify community for any future changes a= nd 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 enfor= ce the current one, that's not a dealbreaker at >> all. :) > The additional discussion topics I have seen that are not addressed by so= urce 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 support= ed 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. =C2=A0 Status =3D Function ( =C2=A0=C2=A0=C2=A0 A =C2=A0=C2=A0=C2=A0 ); instead of =C2=A0 Status =3D Function ( =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 A =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ); Uncrustify might support this now, but I have never seen even a single=20 IDE that does. It'd be kind of awkward to always write "style-broken"=20 code and rely on the external tool to fix it. 6) Allow static function declarations. 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 : >> >>> Andrew, >>> >>> I think Michael Kubacki started with a nuget feed because that can be e= asily used by EDK II CI agents. >>> >>> However, that does not work as easily for all development environments = using Windows, Linux, and MacOS.=C2=A0 Creating >> releases that can be easily installed by developers is critical for succ= ess. >>> For MacOS, there is a homebrew recipe.=C2=A0 You should just have to up= date 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 >>> *Sent:* Monday, November 8, 2021 6:47 PM >>> *To:* Kinney, Michael D >>> *Cc:* devel@edk2.groups.io; Marvin H=C3=A4user ; Mi= chael Kubacki ; Leif >> Lindholm ; mikuback@linux.microsoft.com; rebecca@nuvi= ainc.com; Bret Barkelew >> >>> *Subject:* Re: [edk2-devel] Progress on getting Uncrustify working for = EDK2? >>> >>> >>> >>> >>> On Nov 8, 2021, at 5:13 PM, Kinney, Michael D wrote: >>> >>> HI Andrew, >>> >>> Great feedback. >>> >>> What your preferred way to install a tool like this?=C2=A0=C2=A0If we c= ollect 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=C2=A0http://uncrustify.sourceforge.net=C2=A0I see=C2=A0https= ://sourceforge.net/projects/uncrustify/files/=C2=A0and a button to >> download binaries. Not ideal that it is only for Windows but at least th= e 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]=C2=A0page = 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:*=C2=A0Andrew Fish >>> *Sent:*=C2=A0Monday, November 8, 2021 5:09 PM >>> *To:*=C2=A0devel@edk2.groups.io; Kinney, Michael D >>> *Cc:*=C2=A0Marvin H=C3=A4user ; Michael Kubacki ; Leif Lindholm >> ; mikuback@linux.microsoft.com; rebecca@nuviainc.com;= Bret Barkelew >>> *Subject:*=C2=A0Re: [edk2-devel] Progress on getting Uncrustify working= for EDK2? >>> >>> MIke, >>> >>> I could not figure out how to download uncrustify tool from the provide= d 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 wrote: >>> >>> Hello, >>> >>> Good information in this thread on code style. >>> >>> Some of the topics apply to=C2=A0uncrustify=C2=A0and some are out of sc= ope for what=C2=A0uncrustify=C2=A0can fix on its own. >>> >>> I would like to focus on a date to convert all source code in edk2 repo= using the=C2=A0uncrustify=C2=A0tool and to capture the >> other code style topics into their own thread andbugzillas. >>> I would like to propose a conversion date for=C2=A0uncrustify=C2=A0imme= diately after the edk2-stable202111 release on 2021-11-26. >>> >>> I have been working with Michael Kubacki on a build comparison tool tha= t verifies that the build generate the same >> obj/lib/dll/efi/fv/fd=C2=A0files before and after the=C2=A0uncrustify=C2= =A0changes.=C2=A0=C2=A0We would run and publish the results from this tool >> before committing the changes. >>> We need=C2=A0TianoCore=C2=A0community approval of the following: >>> >> 1. > Approve format of C source generated by the=C2=A0uncrustify. >> 2. > Approve=C2=A0uncrustify=C2=A0changes right after edk2-stable-202111= release. >> 1. > Extend code freeze until these changes are committed. >> 3. > Require use of=C2=A0uncrustify=C2=A0tool before submitting patch re= view emails or PRs. >> 1. > The required version would be a formally released version=C2=A0= =C2=A0from the fork maintained by Michael Kubacki until the >> changes can be=C2=A0upstreamed. >> 2. > https://dev.azure.com/projectmu/Uncrustify >> 4. > Add EDK II CI check to verify that all PRs submitted exactly match= =C2=A0uncrustified=C2=A0version.=C2=A0=C2=A0Reject PRs that do not >> match exactly. >> 5. > Implement a git hook available that would automatically run=C2=A0un= cristufy=C2=A0before committing changes to a local branch of >> an edk2 repo. >>> Thanks, >>> >>> Mike >>> >>> *From:*=C2=A0Andrew Fish >>> *Sent:*=C2=A0Thursday, October 7, 2021 2:09 PM >>> *To:*=C2=A0Marvin H=C3=A4user >>> *Cc:*=C2=A0edk2-devel-groups-io ; Kinney, Michael= D ; Leif Lindholm >> ;=C2=A0mikuback@linux.microsoft.com;=C2=A0rebecca@nuv= iainc.com; Michael Kubacki ; >> Bret Barkelew >>> *Subject:*=C2=A0Re: [edk2-devel] Progress on getting Uncrustify working= for EDK2? >>> >>> >>> >>> >>> >>> >>> On Oct 7, 2021, at 1:43 PM, Marvin H=C3=A4user wro= te: >>> >>> Hey Mike, >>> Hey Andrew, >>> >>> I'll just reply to both mails at once :) >>> >>> On 07/10/2021 19:36, Andrew Fish wrote: >>> >>> >>> >>> >>> >>> >>> >>> >>> =E2=80=A6 >>> >>> Thanks! I'd keep STATIC actually just for the sake of not doing no-op c= hanges that do not really do anything and for >> consistency with CONST, but whatever works really. >>> >>> >>> >>> =E2=80=A6 >>> >>> Yes. >>> >>> >>> >>> >>> =E2=80=A6 >>> >>> This issue is independent of CONST. I=E2=80=99m 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=E2=80=99ve 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 buil= d 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=E2=80=99m trying to remember are there flags to >> built to tell it to compile and skip the FD construction? Maybe we shoul= d 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 m= emcpy() is in the standard, and the rest remained >> stable to my knowledge. Maybe it's time to fix the issues at the root? W= orks for us: >>> https://github.com/acidanthera/OpenCorePkg/tree/master/Library/OcCompil= erIntrinsicsLib >>> >>> >>> >>> Marvin, >>> >>> Good point. This would make the rule moot. So maybe just removing the r= equirement 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 >>> >>> >>> >>> >>> =E2=80=A6 >>> >>> >>> >> >>=20 >>