[edk2-devel] [Patch 4/4] BaseTools/Scripts/PatchCheck: Error if commit modifies multiple packages

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

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4679

Update PatchCheck.py to evaluate all the files modified in each commit
and generate an error if:
* A commit adds/modifies files in multiple package directories
* A commit adds/modifies files in multiple non-package directories
* A commit adds/modifies files in both a package and a non-package
  directory
* A commit deletes files from multiple package directories
* A commit deletes files from multiple non-package directories
* A commit deletes files from both a package and a non-package
  directory

Modifications to files in the root of the repository are not
evaluated.

This check is skipped if PatchCheck.py is run on a patch file or
input from stdin because this multiple package commit check depends
on information from a git repository.

If --ignore-multi-package option is set, then reduce the multiple
package commit check from an error to a warning for all commits in
the commit range provided to PatchCheck.py.

Add check for a 'Continuous-integration-options:' commit message
tag that allows one or more options to be specified at the individual
commit scope to enable/disable continuous integration checks. This
tag must start at the beginning of a commit message line and may
appear more than once in a commit message.

Add support for a Continuous-integration-options tag value of
'PatchCheck.ignore-multi-package' that reduces the multiple package
commit check from an error to a warning for the specific commits that
specify this option.  Example:

  Continuous-integration-options: PatchCheck.ignore-multi-package

The set of packages are found by searching for DEC files in a git
repository. The list of DEC files in a git repository is collected
with the following git command:

  git ls-files *.dec

The set of files added/modified by each commit is found using the
following git command:

  git diff-tree --no-commit-id --name-only --diff-filter=AM -r <commit>

The set of files deleted by each commit is found using the
following git command:

  git diff-tree --no-commit-id --name-only --diff-filter=D -r <commit>

Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Yuwei Chen <yuwei.chen@intel.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 BaseTools/Scripts/PatchCheck.py | 77 ++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 415198e3824e..a3b812fb7324 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -28,6 +28,7 @@ class Verbose:
 
 class PatchCheckConf:
     ignore_change_id = False
+    ignore_multi_package = False
 
 class EmailAddressCheck:
     """Checks an email address."""
@@ -98,6 +99,7 @@ class CommitMessageCheck:
 
     def __init__(self, subject, message, author_email):
         self.ok = True
+        self.ignore_multi_package = False
 
         if subject is None and  message is None:
             self.error('Commit message is missing!')
@@ -120,6 +122,7 @@ class CommitMessageCheck:
             self.check_overall_format()
             if not PatchCheckConf.ignore_change_id:
                 self.check_change_id_format()
+            self.check_ci_options_format()
         self.report_message_result()
 
     url = 'https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format'
@@ -324,6 +327,15 @@ class CommitMessageCheck:
             self.error('\"%s\" found in commit message:' % cid)
             return
 
+    def check_ci_options_format(self):
+        cio='Continuous-integration-options:'
+        for line in self.msg.splitlines():
+            if not line.startswith(cio):
+                continue
+            options = line.split(':', 1)[1].split()
+            if 'PatchCheck.ignore-multi-package' in options:
+                self.ignore_multi_package = True
+
 (START, PRE_PATCH, PATCH) = range(3)
 
 class GitDiffCheck:
@@ -561,6 +573,7 @@ class CheckOnePatch:
 
         msg_check = CommitMessageCheck(self.commit_subject, self.commit_msg, self.author_email)
         msg_ok = msg_check.ok
+        self.ignore_multi_package = msg_check.ignore_multi_package
 
         diff_ok = True
         if self.diff is not None:
@@ -671,6 +684,7 @@ class CheckGitCommits:
     """
 
     def __init__(self, rev_spec, max_count):
+        dec_files = self.read_dec_files_from_git()
         commits = self.read_commit_list_from_git(rev_spec, max_count)
         if len(commits) == 1 and Verbose.level > Verbose.ONELINE:
             commits = [ rev_spec ]
@@ -686,10 +700,66 @@ class CheckGitCommits:
             email = self.read_committer_email_address_from_git(commit)
             self.ok &= EmailAddressCheck(email, 'Committer').ok
             patch = self.read_patch_from_git(commit)
-            self.ok &= CheckOnePatch(commit, patch).ok
+            check_patch = CheckOnePatch(commit, patch)
+            self.ok &= check_patch.ok
+            ignore_multi_package = check_patch.ignore_multi_package
+            if PatchCheckConf.ignore_multi_package:
+                ignore_multi_package = True
+            prefix = 'WARNING: ' if ignore_multi_package else ''
+            check_parent = self.check_parent_packages (dec_files, commit, prefix)
+            if not ignore_multi_package:
+                self.ok &= check_parent
+
         if not commits:
             print("Couldn't find commit matching: '{}'".format(rev_spec))
 
+    def check_parent_packages(self, dec_files, commit, prefix):
+        ok = True
+        modified = self.get_parent_packages (dec_files, commit, 'AM')
+        if len (modified) > 1:
+            print("{}The commit adds/modifies files in multiple packages:".format(prefix))
+            print(" *", '\n * '.join(modified))
+            ok = False
+        deleted = self.get_parent_packages (dec_files, commit, 'D')
+        if len (deleted) > 1:
+            print("{}The commit deletes files from multiple packages:".format(prefix))
+            print(" *", '\n * '.join(deleted))
+            ok = False
+        return ok
+
+    def get_parent_packages(self, dec_files, commit, filter):
+        filelist = self.read_files_modified_from_git (commit, filter)
+        parents = set()
+        for file in filelist:
+            dec_found = False
+            for dec_file in dec_files:
+                if os.path.commonpath([dec_file, file]):
+                    dec_found = True
+                    parents.add(dec_file)
+            if not dec_found and os.path.dirname (file):
+                # No DEC file found and file is in a subdir
+                # Covers BaseTools, .github, .azurepipelines, .pytool
+                parents.add(file.split('/')[0])
+        return list(parents)
+
+    def read_dec_files_from_git(self):
+        # run git ls-files *.dec
+        out = self.run_git('ls-files', '*.dec')
+        # return list of .dec files
+        try:
+            return out.split()
+        except:
+            return []
+
+    def read_files_modified_from_git(self, commit, filter):
+        # run git diff-tree --no-commit-id --name-only -r <commit>
+        out = self.run_git('diff-tree', '--no-commit-id', '--name-only',
+                           '--diff-filter=' + filter, '-r', commit)
+        try:
+            return out.split()
+        except:
+            return []
+
     def read_commit_list_from_git(self, rev_spec, max_count):
         # Run git to get the commit patch
         cmd = [ 'rev-list', '--abbrev-commit', '--no-walk' ]
@@ -800,6 +870,9 @@ class PatchCheckApp:
         group.add_argument("--ignore-change-id",
                            action="store_true",
                            help="Ignore the presence of 'Change-Id:' tags in commit message")
+        group.add_argument("--ignore-multi-package",
+                           action="store_true",
+                           help="Ignore if commit modifies files in multiple packages")
         self.args = parser.parse_args()
         if self.args.oneline:
             Verbose.level = Verbose.ONELINE
@@ -807,6 +880,8 @@ class PatchCheckApp:
             Verbose.level = Verbose.SILENT
         if self.args.ignore_change_id:
             PatchCheckConf.ignore_change_id = True
+        if self.args.ignore_multi_package:
+            PatchCheckConf.ignore_multi_package = True
 
 if __name__ == "__main__":
     sys.exit(PatchCheckApp().retval)
-- 
2.40.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115572): https://edk2.groups.io/g/devel/message/115572
Mute This Topic: https://groups.io/mt/104434585/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-