Re: Uncrustify Conversion Detailed Plan and Extended Hard Freeze Update #4

["Wang, Jian J" <jian.j.wang@intel.com>]

For SecurityPkg and CryptoPkg,

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

Regards,
Jian

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D
> Kinney
> Sent: Wednesday, December 01, 2021 6:34 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>;
> Michael Kubacki <michael.kubacki@microsoft.com>; Andrew Fish
> (afish@apple.com) <afish@apple.com>; Leif Lindholm <leif@nuviainc.com>
> Subject: [edk2-devel] Uncrustify Conversion Detailed Plan and Extended Hard
> Freeze Update #4
> 
> 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 its
> 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 results
> and BuildCompare results.  I do not expect a line by line review
> because we already had time to provide feedback on the source style
> performed by uncrustify.  Instead, a Reviewed-by for your package
> indicates that you have reviewed the EDK II CI results and CompareBuild
> tool functionality and results and you accept the source style
> changes to your package.
> 
>    *
> https://github.com/mdkinney/edk2/tree/Bug_3737_3739_ApplyUncrustifyChan
> ges_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 PASS.
> * 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 other
>   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 changes 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.  This may
>   be useful when evaluating changes to files using tools like git blame
>   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 unconditionally
>   checks all C/H files in all packages.  Not just files that have been
>   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_v
> 6
> * 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 this
> 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 ordered
> 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
> =============================
> * Sources: https://dev.azure.com/projectmu/_git/Uncrustify
> * Documentation:
> https://dev.azure.com/projectmu/Uncrustify/_wiki/wikis/Uncrustify.wiki/1/Proj
> ect-Mu-(EDK-II)-Fork-Readme
> * Download:
> https://dev.azure.com/projectmu/Uncrustify/_packaging?_a=package&feed=m
> u_uncrustify&package=mu-uncrustify-
> release&protocolType=NuGet&version=73.0.3
> 
> Installing Uncrustify
> ======================
> 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 above
> or built from sources from the source link above.
> 
> The Documentation link provides instruction on how to run uncrustify from
> the command line or install as a Visual Studio Code plugin.  The main
> uncrustify documentation also describes how to integrate with a few 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
> ======================================
> Once the uncrustify checker is active in EDK II CI, developers must
> make sure their patches are run through the uncrustify tool before
> 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-stable201121
> soft/hard freeze
> =================================================================
> ======================
> 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_ApplyUncrustifyChan
> ges_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 working
>        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_Uncrustified)
> 
>            git checkout -b MyBranch_Uncrustified
> 
>     6) Use git filter-branch to uncrustify all the commits in the series
>        between the rebase target from (2) and HEAD of the branch.  A filter
>        can be used to scope the uncrustify operations to only the C/H files
>        in the specific package the patch series is against. (e.g. DynamicTablesPkg).
>        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\\uncrustify.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 uncrustified,
>        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
> =======================================================
> Tools the view file and line history do work with the large uncrustify
> patch series.  One impact is that the operations can be very slow due
> to the large uncrustify patches.
> 
> One option to provide a faster experience is to provide an alternate
> 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 hours)
> 
>    https://github.com/newren/git-filter-repo
>    https://github.com/newren/git-filter-repo/blob/main/contrib/filter-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 clone https://github.com/tianocore/edk2.git edk2-uncrustified
>            cd edk2-uncrustified
> 
>     2) Setup python virtual env, install pytools, and run stuart commands
>        to setup build environment which includes installing uncrustify tools.
> 
>            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 working
>        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/filter-repo-
> demos/lint-history
> 
>        Line #127 - Add try except around subprocess.check_call() with except
>        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 reformat
>        of C files that can not be parsed by uncrustify.  These rare instances
>        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 'BaseTools'.
>        This step took about 2.2 hours on a laptop.
> 
>            lint-history.py
>                --relevant "return (not filename.startswith(b'Tools') and not
> filename.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 --replace --
> no-backup --if-changed
> 
> Order of PRs to apply during extended hard freeze
> ==================================================
> 1) Update EmulatorPkg Win Host [BuildOptions] MSFT CC_FLAGS to not force
> debug information
>    * https://bugzilla.tianocore.org/show_bug.cgi?id=3747
>    *
> https://github.com/mdkinney/edk2/tree/Bug_3747_EmulatorPkg_WinHost_Rep
> roducibleBuild
>    * https://github.com/tianocore/edk2/pull/2215
>    * Required for EmulatorPkg to pass CompareBuild for VS2019 IA32/X64 builds.
>    * Status: Review complete.  PR pushed.
> 
> 2) EccCheck should not revert staged and local changes
>    * https://bugzilla.tianocore.org/show_bug.cgi?id=2986
>    *
> https://github.com/mdkinney/edk2/tree/Bug_2986_EccCheckRemoveGitRevert
> _V2
>    * https://github.com/tianocore/edk2/pull/2216
>    * Required for EDK II CI to complete in a reasonable period of time 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 diff output file
>    * https://bugzilla.tianocore.org/show_bug.cgi?id=3746
>    *
> https://github.com/mdkinney/edk2/tree/Bug_3746_LicenseCheckUseDiffOutput
> File_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 .azurepipelines/templates
>    * https://bugzilla.tianocore.org/show_bug.cgi?id=3750
>    *
> https://github.com/mdkinney/edk2/tree/Bug_3750_IncreaseAzurePipelinesTime
> out
>    * 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=3749
>    *
> https://github.com/mdkinney/edk2/tree/Bug_3749_EccCheckIgnoreFilesErrors
>    * 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=3737
>    * https://bugzilla.tianocore.org/show_bug.cgi?id=3739
>    *
> https://github.com/mdkinney/edk2/tree/Bug_3737_3739_ApplyUncrustifyChan
> ges_V5
>    * https://github.com/tianocore/edk2/pull/2229
>    * Build comparison result PASS:
> https://github.com/mdkinney/edk2/actions/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=3748
>    *
> https://github.com/mdkinney/edk2/tree/Bug_3748_add_uncrustify_ci_plugin_v
> 6
>    * Required to enforce all PRs submitted to EDK II CI match uncrustify 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
> ==================================
> * 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_Uncrustify_PR_Series
> * PR: https://github.com/tianocore/edk2/pull/2229
>     Status = PASS
> * CompareBuild:
>     Branch:
> https://github.com/mdkinney/edk2/tree/Bug_3737_3739_ApplyUncrustifyChan
> ges_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 --ref 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)
> UnitTestFrameworkPkg: 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) UnitTestFrameworkPkg:
> Update YAML to ignore specific ECC files/errors
> c05734797790 UefiPayloadPkg: Update YAML to ignore specific ECC files/errors
> c30c40d6c63d StandaloneMmPkg: Update YAML to ignore specific ECC
> files/errors
> 9944508e85f1 ShellPkg: Update YAML to ignore specific ECC files/errors
> 60fa40be458d SecurityPkg: Update YAML to ignore specific ECC files/errors
> df790cd6b37e MdePkg: Update YAML to ignore specific ECC files/errors
> 9deb9370766e MdeModulePkg: Update YAML to ignore specific ECC files/errors
> d7d30e8f219f EmulatorPkg: Update YAML to ignore specific ECC files/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 files/errors
> 365dced2c37a ArmPkg: Update YAML to ignore specific ECC files/errors
> 76a1ce4d5fec .azurepipelines/templates: Update max pipeline job time to 2
> hours
> 99f84ff47390 .pytools/Plugin/LicenseCheck: Use temp directory for git diff
> output
> 3019f1bbabf1 .pytool/Plugin/EccCheck: Add performance optimizations
> 854462bd3479 .pytool/Plugin/EccCheck: Remove temp directory on exception
> 69877614fdee .pytool/Plugin/EccCheck: Remove RevertCode()
> --ref1
> ef9a059cdb15 EmulatorPkg/Win/Host: Update CC_FLAGS
> bb1bba3d7767 (tag: edk2-stable202111) NetworkPkg: Fix invalid pointer for DNS
> response token on error
> 
> Best regards,
> 
> Mike
> 
> 
> 
> 
> 
> 
> 
> 
>