> On Nov 8, 2021, at 5:13 PM, Kinney, Michael D <michael.d.kin...@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 <http://uncrustify.sourceforge.net/> I see https://sourceforge.net/projects/uncrustify/files/ <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 <af...@apple.com> > Sent: Monday, November 8, 2021 5:09 PM > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com> > Cc: Marvin Häuser <mhaeu...@posteo.de>; Michael Kubacki > <michael.kuba...@microsoft.com>; Leif Lindholm <l...@nuviainc.com>; > mikub...@linux.microsoft.com; rebe...@nuviainc.com; Bret Barkelew > <bret.barke...@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.kin...@intel.com > <mailto:michael.d.kin...@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: > > Approve format of C source generated by the uncrustify. > Approve uncrustify changes right after edk2-stable-202111 release. > Extend code freeze until these changes are committed. > Require use of uncrustify tool before submitting patch review emails or PRs. > The required version would be a formally released version from the fork > maintained by Michael Kubacki until the changes can be upstreamed. > https://dev.azure.com/projectmu/Uncrustify > <https://dev.azure.com/projectmu/Uncrustify> > Add EDK II CI check to verify that all PRs submitted exactly match > uncrustified version. Reject PRs that do not match exactly. > 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 <af...@apple.com <mailto:af...@apple.com>> > Sent: Thursday, October 7, 2021 2:09 PM > To: Marvin Häuser <mhaeu...@posteo.de <mailto:mhaeu...@posteo.de>> > Cc: edk2-devel-groups-io <devel@edk2.groups.io > <mailto:devel@edk2.groups.io>>; Kinney, Michael D <michael.d.kin...@intel.com > <mailto:michael.d.kin...@intel.com>>; Leif Lindholm <l...@nuviainc.com > <mailto:l...@nuviainc.com>>; mikub...@linux.microsoft.com > <mailto:mikub...@linux.microsoft.com>; rebe...@nuviainc.com > <mailto:rebe...@nuviainc.com>; Michael Kubacki <michael.kuba...@microsoft.com > <mailto:michael.kuba...@microsoft.com>>; Bret Barkelew > <bret.barke...@microsoft.com <mailto:bret.barke...@microsoft.com>> > Subject: Re: [edk2-devel] Progress on getting Uncrustify working for EDK2? > > > > > > On Oct 7, 2021, at 1:43 PM, Marvin Häuser <mhaeu...@posteo.de > <mailto:mhaeu...@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: > > > > > > > On Oct 7, 2021, at 1:19 PM, Michael D Kinney <michael.d.kin...@intel.com > <mailto:michael.d.kin...@intel.com>> wrote: > > Hi Marvin, > > Some comments below. > > Mike > > > > -----Original Message----- > From:devel@edk2.groups.io <mailto:devel@edk2.groups.io><devel@edk2.groups.io > <mailto:devel@edk2.groups.io>> On Behalf Of Marvin Häuser > Sent: Thursday, October 7, 2021 11:31 AM > To: Leif Lindholm <l...@nuviainc.com > <mailto:l...@nuviainc.com>>;devel@edk2.groups.io > <mailto:devel@edk2.groups.io>;mikub...@linux.microsoft.com > <mailto:mikub...@linux.microsoft.com> > Cc:rebe...@nuviainc.com <mailto:rebe...@nuviainc.com>; Michael Kubacki > <michael.kuba...@microsoft.com <mailto:michael.kuba...@microsoft.com>>; Bret > Barkelew <bret.barke...@microsoft.com <mailto:bret.barke...@microsoft.com>>; > Kinney, Michael D <michael.d.kin...@intel.com > <mailto:michael.d.kin...@intel.com>> > Subject: Re: [edk2-devel] Progress on getting Uncrustify working for EDK2? > > Good day, > > +1, but while you're at it, can we have arguments not align to the > function name... > > Status = Test ( > a > ); > > ... but to the next natural indentation level? > > Status = Test ( > a > ); > > Basically no IDE I have seen supports EDK II's style, and I wouldn't be > keen on writing known-broken style to then rely on Uncrustify to fix it. > > I also have heard some controversy regarding casts off-list, where some > prefer no spaces after casts to stress the evaluation order, and some > prefer spaces to have clearer visuals (as a cast *ideally* would be > something rare that requires good justification). Just throwing that out > there. > > > For things unrelated to autoformat (so semi-offtopic) but still relevant > to the coding spec: > > 1. Allow STATIC functions (if the debugging concerns are still relevant, > there could be another level of indirection, like RELEASE_STATIC)? > > Debugging concerns are no longer relevant. The suggestion in the BZ below > is to remove the STATIC macro and allow EDK II sources to add 'static' > to any functions it makes sense to use on. > > https://bugzilla.tianocore.org/show_bug.cgi?id=1766 > <https://bugzilla.tianocore.org/show_bug.cgi?id=1766> > > 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. > > > > > > > > 2. Allow variable assignments on definition (basically non-static CONST > variables are banned...)? > > Are referring to use of pre-initialized CONST variables declared within > a function? I think Bret brought this topic up when implementing some > unit tests and the suggestion to pass ECCC was to promote them to > pre-initialized CONST global variables. > > Yes. > > > > > The challenges we have seen in the past with pre-initialized variables within > a function is that they can cause compilers to inject use of memcpy() calls, > especially if the variable being initialized on the stack is a structure. > These cause build breaks today. > > 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 > > <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 > > > > > > > > 3. Allow variable declarations at any scope (I had some nasty shadowing > bugs before, probably prohibit shadowing with warnings)? > > By shadowing do you mean the declaration of the same variable name in > multiple scoped within the same function? > > > > > 4. Require that exactly all function declarations and all function > definitions with no prior declaration must be documented (first > direction is enforcing docs, second is prohibiting doc duplication, I've > seen them go out-of-sync plenty of times)? > > I agree that this can reduce duplication and sync issues. The uncrustify > tool being discussed here could not help clean this up or enforce this > type of rule. It is a good topic, but may need to be split out into its > own thread. > > > > > The latter bunch would not require any autoformat rules or reformatation > of existing code, but would be target only new submissions in my > opinion. Thoughts? > > > Thanks for your efforts! > > Best regards, > Marvin > > > Am 07.10.2021 um 12:48 schrieb Leif Lindholm: > > > Hi Michael, > > Apologies, I've owed you a response (promised off-list) for a while > now. > > First, let me say I hugely appreciate this effort. Apart from aligning > the codebase(s), this will reduce manual reviewing effort > substantially, as well as cutting down on number of rework cycles for > developers. > > Looking at the changes to (well, the comments in) uncrustify, this > seems to be constrained to: > - Newline after '(' for multi-line function calls. > - Dealing with "(("/"))" for DEBUG macros. > - Function pointer typedefs: > - typedef\nEFIAPI > - closing parentheses indentation > > I don't think I've made any secret over the years that I am not a > massive fan of the EDK2 coding style in general. So I think for any > of its quirks that are substantial enough that they require not just > custom configuration but actual new function added to existing code > conformance tools, this would be an excellent point to sanitise the > coding style instead. > > Taking these in order: > > Newline after '(' > ----------------- > I think we already reached a level of flexibility around this, where > we don't actually enforce this (or single argument per > line). Personally, I'd be happy to update the coding style as > required instead. > > DEBUG macro parentheses > ----------------------- > How does uncrustify treat DEBUG macros without this modification? > Do we start getting everything turned into multi-level indented > multi-line statements without this change? > > Function pointer typedefs: > -------------------------- > I don't see that function pointer typedefs need to rigidly follow the > same pattern as the declaration of functions implementing them. Could > we update the coding style (if needed) instead? > > Best Regards, > > Leif > > On Mon, Aug 16, 2021 at 16:00:38 -0400, Michael Kubacki wrote: > > > The edk2 branch was created here: > https://github.com/makubacki/edk2/tree/uncrustify_poc_2 > <https://github.com/makubacki/edk2/tree/uncrustify_poc_2> > > We have a Project Mu fork with custom changes to the Uncrustify tool to help > comply with EDK II formatting here: > https://dev.azure.com/projectmu/_git/Uncrustify > <https://dev.azure.com/projectmu/_git/Uncrustify> > > The latest information about the status and how to experiment with the > configuration file and the tool are in that fork: > https://dev.azure.com/projectmu/Uncrustify/_wiki/wikis/Uncrustify.wiki/1/Project-Mu-(EDK-II)-Fork-Readme > > <https://dev.azure.com/projectmu/Uncrustify/_wiki/wikis/Uncrustify.wiki/1/Project-Mu-(EDK-II)-Fork-Readme> > > > > That said, I have also finished a CI plugin to run Uncrustify that should be > ready soon to initially deploy in Project Mu. Before doing so, I am trying > to settle on an initial configuration file that less strictly but more > reliably formats the code than in the examples in those branches. For > example, remove heuristics that when run against the same set of code > multiple times can produce different results. An example would be a rule > that reformats code because it exceeds a specified column width on one run > but on the next run that reformatted code triggers a different rule to > further align the code and so on. At least initially, some rules might be > tweaked in a more conservative approach that can be tightened in the future. > Once this configuration file is ready, we will baseline Project Mu code as > an example and turn on the plugin. The CI plugin runs Uncrustify against > modified files and if there's any changes, indicating a formatting > deviation, the diff chunks are saved in a log so they can be viewed as a > build artifact. > > I am making progress on the updated config file and I should be able to post > a "uncrustify_poc_3" branch soon with the results. > > Regarding indentation, Marvin is right that Uncrustify cannot support edk2 > indentation style out-of-box. Some changes are made in that fork to handle > the formatting. At this point, it can handle the indentation in the cases > I've seen. Uncrustify does potentially give us the ability to massively > deploy changes across the codebase in case a decision were made to change > the style. > > Thanks, > Michael > > On 8/16/2021 3:39 PM, Marvin Häuser wrote: > > > Hey Rebecca, > > I think even Uncrustify has issues with the EDK II indentation style. > You might want to check the UEFI Talkbox Discord server, I had a brief > chat with Michael about it there. I don't think realistically any tool > supports EDK II's indentation style however, so I'd propose it is > changed. This could be for new submissions only, or actually the entire > codebase could be reformatted at once with a good tool setup. While this > screws with git blame, the (to my understanding) decided on CRLF -> LF > change does that anyway, so at least two evils could be dealt with in > one go really. > > Best regards, > Marvin > > On 16/08/2021 21:34, Rebecca Cran wrote: > > > > cc devel@ . > > On 8/16/21 1:33 PM, Rebecca Cran wrote: > > > > I noticed a message on Twitter about an idea of using Uncrustify > for EDK2 instead of the ECC tool, and came across https://www.mail- > <https://www.mail-/> > archive.com/search?l=devel@edk2.groups.io&q=subject:%22Re%5C%3A+%5C%5Bedk2%5C- > > <mailto:archive.com/search?l=devel@edk2.groups.io&q=subject:%22Re%5C%3A+%5C%5Bedk2%5C-> > devel%5C%5D+TianoCore+Community+Meeting+Minutes+%5C-+2%5C%2F4%22&o=newest&f=1 > > > . > > I was wondering if there's been any progress on it that I could > check out? > > > Michael Kubacki: in that message, you said: > > "I'm planning to put up a branch that we can use as a reference > for a conversation around uncrustify in the next couple of > weeks." > > > Did you end up creating that branch, and if so could you provide > a link to it please? > > > -- > Rebecca Cran > > > > > > > > > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#83479): https://edk2.groups.io/g/devel/message/83479 Mute This Topic: https://groups.io/mt/84932137/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
