Re: [edk2-devel] [PATCH v1 2/4] .pytool/Plugin/UncrustifyCheck: Add ignore file support

["Michael D Kinney" <michael.d.kinney@intel.com>]

Can you explain how this filename is used?  Perhaps add a comment in this series.

If this pattern is used in several places, then I agree that can be cleaned up in a different patch series.

Mike 


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
> Sent: Friday, April 1, 2022 9:07 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Sean Brogan <sean.brogan@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH v1 2/4] .pytool/Plugin/UncrustifyCheck: Add ignore file support
> 
> That is borrowed from a pattern used in other plugins. Example:
> https://github.com/tianocore/edk2/blob/master/.pytool/Plugin/SpellCheck/SpellCheck.py#L160-#L161
> 
> I don't mind updating the plugins to do something different if that is
> desired, but perhaps that should be a separate change.
> 
> Regards,
> Michael
> 
> On 3/22/2022 12:14 PM, Michael D Kinney wrote:
> > Michael,
> >
> > One comment below.
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> >> Sent: Monday, March 21, 2022 6:58 PM
> >> To: devel@edk2.groups.io
> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Sean Brogan
> >> <sean.brogan@microsoft.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
> >> Subject: [PATCH v1 2/4] .pytool/Plugin/UncrustifyCheck: Add ignore file support
> >>
> >> From: Michael Kubacki <michael.kubacki@microsoft.com>
> >>
> >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3880
> >>
> >> Currently UncrustifyCheck.py provides the following exclusion
> >> options:
> >>
> >> 1. Override the type of files UncrustifyCheck operates against by
> >> default (.c and .h files). Using the "IgnoreStandardPaths"
> >> configuration option.
> >>
> >> 2. By default, UncrustifyCheck skips files in git submodules and
> >> ignored by git (the "SkipGitExclusions" configuration option can
> >> override this behavior).
> >>
> >> The goal of UncrustifyCheck is to provide consistent formatting
> >> across the codebase. In some rare circumstances, maintainers might
> >> need to exclude a specific file (or file pattern) within their
> >> package. For example, a small set of auto-generated files from
> >> another repository.
> >>
> >> This change adds a new configuration option that can be specified
> >> in a package CI YAML file to describe a list of files within the
> >> package that should be ignored by UncrustifyCheck.
> >>
> >> The configuration option is called "IgnoreFiles" and it uses similar
> >> syntax to git ignore to ignore a list of files.
> >>
> >> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> >> Cc: Sean Brogan <sean.brogan@microsoft.com>
> >> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> >> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> >> ---
> >>   .pytool/Plugin/UncrustifyCheck/Readme.md          |  7 ++++++
> >>   .pytool/Plugin/UncrustifyCheck/UncrustifyCheck.py | 24 ++++++++++++++++++++
> >>   2 files changed, 31 insertions(+)
> >>
> >> diff --git a/.pytool/Plugin/UncrustifyCheck/Readme.md b/.pytool/Plugin/UncrustifyCheck/Readme.md
> >> index 0c46fd241a7a..efe7a573e4fa 100644
> >> --- a/.pytool/Plugin/UncrustifyCheck/Readme.md
> >> +++ b/.pytool/Plugin/UncrustifyCheck/Readme.md
> >> @@ -41,6 +41,7 @@ The plugin can be configured with a few optional configuration options.
> >>         "AdditionalIncludePaths": [], # Additional paths to check formatting (wildcards supported).
> >>         "AuditOnly": False,           # Don't fail the build if there are errors.  Just log them.
> >>         "ConfigFilePath": "",         # Custom path to an Uncrustify config file.
> >> +      "IgnoreFiles": [],            # A list of file patterns to ignore.
> >>         "IgnoreStandardPaths": [],    # Standard Plugin defined paths that should be ignored.
> >>         "OutputFileDiffs": True,      # Output chunks of formatting diffs in the test case log.
> >>                                       # This can significantly slow down the plugin on very large packages.
> >> @@ -67,6 +68,12 @@ the test as skipped. This allows visibility into the failures without breaking t
> >>
> >>   When specified in the config file, this is a package relative path to the Uncrustify configuration file.
> >>
> >> +### `IgnoreFiles`
> >> +
> >> +This option supports .gitignore file and folder matching strings including wildcards.
> >> +
> >> +The files specified by this configuration option will not be processed by Uncrustify.
> >> +
> >>   ### `IgnoreStandardPaths`
> >>
> >>   This plugin by default will check the below standard paths. A package configuration file can specify any of these paths
> >> diff --git a/.pytool/Plugin/UncrustifyCheck/UncrustifyCheck.py b/.pytool/Plugin/UncrustifyCheck/UncrustifyCheck.py
> >> index dc133fecc4b2..e8370381032f 100644
> >> --- a/.pytool/Plugin/UncrustifyCheck/UncrustifyCheck.py
> >> +++ b/.pytool/Plugin/UncrustifyCheck/UncrustifyCheck.py
> >> @@ -13,11 +13,13 @@ import os
> >>   import pathlib
> >>   import shutil
> >>   import timeit
> >> +from collections.abc import Callable
> >>   from edk2toolext.environment import version_aggregator
> >>   from edk2toolext.environment.plugin_manager import PluginManager
> >>   from edk2toolext.environment.plugintypes.ci_build_plugin import ICiBuildPlugin
> >>   from edk2toolext.environment.plugintypes.uefi_helper_plugin import HelperFunctions
> >>   from edk2toolext.environment.var_dict import VarDict
> >> +from edk2toollib.gitignore_parser import parse_gitignore_lines
> >>   from edk2toollib.log.junit_report_format import JunitReportTestCase
> >>   from edk2toollib.uefi.edk2.path_utilities import Edk2Path
> >>   from edk2toollib.utility_functions import  RunCmd
> >> @@ -273,6 +275,15 @@ class UncrustifyCheck(ICiBuildPlugin):
> >>               f"-c {self._app_config_file} -F {self._app_input_file_path} --if-changed --suffix
> >> {UncrustifyCheck.FORMATTED_FILE_EXTENSION}", outstream=output)
> >>           self._app_output = output.getvalue().strip().splitlines()
> >>
> >> +    def _get_files_ignored_in_config(self) -> Callable[[str], bool]:
> >> +        """"
> >> +        Returns a function that returns true if a given file string path is ignored in the plugin configuration file and
> false
> >> otherwise.
> >> +        """
> >> +        ignored_files = []
> >> +        if "IgnoreFiles" in self._package_config:
> >> +            ignored_files = self._package_config["IgnoreFiles"]
> >> +        return parse_gitignore_lines(ignored_files, os.path.join(self._abs_workspace_path, "nofile.txt"),
> >> self._abs_workspace_path)
> >
> > What is the purpose of "nofile.txt" here?  If this a param that is optional/ignored?  If so, there
> > are better python methods to do that.
> >
> >> +
> >>       def _get_git_ignored_paths(self) -> List[str]:
> >>           """"
> >>           Returns a list of file absolute path strings to all files ignored in this git repository.
> >> @@ -464,6 +475,19 @@ class UncrustifyCheck(ICiBuildPlugin):
> >>               self._abs_file_paths_to_format.extend(
> >>                   [str(path.resolve()) for path in pathlib.Path(self._abs_package_path).rglob(path)])
> >>
> >> +        # Remove files ignore in the plugin configuration file
> >> +        plugin_ignored_files = list(filter(self._get_files_ignored_in_config(), self._abs_file_paths_to_format))
> >> +
> >> +        if plugin_ignored_files:
> >> +            logging.info(
> >> +                f"{self._package_name} file count before plugin ignore file exclusion:
> {len(self._abs_file_paths_to_format)}")
> >> +            for path in plugin_ignored_files:
> >> +                if path in self._abs_file_paths_to_format:
> >> +                    logging.info(f"  File ignored in plugin config file: {path}")
> >> +                    self._abs_file_paths_to_format.remove(path)
> >> +            logging.info(
> >> +                f"{self._package_name} file count after plugin ignore file exclusion:
> {len(self._abs_file_paths_to_format)}")
> >> +
> >>           if not "SkipGitExclusions" in self._package_config or not self._package_config["SkipGitExclusions"]:
> >>               # Remove files ignored by git
> >>               logging.info(
> >> --
> >> 2.28.0.windows.1
> >
> >
> >
> >
> >
> >
> 
> 
> 
>