From: "Michael Kubacki" <mikuback@linux.microsoft.com>
To: devel@edk2.groups.io, michael.d.kinney@intel.com
Cc: Rebecca Cran <rebecca@bsdio.com>,
Liming Gao <gaoliming@byosoft.com.cn>,
Bob Feng <bob.c.feng@intel.com>,
Yuwei Chen <yuwei.chen@intel.com>,
Ard Biesheuvel <ardb+tianocore@kernel.org>,
Leif Lindholm <quic_llindhol@quicinc.com>
Subject: Re: [edk2-devel] [Patch 4/4] BaseTools/Scripts/PatchCheck: Error if commit modifies multiple packages
Date: Fri, 23 Feb 2024 20:27:00 -0500 [thread overview]
Message-ID: <84d4277c-40fb-4a5a-bf63-abdebd5f4805@linux.microsoft.com> (raw)
In-Reply-To: <20240218205951.497-5-michael.d.kinney@intel.com>
Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>
On 2/18/2024 3:59 PM, Michael D Kinney wrote:
> 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)
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115903): https://edk2.groups.io/g/devel/message/115903
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]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2024-02-24 1:27 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-18 20:59 [edk2-devel] [Patch 0/4] PatchCheck Updates Michael D Kinney
2024-02-18 20:59 ` [edk2-devel] [Patch 1/4] BaseTools/Scripts/PatchCheck: Update Author checks Michael D Kinney
2024-02-27 17:59 ` Rebecca Cran
2024-02-18 20:59 ` [edk2-devel] [Patch 2/4] BaseTools/Scripts/PatchCheck: Return CommitMessageCheck errors Michael D Kinney
2024-02-24 1:26 ` Michael Kubacki
2024-02-18 20:59 ` [edk2-devel] [Patch 3/4] BaseTools/Scripts/PatchCheck: Error if no Cc tags are present Michael D Kinney
2024-02-20 14:48 ` Ard Biesheuvel
2024-02-20 22:09 ` Michael D Kinney
2024-02-21 22:16 ` Laszlo Ersek
2024-02-21 23:32 ` Ard Biesheuvel
2024-02-24 1:26 ` Michael Kubacki
2024-02-18 20:59 ` [edk2-devel] [Patch 4/4] BaseTools/Scripts/PatchCheck: Error if commit modifies multiple packages Michael D Kinney
2024-02-24 1:27 ` Michael Kubacki [this message]
2024-02-27 19:28 ` [edk2-devel] [Patch 0/4] PatchCheck Updates Rebecca Cran
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=84d4277c-40fb-4a5a-bf63-abdebd5f4805@linux.microsoft.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox