From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web09.5561.1638491490215163629 for ; Thu, 02 Dec 2021 16:31:30 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=RPbsO2yf; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [10.0.0.19] (c-73-27-179-174.hsd1.fl.comcast.net [73.27.179.174]) by linux.microsoft.com (Postfix) with ESMTPSA id C0CB920E67A0; Thu, 2 Dec 2021 16:31:28 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com C0CB920E67A0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1638491489; bh=fSkvWysLai253E+gTlUXLcPcqBk+b21FR+mSDfvgTjo=; h=Date:Subject:To:References:From:In-Reply-To:From; b=RPbsO2yfqtertOnO53+Z79AhgDstMNjybnHSse6OCA7MvobOVjh4S2rzvShvZl0Kq IVr4tFM9iqGOmoZK8UEVyuh0I0T4EQNl1C9s0gITlCc2ok+Cy/DcmtFfBFEA9UTXBy 9tniQIDKLmxcqdlVSke1nkzJ0TXmUNXVc+3IWKps= Message-ID: <0c319cd4-3726-c6bb-c31e-6693b6a18601@linux.microsoft.com> Date: Thu, 2 Dec 2021 19:31:28 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.3.2 Subject: Re: [edk2-devel] Uncrustify Conversion Detailed Plan and Extended Hard Freeze Update #4 To: "Kinney, Michael D" , "devel@edk2.groups.io" , "maciej.rabeda@linux.intel.com" , Michael Kubacki , "Andrew Fish (afish@apple.com)" , Leif Lindholm References: <6f929e61-6b8f-9ac5-5184-a9949c9fae75@linux.intel.com> <438f9138-641b-4973-bca3-cad990526cee@linux.microsoft.com> From: "Michael Kubacki" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Hi Mike, Thank you for the detailed analysis and recommendations. I agree with the recommendations and I will try to have an Uncrustify=20 tool update by tomorrow for (4). That will require an update in uncrustify_ext_dep.yaml to pull in the=20 new version. I'm assuming I should also update uncrustify.cfg to set=20 align_assign_thresh =3D 0 in that new patch series? Regards, Michael On 12/2/2021 7:18 PM, Kinney, Michael D wrote: > Hi Michael, >=20 > CORRECTION: set align_assign_threshold to 0. >=20 > Reponses inline below. >=20 > I would like to summarize the 4 issues raised in the past day along with = the recommendations. >=20 > 1) Exclusion feature for UncrustifyCheck. There are 2 directories with 8= files total that > maintainers have noted they would like to see not go through uncrusti= fy formatting. Today > the only content that is skipped is BaseTools and submodules. >=20 > Adding a general purpose exclusion feature would then require all dev= elopers to make > sure their method of using uncrustify also excludes those same areas.= This requires > extra steps for all developers and maintainers. >=20 > If we do not add the exclusion feature, then the 8 files will require= an extra step > to sync with the original source of those files. The rate of changes= of these 8 files > is very low today. >=20 > RECOMMENDATION: Do not add exclusion feature at this time. Revisit i= f the extra work > to maintain the files that would be candidates for exclusions increas= es significantly. >=20 > 2) Alignment of assignments. The threshold of 4 characters appears to be= too low and causes > source files that are already aligned to become unaligned. >=20 > RECOMMENDATION: Change threshold to the default value of 0 which mean= s no limit. >=20 > align_assign_thresh=3D 0 >=20 > 3) Alignment of parameters in function declaration not correct. The root= cause of this > is the use of the OPTIONAL keyword. If the OPTIONAL keyword is remov= ed, then the > alignment is correct. The alignment is also correct if the OPTIONAL = keyword appears > before the ','. If the OPTIONAL keyword appears after the ',', then = the format is > not correct. The OPTIONAL keyword indicates that the parameter in th= e function is > not required and may be passed in as NULL or 0 or some other default = value defined by > the API. It makes more sense for this OPTIONAL keyword that follows = the parameter > names to appear before the ',' so it is scoped to the parameter on th= at line. If it > appears after the ',', then C parsers thinks it is a prefix (IN, OUT,= CONST, volatile, > static) for the next parameter in the function. >=20 > RECOMMENDATION: Update patch series with a global search and replace = so OPTIONAL > keyword always appears before the ',' on the same line. >=20 > RegEx search string: ',( *)OPTIONAL( *)' > RegEx replace string: ' $1OPTIONAL,$2' >=20 > 4) Format issues with complex blocks in DEBUG_CODE(), or between DEBUG_CO= DE_BEGIN() and > DEBUG_CODE_END(). Uncrustify treats these as macros and is not aware= that the > parameter passed into the macro call is a block of C code that needs = to be formatted. > Complex blocks with if/while/for/case statements are impacted the mos= t. >=20 > RECOMMENDATION: Update the uncrustify with an edk2 specific extension= to treat these > macros as a block of code as if they were surrounded by an extra set = of braces {}. >=20 >=20 > I have posted a branch for testing purposes that implements (2) and (3). >=20 > Branch: https://github.com/mdkinney/edk2/tree/TestOnly_Bug_3737_3739_Appl= yUncrustifyChanges_V6_OPTIONAL_Keyword_Fix > PR: https://github.com/tianocore/edk2/pull/2233 > Status: PASS > CompareBuild: https://github.com/mdkinney/edk2/actions/runs/1532855914 > Status: PASS >=20 > You can see what changed by fetching and comparing the following 2 branch= es: >=20 > https://github.com/mdkinney/edk2/tree/Bug_3737_3739_ApplyUncrustifyC= hanges_V5 > https://github.com/mdkinney/edk2/tree/TestOnly_Bug_3737_3739_ApplyUn= crustifyChanges_V6_OPTIONAL_Keyword_Fix >=20 > Please provide feedback on the RECOMMENDATIONS above. I will go ahead an= d prepare of V6 version of > the patch series now that that test results are all PASS. >=20 > Best regards, >=20 > Mike >=20 >=20 >> -----Original Message----- >> From: Kinney, Michael D >> Sent: Thursday, December 2, 2021 4:15 PM >> To: Michael Kubacki ; devel@edk2.groups.io= ; maciej.rabeda@linux.intel.com; Michael Kubacki >> ; Andrew Fish (afish@apple.com) ; Leif Lindholm ; >> Kinney, Michael D >> Subject: RE: [edk2-devel] Uncrustify Conversion Detailed Plan and Extend= ed Hard Freeze Update #4 >> >> Hi Michael, >> >> Reponses inline below. >> >> I would like to summarize the 4 issues raised in the past day along with= the recommendations. >> >> 1) Exclusion feature for UncrustifyCheck. There are 2 directories with = 8 files total that >> maintainers have noted they would like to see not go through uncrust= ify formatting. Today >> the only content that is skipped is BaseTools and submodules. >> >> Adding a general purpose exclusion feature would then require all de= velopers to make >> sure their method of using uncrustify also excludes those same areas= . This requires >> extra steps for all developers and maintainers. >> >> If we do not add the exclusion feature, then the 8 files will requir= e an extra step >> to sync with the original source of those files. The rate of change= s of these 8 files >> is very low today. >> >> RECOMMENDATION: Do not add exclusion feature at this time. Revisit = if the extra work >> to maintain the files that would be candidates for exclusions increa= ses significantly. >> >> 2) Alignment of assignments. The threshold of 4 characters appears to b= e too low and causes >> source files that are already aligned to become unaligned. >> >> RECOMMENDATION: Change threshold to the default value of 0 which mea= ns no limit. >> >> align_assign_thresh=3D 4 >> >> 3) Alignment of parameters in function declaration not correct. The roo= t cause of this >> is the use of the OPTIONAL keyword. If the OPTIONAL keyword is remo= ved, then the >> alignment is correct. The alignment is also correct if the OPTIONAL= keyword appears >> before the ','. If the OPTIONAL keyword appears after the ',', then= the format is >> not correct. The OPTIONAL keyword indicates that the parameter in t= he function is >> not required and may be passed in as NULL or 0 or some other default= value defined by >> the API. It makes more sense for this OPTIONAL keyword that follows= the parameter >> names to appear before the ',' so it is scoped to the parameter on t= hat line. If it >> appears after the ',', then C parsers thinks it is a prefix (IN, OUT= , CONST, volatile, >> static) for the next parameter in the function. >> >> RECOMMENDATION: Update patch series with a global search and replace= so OPTIONAL >> keyword always appears before the ',' on the same line. >> >> RegEx search string: ',( *)OPTIONAL( *)' >> RegEx replace string: ' $1OPTIONAL,$2' >> >> 4) Format issues with complex blocks in DEBUG_CODE(), or between DEBUG_C= ODE_BEGIN() and >> DEBUG_CODE_END(). Uncrustify treats these as macros and is not awar= e that the >> parameter passed into the macro call is a block of C code that needs= to be formatted. >> Complex blocks with if/while/for/case statements are impacted the mo= st. >> >> RECOMMENDATION: Update the uncrustify with an edk2 specific extensio= n to treat these >> macros as a block of code as if they were surrounded by an extra set= of braces {}. >> >> >> I have posted a branch for testing purposes that implements (2) and (3). >> >> Branch: https://github.com/mdkinney/edk2/tree/TestOnly_Bug_3737_3739_App= lyUncrustifyChanges_V6_OPTIONAL_Keyword_Fix >> PR: https://github.com/tianocore/edk2/pull/2233 >> Status: PASS >> CompareBuild: https://github.com/mdkinney/edk2/actions/runs/1532855914 >> Status: PASS >> >> You can see what changed by fetching and comparing the following 2 branc= hes: >> >> https://github.com/mdkinney/edk2/tree/Bug_3737_3739_ApplyUncrustify= Changes_V5 >> https://github.com/mdkinney/edk2/tree/TestOnly_Bug_3737_3739_ApplyU= ncrustifyChanges_V6_OPTIONAL_Keyword_Fix >> >> Please provide feedback on the RECOMMENDATIONS above. I will go ahead a= nd prepare of V6 version of >> the patch series now that that test results are all PASS. >> >> Best regards, >> >> Mike >> >> >>> -----Original Message----- >>> From: Michael Kubacki >>> Sent: Thursday, December 2, 2021 1:57 PM >>> To: devel@edk2.groups.io; Kinney, Michael D ; maciej.rabeda@linux.intel.com; Michael Kubacki >>> ; Andrew Fish (afish@apple.com) ; Leif Lindholm >>> Subject: Re: [edk2-devel] Uncrustify Conversion Detailed Plan and Exten= ded Hard Freeze Update #4 >>> >>> My reply is inline. >>> >>> Regards, >>> Michael >>> >>> On 12/2/2021 2:45 PM, Michael D Kinney wrote: >>>> Hi Maciej, >>>> >>>> Thanks for the feedback. >>>> >>>> * Example #1.This appears to be caused by the following uncrustify >>>> settings: >>>> >>>> # The threshold for aligning on '=3D' in assignments. >>>> >>>> # Use a negative number for absolute thresholds. >>>> >>>> # >>>> >>>> # 0: No limit (default). >>>> >>>> align_assign_thresh=3D 0# number >>>> >>>> The edk2 setting for this is: >>>> >>>> align_assign_thresh=3D 4 >>>> >>>> This means blocks of assignments that are different than more than 4 >>>> spaces will be considered a new block. >>>> >>>> =E2=80=98HwAddreLen=E2=80=99 and =E2=80=98Xid=E2=80=99 are more than 4= characters in length >>>> different.Same for =E2=80=98Xid=E2=80=99 and =E2=80=98Reserved=E2=80= =99.So >>>> >>>> uncrustify treats these as 3 different assignment blocks. >>>> >>>> If we change to the default value of 0: No limit, this example is >>>> treated as a single block and all =E2=80=98=3D=E2=80=99 are aligned. >>>> >>>> Is there a reason =E2=80=984=E2=80=99 was selected?Is there is any har= m in using the >>>> default of 0? >>>> >>> We can certainly change the threshold. '4' was derived by >>> experimentation. This is an area that is somewhat subjective and every >>> case is difficult to cover well. Please feel free to suggest any change= s. >>> >> >> I recommend we use the default value of 0. That way, any code that choo= se to >> align assignments will still be aligned. If a developer does not like >> short assignments and long assignments in the same code block to use the >> long assignment column, they can always break the block up into multiple >> blocks by adding a carriage return. >> >>> For the benefit of others, this file can be a useful refernce >>> understanding spans, gaps, and thresholds. >>> >>> https://github.com/uncrustify/uncrustify/blob/master/documentation/htdo= cs/configuration.txt >>> >>>> * Example #2: Uncruistfy is confused by the DEBUG_CODE() macro.This= is >>>> not a traditional macro because the >>>> >>>> contents of the macro is a block of C code.I do not know how to convin= ce >>>> uncrustify that the contents of a >>>> >>>> macro like function call to be treated as a code block from an indent >>>> perspective. >>>> >>> I believe this would have to be overridden in the Uncrustify fork since >>> DEBUG_CODE() is being treated as a macro function call and code blocks >>> are not expected there. In addition, some special treatment might be >>> needed for alignment in between DEBUG_CODE_BEGIN()/DEBUG_CODE_END(). >>> >>> I'm happy to look at this. However, regression validation can take a >>> while so I'd like to make sure we need this now. In cases that do not >>> have additional code blocks, it seems to format fairly well. Is this >>> prevalent and impactful enough it must be fixed now? Or, could we >>> revisit it with a follow up patch? >> >> How long does regression testing take? I see about 20 instances of >> DEBUG_CODE() that are producing bad formatting. If the content inside >> DEBUG_CODE() is a single line of a single block of statements without >> any if/while/for/case statements that require further indent, then the >> format looks ok. >> >> DEBUG_CODE_BEGIN() and DEBUG_CODE_END() would look better if the >> contents between are also indented one level. >> >>> >>>> * Example #3/#4: Uncrustify is confused by the OPTIONAL keyword.The >>>> edk2 config declares it as a QUALIFIER >>>> >>>> like IN and OUT.However, OPTIONAL only appears at the end of the line >>>> that declares a parameter to a >>>> >>>> function.There are 3 forms. One with comma after OPTIONAL.One with com= ma >>>> before OPTIONAL, and >>>> >>>> one with no comma if the parameter is the last parameter in the functi= on >>>> declaration. >>>> >>>> TYPE ParamName OPTIONAL, >>>> >>>> TYPEParamName, OPTIONAL >>>> >>>> TYPEParamName OPTIONAL >>>> >>>> OPTIONAL is defined to nothing in edk2 builds.From a uncrustify >>>> perspective, we really want is to be >>>> >>>> ignored or more correctly treated it as a token that is attached to >>>> ParamName and the combination of >>>> >>>> ParamName and OPTIONAL treated as one unit to determine indents. >>>> >>> "TYPE ParamName, OPTIONAL" seems especially problematic and presents an >>> inconsistency with the other formats. Mike, can you please let me know >>> if you have the same observation and your thoughts on a consistent patt= ern? >>> >>>> Thanks, >>>> >>>> Mike >>>> >>>> *From:*devel@edk2.groups.io *On Behalf Of *Maci= ej >>>> Rabeda >>>> *Sent:* Thursday, December 2, 2021 10:27 AM >>>> *To:* devel@edk2.groups.io; Kinney, Michael D >>>> ; Michael Kubacki >>>> ; Andrew Fish (afish@apple.com) >>>> ; Leif Lindholm >>>> *Subject:* Re: [edk2-devel] Uncrustify Conversion Detailed Plan and >>>> Extended Hard Freeze Update #4 >>>> >>>> Hey Mike, >>>> >>>> While most of the changes related to fixing coding style violations, >>>> there are a couple of changes to NetworkPkg in that PR that make the >>>> code less readable. Examples below. >>>> >>>> Example 1: >>>> >>>> >>>> >>>> Example 2: >>>> >>>> >>>> Example 3: >>>> >>>> >>>> Example 4: >>>> >>>> On 30-Nov-21 23:34, Michael D Kinney wrote: >>>> >>>> Hello, >>>> >>>> Thank you for your patience during this extended hard freeze. >>>> >>>> Just one more step to go.There has been a delay in the review of >>>> >>>> the patch series with the uncrustify source changes.PR(6).This >>>> >>>> patch series was not sent out as patch review email because of it= s >>>> >>>> very large size.It only contains source style changes and the >>>> >>>> CompareBuild tool and GitHub action has shown there are no binary >>>> >>>> differences introduced with these source style changes. >>>> >>>> If you are a package maintainer, then please review the following >>>> >>>> branch/PR for your package contents and review the EDK II CI resu= lts >>>> >>>> and BuildCompare results.I do not expect a line by line review >>>> >>>> because we already had time to provide feedback on the source sty= le >>>> >>>> performed by uncrustify.Instead, a Reviewed-by for your package >>>> >>>> indicates that you have reviewed the EDK II CI results and Compar= eBuild >>>> >>>> tool functionality and results and you accept the source style >>>> >>>> changes to your package. >>>> >>>> *https://github.com/mdkinney/edk2/tree/Bug_3737_3739_ApplyUncrust= ifyChanges_V5 >>> >>>> >>>> *https://github.com/tianocore/edk2/pull/2229 >>>> >>>> *https://github.com/mdkinney/edk2/actions/runs/1521618836 >>> >>>> >>>> Additional details on this update below. >>>> >>>> Thank you, >>>> >>>> Mike >>>> >>>> Changes from Update #3 >>>> >>>> -----------------------------------------------------------------= ----------- >>>> >>>> * Pushed PR (5) >>>> >>>> * Added link to PR(6). EDK II CI Status is PASS. Build Compare PA= SS. >>>> >>>> * Waiting for review of PR (6) >>>> >>>> * Review of PR (7) completed and waiting for review of PR (6) >>>> >>>> -----------------------------------------------------------------= ----------- >>>> >>>> Changes from Update #2 >>>> >>>> -----------------------------------------------------------------= ----------- >>>> >>>> * Changed order of PRs swapping (4) and (5).The PR that activates >>>> >>>> increases the max CI agent job time is independent of all the oth= er >>>> >>>> PRs and its review is complete, so it can be committed now. >>>> >>>> * Pushed PRs (1), (2), (3), (4). >>>> >>>> * Waiting for review to complete for PRs (5) and (6) >>>> >>>> * Reviews complete for PR (7) >>>> >>>> * Identifies steps using git filter-branch to apply uncrustify ch= anges to a >>>> >>>> code review patch series that was generated before the uncrustify= changes >>>> >>>> avoiding manual merge. >>>> >>>> * Identified steps using git filter-repo to generate an alternate= history of >>>> >>>> the edk2 repo with uncrustify changes applied on every commit.Thi= s may >>>> >>>> be useful when evaluating changes to files using tools like git b= lame >>>> >>>> without the large uncrustify patch series. >>>> >>>> -----------------------------------------------------------------= ---------- >>>> >>>> Changes from Update #1 >>>> >>>> -----------------------------------------------------------------= ----------- >>>> >>>> * Changed order of PRs swapping (6) and (7).The PR that activates >>>> >>>> EDK II CI check UncrustifyCheck has to be last because it uncondi= tionally >>>> >>>> checks all C/H files in all packages.Not just files that have bee= n >>>> >>>> modified like some of the other checkers. >>>> >>>> * Updated link to the branch with the UncrustifyCheck plugin that= has been >>>> >>>> updated with a one line change and Reviewed-by and Tested-by tags= . >>>> >>>> https://github.com/mdkinney/edk2/tree/Bug_3748_add_uncrustify_ci_= plugin_v6 >>> >>>> >>>> * Reviews complete for (1), (2), (3), (5), and (7) >>>> >>>> -----------------------------------------------------------------= ---------- >>>> >>>> Michael Kubacki and I have prepared the patches required to apply= the >>>> >>>> uncrustify changes and enable EDK II CI to check all submitted >>>> >>>> patches have been run through uncrustify. >>>> >>>> We have verified through the CompareBuild GitHub Action that the >>>> >>>> format changes performed by uncrustify have no functional changes= . >>>> >>>> All of the OBJ, LIB, DLL, EFI, FFS, FV, and FD files match 100% >>>> >>>> across 70 VS2019/GCC5 builds of all package/platform DSC files in >>>> >>>> the edk2 repo. >>>> >>>> The hard freeze will be extended after the edk2-stable202111 tag = until >>>> >>>> all uncrustify related changes are committed.We do not expect thi= s >>>> >>>> to take more than a few days.Do not push any PRs until the hard >>>> >>>> freeze is lifted. >>>> >>>> The changes are broken up into 7 patch series/PRs.The PRs are ord= ered >>>> >>>> so they can be submitted using the normal submission process and = EDK II >>>> >>>> CI will pass for each one.Details are listed below. >>>> >>>> Uncrustify 73.0.3 for EDK II >>>> >>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D >>>> >>>> * Sources:https://dev.azure.com/projectmu/_git/Uncrustify >>>> >>>> * Documentation:https://dev.azure.com/projectmu/Uncrustify/_wiki/= wikis/Uncrustify.wiki/1/Project-Mu-(EDK-II)-Fork- >>> Readme >>>> >>>> * Download:https://dev.azure.com/projectmu/Uncrustify/_packaging?= _a=3Dpackage&feed=3Dmu_uncrustify&package=3Dmu- >>> uncrustify-release&protocolType=3DNuGet&version=3D73.0.3 >>> >> release&protocolType=3DNuGet&version=3D73.0.3> >>>> >>>> Installing Uncrustify >>>> >>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D >>>> >>>> The Uncrustify tool is installed automatically when the Pytools >>>> >>>> environment is used and the stuart* commands are run to complete = the >>>> >>>> environment setup.Please see: >>>> >>>> https://github.com/tianocore/edk2/tree/master/.pytool#running-ci-= locally >>> >>>> >>>> Uncrustify can also be installed from the download page listed ab= ove >>>> >>>> or built from sources from the source link above. >>>> >>>> The Documentation link provides instruction on how to run uncrust= ify from >>>> >>>> the command line or install as a Visual Studio Code plugin.The ma= in >>>> >>>> uncrustify documentation also describes how to integrate with a f= ew other >>>> >>>> editors. >>>> >>>> We have also discussed a client side githook.That effort has not = started. >>>> >>>> Let us know if that is a feature you would find useful. >>>> >>>> Developer impact for new code reviews >>>> >>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>> >>>> Once the uncrustify checker is active in EDK II CI, developers mu= st >>>> >>>> make sure their patches are run through the uncrustify tool befor= e >>>> >>>> sending the patches for review. >>>> >>>> Developers must install and run uncrustify against changes files = before >>>> >>>> sending patch review emails or submitting PR for EDK II CI.If EDK= II CI >>>> >>>> detects and differences in source formatting, then EDK II CI will= fail >>>> >>>> and the developer must run uncrustify and resubmit the patches. >>>> >>>> Developer impact to patch series/PRs reviewed during edk2-stable2= 01121 soft/hard freeze >>>> >>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>> >>>> Developers must rebase their changes after the uncrustify source = changes are >>>> >>>> committed.The branch with a preview of the uncrustify changes can= be used >>>> >>>> to start this rebase work. >>>> >>>> https://github.com/mdkinney/edk2/tree/Bug_3737_3739_ApplyUncrusti= fyChanges_V5 >>> >>>> >>>> The following steps can be used to update an existing branch with= the >>>> >>>> required uncrustify format.This is the Windows version.I will add >>>> >>>> the Linux version soon. >>>> >>>> 1) Fetch and checkout and rebase to latest edk2/master >>>> >>>> git fetch origin >>>> >>>> git checkout master >>>> >>>> git rebase origin/master >>>> >>>> 2) Make a backup copy of plugin UncrustifyCheck outside WORKSPACE= . >>>> >>>> (e.g. C:\Temp\UncrustifyCheck) so the uncrustify tool executable = and >>>> >>>> EDK II specific uncrustify configuration file available when work= ing >>>> >>>> with a branch that does not have those tools in its scope. >>>> >>>> xcopy .pytool\Plugin\UncrustifyCheck C:\Temp\UncrustifyCheck >>>> >>>> 3) Check out the patch series branch (e.g. MyBranch) >>>> >>>> git checkout MyBranch >>>> >>>> 4) Rebase patch series against edk2-stable202111 >>>> >>>> >>>> >>>> git rebase edk2-stable202111 >>>> >>>> 5) Create new branch for the uncrustifed version (e.g. MyBranch_U= ncrustified) >>>> >>>> git checkout -b MyBranch_Uncrustified >>>> >>>> 6) Use git filter-branch to uncrustify all the commits in the ser= ies >>>> >>>> between the rebase target from (2) and HEAD of the branch.A filte= r >>>> >>>> can be used to scope the uncrustify operations to only the C/H fi= les >>>> >>>> in the specific package the patch series is against. (e.g. Dynami= cTablesPkg). >>>> >>>> BaseTools should always be excluded.If the package scoped filter = is >>>> >>>> not used, it will still work, but will take longer to run because >>>> >>>> uncrustify will rescan every C/H files in the whole repo. >>>> >>>> git filter-branch --tree-filter "git ls-files DynamicTablesPkg*.c= DynamicTablesPkg*.h :!BaseTools/* | >>> c:\\Temp\\UncrustifyCheck\\mu-uncrustify-release_extdep\\Windows-x86\\u= ncrustify.exe -c >>> c:\\Temp\\UncrustifyCheck\\uncrustify.cfg -F - --replace --no-backup --= if-changed" edk2-stable202111..HEAD >>>> >>>> 7) Now that all the individual patches in the branch are uncrusti= fied, >>>> >>>> rebase against latest edk2/master that is already uncrustified. >>>> >>>> git rebase master >>>> >>>> 8) Verify the patches in this new branch. >>>> >>>> Impacts to tracing history across the uncrusity changes >>>> >>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D >>>> >>>> Tools the view file and line history do work with the large uncru= stify >>>> >>>> patch series.One impact is that the operations can be very slow d= ue >>>> >>>> to the large uncrustify patches. >>>> >>>> One option to provide a faster experience is to provide an altern= ate >>>> >>>> version of the edk2 repository as "documentation" that has the >>>> >>>> entire history re-written with uncrustify run on every commit. >>>> >>>> The tool called git-filter-repo can be used to perform this >>>> >>>> transformation and runs in a reasonable period of time (a few hou= rs) >>>> >>>> https://github.com/newren/git-filter-repo >>>> >>>> https://github.com/newren/git-filter-repo/blob/main/contrib/filte= r-repo-demos/lint-history >>> >>>> >>>> The following steps can be used to perform this transformation. >>>> >>>> This is the Windows version. I will add the Linux version soon. >>>> >>>> ** WARNING **This operation modifies(rewrites) all the commits >>>> >>>> in the local copy of the repo.Do not perform >>>> >>>> these steps on a local repo you are using for >>>> >>>> active development. >>>> >>>> 1) Clone edk2 into a new directory (see **WARNING**) >>>> >>>> git clonehttps://github.com/tianocore/edk2.git edk2-uncrustified >>>> >>>> cd edk2-uncrustified >>>> >>>> 2) Setup python virtual env, install pytools, and run stuart comm= ands >>>> >>>> to setup build environment which includes installing uncrustify t= ools. >>>> >>>> https://github.com/tianocore/edk2/tree/master/.pytool#running-ci-= locally >>> >>>> >>>> 3) Make a backup copy of plugin UncrustifyCheck outside WORKSPACE= . >>>> >>>> (e.g. C:\Temp\UncrustifyCheck) so the uncrustify tool executable = and >>>> >>>> EDK II specific uncrustify configuration file available when work= ing >>>> >>>> with a branch that does not have those tools in its scope. >>>> >>>> xcopy .pytool\Plugin\UncrustifyCheck C:\Temp\UncrustifyCheck >>>> >>>> 4) Use lint-history.py from git-filter-repo examples >>>> >>>> https://github.com/newren/git-filter-repo >>>> >>>> https://github.com/newren/git-filter-repo/blob/main/contrib/filte= r-repo-demos/lint-history >>> >>>> >>>> Line #127 - Add try except around subprocess.check_call() with ex= cept >>>> >>>> being pass.This is required because there are a few commits of C >>>> >>>> files in the edk2 repo that have incorrect C syntax and do not >>>> >>>> build with a C compiler and break the uncrustify parser.Skip refo= rmat >>>> >>>> of C files that can not be parsed by uncrustify.These rare instan= ces >>>> >>>> are addressed in the commit that fixes the C syntax error. >>>> >>>> Run this slightly modified version of lint-history.Include only >>>> >>>> C/H files and exclude directories that start with 'Tools' or 'Bas= eTools'. >>>> >>>> This step took about 2.2 hours on a laptop. >>>> >>>> lint-history.py >>>> >>>> --relevant "return (not filename.startswith(b'Tools') and not fil= ename.startswith(b'BaseTools') and >>> (filename.endswith(b'.c') or filename.endswith(b'.h')))" >>>> >>>> c:\\work\\GitHub\\tianocore\\foo\\UncrustifyCheck\\mu-uncrustify-= release_extdep\\Windows-x86\\uncrustify.exe -c >>> c:\\work\\GitHub\\tianocore\\foo\\UncrustifyCheck\\uncrustify.cfg --rep= lace --no-backup --if-changed >>>> >>>> Order of PRs to apply during extended hard freeze >>>> >>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D >>>> >>>> 1) Update EmulatorPkg Win Host [BuildOptions] MSFT CC_FLAGS to no= t force debug information >>>> >>>> *https://bugzilla.tianocore.org/show_bug.cgi?id=3D3747 >>>> >>>> *https://github.com/mdkinney/edk2/tree/Bug_3747_EmulatorPkg_WinHo= st_ReproducibleBuild >>> >>>> >>>> *https://github.com/tianocore/edk2/pull/2215 >>>> >>>> * Required for EmulatorPkg to pass CompareBuild for VS2019 IA32/X= 64 builds. >>>> >>>> * Status: Review complete.PR pushed. >>>> >>>> 2) EccCheck should not revert staged and local changes >>>> >>>> *https://bugzilla.tianocore.org/show_bug.cgi?id=3D2986 >>>> >>>> *https://github.com/mdkinney/edk2/tree/Bug_2986_EccCheckRemoveGit= Revert_V2 >>> >>>> >>>> *https://github.com/tianocore/edk2/pull/2216 >>>> >>>> * Required for EDK II CI to complete in a reasonable period of ti= me when >>>> >>>> processing the 4000+ source file style changes made by uncrustify= . >>>> >>>> * Also fixes critical bugs that can potentially corrupt git state= when >>>> >>>> EccCheck is run locally. >>>> >>>> * Status: Review complete.PR pushed. >>>> >>>> 3) Update pytool LicenseCheck plugin to use temp directory for di= ff output file >>>> >>>> *https://bugzilla.tianocore.org/show_bug.cgi?id=3D3746 >>>> >>>> *https://github.com/mdkinney/edk2/tree/Bug_3746_LicenseCheckUseDi= ffOutputFile_V2 >>> >>>> >>>> *https://github.com/tianocore/edk2/pull/2217 >>>> >>>> * Required to reduce EDK II CI build times. >>>> >>>> * Status: Review complete.PR pushed. >>>> >>>> 4) Update max job time from 60 min to 120 minutes in .azurepipeli= nes/templates >>>> >>>> *https://bugzilla.tianocore.org/show_bug.cgi?id=3D3750 >>>> >>>> *https://github.com/mdkinney/edk2/tree/Bug_3750_IncreaseAzurePipe= linesTimeout >>> >>>> >>>> *https://github.com/tianocore/edk2/pull/2219 >>>> >>>> * Required to allow EccCheck of uncrustify changes to complete on= Azure >>>> >>>> Pipelines CI agents without timing out. >>>> >>>> * Status: Review complete.PR pushed. >>>> >>>> 5) Update Package YAML to ignore specific ECC files/errors >>>> >>>> *https://bugzilla.tianocore.org/show_bug.cgi?id=3D3749 >>>> >>>> *https://github.com/mdkinney/edk2/tree/Bug_3749_EccCheckIgnoreFil= esErrors >>> >>>> >>>> *https://github.com/tianocore/edk2/pull/2218 >>>> >>>> * Required to pass EccCheck >>>> >>>> * Status: Review complete. PR pushed >>>> >>>> 6) Uncrustify Source Changes >>>> >>>> *https://bugzilla.tianocore.org/show_bug.cgi?id=3D3737 >>>> >>>> *https://bugzilla.tianocore.org/show_bug.cgi?id=3D3739 >>>> >>>> *https://github.com/mdkinney/edk2/tree/Bug_3737_3739_ApplyUncrust= ifyChanges_V5 >>> >>>> >>>> *https://github.com/tianocore/edk2/pull/2229 >>>> >>>> * Build comparison result PASS:https://github.com/mdkinney/edk2/a= ctions/runs/1521618836 >>> >>>> >>>> * EFI_D_ -> DEBUG changes required to pass PatchCheck >>>> >>>> * Uncrustify format changes required to pass UncrustifyCheck >>>> >>>> * Status: >>>> >>>> Waiting for review >>>> >>>> 7) UncrustifyCheck EDK II CI Plugin >>>> >>>> *https://bugzilla.tianocore.org/show_bug.cgi?id=3D3748 >>>> >>>> *https://github.com/mdkinney/edk2/tree/Bug_3748_add_uncrustify_ci= _plugin_v6 >>> >>>> >>>> * Required to enforce all PRs submitted to EDK II CI match uncrus= tify format. >>>> >>>> * Unconditionally checks all packages.Can not be committed until = all C/H >>>> >>>> source files have been updated. >>>> >>>> * Status: Review complete >>>> >>>> Combined Branch/PR for Review/Test >>>> >>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>> >>>> * Build Comparison results must pass 100% across the full set of = PRs before >>>> >>>> the individual PRs can be pushed in the order listed above. >>>> >>>> * Branch:https://github.com/mdkinney/edk2/tree/TestOnly_Uncrustif= y_PR_Series >>> >>>> >>>> * PR:https://github.com/tianocore/edk2/pull/2229 >>>> >>>> Status =3D PASS >>>> >>>> * CompareBuild: >>>> >>>> Branch:https://github.com/mdkinney/edk2/tree/Bug_3737_3739_ApplyU= ncrustifyChanges_V5 >>> >>>> >>>> --ref1:ef9a059cdb15844fe52a49af2bf7d86b9dd3e9bf >>>> >>>> --ref2:Bug_3737_3739_ApplyUncrustifyChanges_V5 >>>> >>>> Extra Options: -n 4 --quiet >>>> >>>> Results:https://github.com/mdkinney/edk2/actions/runs/1521618836 >>> >>>> >>>> 30 VS2019 build comparisons PASS >>>> >>>> 40 GCC5 build comparisons PASS >>>> >>>> 100% PASS >>>> >>>> The following git log shows the set of patches from --ref1 to --r= ef 2across >>>> >>>> which there are no differences in any of the OBJ/LIB/DLL/EFI/FFS/= FV/FD files. >>>> >>>> --ref2 >>>> >>>> b7d4bf0675b7 (HEAD -> Bug_3737_3739_ApplyUncrustifyChanges_V5) Un= itTestFrameworkPkg: Apply uncrusitify changes >>>> >>>> 7f03d25f60e7 UefiPayloadPkg: Apply uncrusitify changes >>>> >>>> 0bfd8d9b5ac9 UefiCpuPkg: Apply uncrusitify changes >>>> >>>> e1cd9bfb9dea StandaloneMmPkg: Apply uncrusitify changes >>>> >>>> 5da2f65be378 SourceLevelDebugPkg: Apply uncrusitify changes >>>> >>>> 95b86de07e5d SignedCapsulePkg: Apply uncrusitify changes >>>> >>>> fe71d97246c4 ShellPkg: Apply uncrusitify changes >>>> >>>> 54c21c952992 SecurityPkg: Apply uncrusitify changes >>>> >>>> 187a3785f12b RedfishPkg: Apply uncrusitify changes >>>> >>>> 810100002a46 PcAtChipsetPkg: Apply uncrusitify changes >>>> >>>> 276a695c0cf2 OvmfPkg: Apply uncrusitify changes >>>> >>>> 303c0a91ab07 NetworkPkg: Apply uncrusitify changes >>>> >>>> bc80792cd1b1 MdePkg: Apply uncrusitify changes >>>> >>>> 3ea86be17a2a MdeModulePkg: Apply uncrusitify changes >>>> >>>> c70ef11ed0cd IntelFsp2WrapperPkg: Apply uncrusitify changes >>>> >>>> c0291221f252 IntelFsp2Pkg: Apply uncrusitify changes >>>> >>>> 6a479952a690 FmpDevicePkg: Apply uncrusitify changes >>>> >>>> 3a7c05b7070d FatPkg: Apply uncrusitify changes >>>> >>>> b789f98c8959 EmulatorPkg: Apply uncrusitify changes >>>> >>>> 952d7a1c9220 EmbeddedPkg: Apply uncrusitify changes >>>> >>>> a1cc9881bab6 DynamicTablesPkg: Apply uncrusitify changes >>>> >>>> 50654dfe5785 CryptoPkg: Apply uncrusitify changes >>>> >>>> ed965a02dfa1 ArmVirtPkg: Apply uncrusitify changes >>>> >>>> 9744023fbc46 ArmPlatformPkg: Apply uncrusitify changes >>>> >>>> 7a1cde5f5bba ArmPkg: Apply uncrusitify changes >>>> >>>> 19d17e0913e8 UefiCpuPkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> ffa718b4f994 SourceLevelDebugPkg: Change use of EFI_D_* to DEBUG_= * >>>> >>>> b86cb3c5e5b4 ShellPkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> c7c42204dc07 SecurityPkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> 16b8e6f958e4 PcAtChipsetPkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> 0ac3f8b2dac5 OvmfPkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> bc5004b8d294 NetworkPkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> 6f671a8e2377 MdePkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> a10c610ff9a3 MdeModulePkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> 09a3bddba390 FatPkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> 59c61318246a EmulatorPkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> 3a80367dda3b EmbeddedPkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> 23eb1aaf80ca ArmVirtPkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> 875914b45c54 ArmPlatformPkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> eb2eca82b451 ArmPkg: Change use of EFI_D_* to DEBUG_* >>>> >>>> f0f3f5aae7c4 (origin/master, origin/HEAD, master) UnitTestFramewo= rkPkg: Update YAML to ignore specific ECC >>> files/errors >>>> >>>> c05734797790 UefiPayloadPkg: Update YAML to ignore specific ECC f= iles/errors >>>> >>>> c30c40d6c63d StandaloneMmPkg: Update YAML to ignore specific ECC = files/errors >>>> >>>> 9944508e85f1 ShellPkg: Update YAML to ignore specific ECC files/e= rrors >>>> >>>> 60fa40be458d SecurityPkg: Update YAML to ignore specific ECC file= s/errors >>>> >>>> df790cd6b37e MdePkg: Update YAML to ignore specific ECC files/err= ors >>>> >>>> 9deb9370766e MdeModulePkg: Update YAML to ignore specific ECC fil= es/errors >>>> >>>> d7d30e8f219f EmulatorPkg: Update YAML to ignore specific ECC file= s/errors >>>> >>>> d5744ecba813 CryptoPkg: Update YAML to ignore specific ECC files/= errors >>>> >>>> c97fee87f0f9 ArmVirtPkg: Update YAML to ignore specific ECC files= /errors >>>> >>>> 1939fc9569f2 ArmPlatformPkg: Update YAML to ignore specific ECC f= iles/errors >>>> >>>> 365dced2c37a ArmPkg: Update YAML to ignore specific ECC files/err= ors >>>> >>>> 76a1ce4d5fec .azurepipelines/templates: Update max pipeline job t= ime to 2 hours >>>> >>>> 99f84ff47390 .pytools/Plugin/LicenseCheck: Use temp directory for= git diff output >>>> >>>> 3019f1bbabf1 .pytool/Plugin/EccCheck: Add performance optimizatio= ns >>>> >>>> 854462bd3479 .pytool/Plugin/EccCheck: Remove temp directory on ex= ception >>>> >>>> 69877614fdee .pytool/Plugin/EccCheck: Remove RevertCode() >>>> >>>> --ref1 >>>> >>>> ef9a059cdb15 EmulatorPkg/Win/Host: Update CC_FLAGS >>>> >>>> bb1bba3d7767 (tag: edk2-stable202111) NetworkPkg: Fix invalid poi= nter for DNS response token on error >>>> >>>> Best regards, >>>> >>>> Mike >>>> >>>>=20