Uncrustify Conversion Detailed Plan and Extended Hard Freeze Update #3

Uncrustify Conversion Detailed Plan and Extended Hard Freeze Update #3

[Michael D Kinney]

Hello,

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_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 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/Project-Mu-(EDK-II)-Fork-Readme
* Download: https://dev.azure.com/projectmu/Uncrustify/_packaging?_a=package&feed=mu_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_ApplyUncrustifyChanges_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_ReproducibleBuild
   * 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_LicenseCheckUseDiffOutputFile_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_IncreaseAzurePipelinesTimeout
   * 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
       Partially reviewed

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_ApplyUncrustifyChanges_V5
   * EFI_D_ -> DEBUG changes required to pass PatchCheck
   * Uncrustify format changes required to pass UncrustifyCheck
   * PR not created yet because it will fail EccCheck without BZ #3749
   * Status:
       Waiting for review by stewards

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_v6
   * 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/2220
    Status = PASS
* CompareBuild:
    Branch: https://github.com/mdkinney/edk2/tree/TestOnly_Uncrustify_PR_Series
    --ref1:        198028bf5e46d17f9c502b4ae35ecc406e211c37
    --ref2:        TestOnly_Uncrustify_PR_Series
    Extra Options: -n 4 --quiet
    Results: https://github.com/mdkinney/edk2/actions/runs/1498056902 
	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 641895102bcc (HEAD -> TestOnly_Uncrustify_PR_Series) UnitTestFrameworkPkg: Apply uncrusitify changes
       abdb2f5634db UefiPayloadPkg: Apply uncrusitify changes
       cb9d471105c2 UefiCpuPkg: Apply uncrusitify changes
       e52477cd96a1 StandaloneMmPkg: Apply uncrusitify changes
       c94b69de1b0c SourceLevelDebugPkg: Apply uncrusitify changes
       f4732a917d54 SignedCapsulePkg: Apply uncrusitify changes
       815dcf7b2cd6 ShellPkg: Apply uncrusitify changes
       8a699af3521c SecurityPkg: Apply uncrusitify changes
       d5907a145b37 RedfishPkg: Apply uncrusitify changes
       5ce406d3823b PcAtChipsetPkg: Apply uncrusitify changes
       082006536617 OvmfPkg: Apply uncrusitify changes
       9c670c478ab4 NetworkPkg: Apply uncrusitify changes
       51fa52ef0066 MdePkg: Apply uncrusitify changes
       1b4476301abb MdeModulePkg: Apply uncrusitify changes
       3701ed8a5588 IntelFsp2WrapperPkg: Apply uncrusitify changes
       87db8c3e8f31 IntelFsp2Pkg: Apply uncrusitify changes
       add105d79021 FmpDevicePkg: Apply uncrusitify changes
       f8543fc3e5c5 FatPkg: Apply uncrusitify changes
       65b5fc52fbb4 EmulatorPkg: Apply uncrusitify changes
       3f436d2570b0 EmbeddedPkg: Apply uncrusitify changes
       cf01f39dd80d DynamicTablesPkg: Apply uncrusitify changes
       550476264900 CryptoPkg: Apply uncrusitify changes
       c09e92b19e13 ArmVirtPkg: Apply uncrusitify changes
       3d86c449ef90 ArmPlatformPkg: Apply uncrusitify changes
       bfa1b9c09f07 ArmPkg: Apply uncrusitify changes
       541b0f55afa8 UefiCpuPkg: Change use of EFI_D_* to DEBUG_*
       b783d2499182 SourceLevelDebugPkg: Change use of EFI_D_* to DEBUG_*
       df1821d6ac0e ShellPkg: Change use of EFI_D_* to DEBUG_*
       0c8159204288 SecurityPkg: Change use of EFI_D_* to DEBUG_*
       cb6643c08293 PcAtChipsetPkg: Change use of EFI_D_* to DEBUG_*
       db3f085cb414 OvmfPkg: Change use of EFI_D_* to DEBUG_*
       04199e7b8779 NetworkPkg: Change use of EFI_D_* to DEBUG_*
       a429397c68ef MdePkg: Change use of EFI_D_* to DEBUG_*
       408daf989bb1 MdeModulePkg: Change use of EFI_D_* to DEBUG_*
       7846ceaf387e FatPkg: Change use of EFI_D_* to DEBUG_*
       9934d3b85a31 EmulatorPkg: Change use of EFI_D_* to DEBUG_*
       e5b2066f2cc7 EmbeddedPkg: Change use of EFI_D_* to DEBUG_*
       c4b99a53238f ArmVirtPkg: Change use of EFI_D_* to DEBUG_*
       db8702afc3cf ArmPlatformPkg: Change use of EFI_D_* to DEBUG_*
       f62ed485f30f ArmPkg: Change use of EFI_D_* to DEBUG_*
       04e68228fbbb .pytool/Plugin/Uncrustify: Add Uncrustify plugin
       3d136bbc9935 .azurepipelines/templates: Update max pipeline job time to 2 hours
       34bd4be55e66 UnitTestFrameworkPkg: Update YAML to ignore specific ECC files/errors
       793bedcdfb00 UefiPayloadPkg: Update YAML to ignore specific ECC files/errors
       7e2e089a77f2 StandaloneMmPkg: Update YAML to ignore specific ECC files/errors
       504e1839503c ShellPkg: Update YAML to ignore specific ECC files/errors
       fda332b58dfb SecurityPkg: Update YAML to ignore specific ECC files/errors
       8d24c7c5e971 MdePkg: Update YAML to ignore specific ECC files/errors
       e1182fcdbcea MdeModulePkg: Update YAML to ignore specific ECC files/errors
       8a2b93ad4848 EmulatorPkg: Update YAML to ignore specific ECC files/errors
       eabfb14f432a CryptoPkg: Update YAML to ignore specific ECC files/errors
       cc4fc209113e ArmVirtPkg: Update YAML to ignore specific ECC files/errors
       9ec1a1f0e996 ArmPlatformPkg: Update YAML to ignore specific ECC files/errors
       3e6a038a080a ArmPkg: Update YAML to ignore specific ECC files/errors
       b93693132ec6 .pytools/Plugin/LicenseCheck: Use temp directory for git diff output
       b9fee7fe9d75 .pytool/Plugin/EccCheck: Add performance optimizations
       26164e6e120c .pytool/Plugin/EccCheck: Remove temp directory on exception
       8715acca4b68 .pytool/Plugin/EccCheck: Remove RevertCode()
--ref1 198028bf5e46 EmulatorPkg/Win/Host: Update CC_FLAGS
       e1e7306b5414 (origin/master) OvmfPkg/Library/ResetSystemLib: Fix Microvm VS2019 NOOPT build issue

Best regards,

Mike