From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id C2393D81562 for ; Wed, 14 Feb 2024 15:59:19 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=+lTiGJmJfiakFljktOnEFImZAeCiF5eoadWRVlF2HSQ=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1707926358; v=1; b=O1yxS15SKNibnCoW4HRfRrHFwfndmSPTju5nfi+CPjwyrUn3UO9DS/JF4CIJH81pYNF74YkE nuXuLEaSeu9A3hGM9XGqZzjsof7hoJxuxe1K3B4RMq7lfL3RcG+TEDNJ1PhslRub5Ot7cceQvZC WDigd+jH+cm4GkEKQGRG4JS4= X-Received: by 127.0.0.2 with SMTP id mzFoYY7687511xTDPF8l9Wtr; Wed, 14 Feb 2024 07:59:18 -0800 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.44975.1707926357753421677 for ; Wed, 14 Feb 2024 07:59:17 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 20FC561A97 for ; Wed, 14 Feb 2024 15:59:17 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id BB3B2C433F1 for ; Wed, 14 Feb 2024 15:59:16 +0000 (UTC) X-Received: by mail-lf1-f50.google.com with SMTP id 2adb3069b0e04-51187830d6dso4049556e87.3 for ; Wed, 14 Feb 2024 07:59:16 -0800 (PST) X-Gm-Message-State: UTNsbh33pZ8QPME9t1rNhtLcx7686176AA= X-Google-Smtp-Source: AGHT+IHwztdh87MKI69NuSdJ9iKdwYNvUn60U9LC6jAzBeIpEEJludGiGHZNJ0kEAFCL3aVR8+wWUrGNY++u9YC9D4s= X-Received: by 2002:ac2:5e6a:0:b0:511:49a0:200e with SMTP id a10-20020ac25e6a000000b0051149a0200emr1875534lfr.37.1707926354979; Wed, 14 Feb 2024 07:59:14 -0800 (PST) MIME-Version: 1.0 References: <20240214011751.2529-1-michael.d.kinney@intel.com> In-Reply-To: From: "Ard Biesheuvel" Date: Wed, 14 Feb 2024 16:59:03 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [Patch 1/1] BaseTools/Scripts/PatchCheck: Error if commit modifies multiple packages To: "Kinney, Michael D" , Leif Lindholm Cc: "devel@edk2.groups.io" , Rebecca Cran , Liming Gao , "Feng, Bob C" , "Chen, Christine" , Michael Kubacki Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=O1yxS15S; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none) (cc Leif) On Wed, 14 Feb 2024 at 16:51, Kinney, Michael D wrote: > > Hi Ard, > > Please review the analysis and proposal in the BZ and provide > alternate proposals for the rules. > Hi Mike, I think the logic is fine in principle. But I do have a problem with a rigid application of this logic in the GitHub CI workflow. As a package maintainer, I have to balance this requirement against other requirements, such as bisect-ability, so if this change removes my ability to override this decision, I strongly object to it. Thanks, Ard. > > -----Original Message----- > > From: Ard Biesheuvel > > Sent: Tuesday, February 13, 2024 11:12 PM > > To: devel@edk2.groups.io; Kinney, Michael D > > > > Cc: Rebecca Cran ; Liming Gao > > ; Feng, Bob C ; Chen, > > Christine ; Michael Kubacki > > > > Subject: Re: [edk2-devel] [Patch 1/1] BaseTools/Scripts/PatchCheck: > > Error if commit modifies multiple packages > > > > On Wed, 14 Feb 2024 at 02:18, 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 single commit modifies files in multiple packages > > > * A single commit modifies files in multiple non-package dirs > > > * A single commit modifies files in both a package and a > > > non-package dir. > > > > > > Modifications to files in the root of the repository are not > > > evaluated. > > > > > > The set of packages are found by search for DEC files in the > > > repository. The list of DEC files in the 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 > > > > > > > > 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 > > > > > > > > Cc: Rebecca Cran > > > Cc: Liming Gao > > > Cc: Bob Feng > > > Cc: Yuwei Chen > > > Cc: Michael Kubacki > > > Signed-off-by: Michael D Kinney > > > > Will this trigger pre-merge CI failures if the patch touches more than > > one package? > > > > If so, I will need an opt-out/override for this. > > > > > > > --- > > > BaseTools/Scripts/PatchCheck.py | 49 > > +++++++++++++++++++++++++++++++++ > > > 1 file changed, 49 insertions(+) > > > > > > diff --git a/BaseTools/Scripts/PatchCheck.py > > b/BaseTools/Scripts/PatchCheck.py > > > index 1675dcbd7321..988f152e38d7 100755 > > > --- a/BaseTools/Scripts/PatchCheck.py > > > +++ b/BaseTools/Scripts/PatchCheck.py > > > @@ -665,6 +665,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 ] > > > @@ -681,9 +682,57 @@ class CheckGitCommits: > > > self.ok &= EmailAddressCheck(email, 'Committer').ok > > > patch = self.read_patch_from_git(commit) > > > self.ok &= CheckOnePatch(commit, patch).ok > > > + self.ok &= self.check_parent_packages (dec_files, > > commit) > > > + > > > if not commits: > > > print("Couldn't find commit matching: > > '{}'".format(rev_spec)) > > > > > > + 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 check_parent_packages(self, dec_files, commit): > > > + modified = self.get_parent_packages (dec_files, commit, > > 'AM') > > > + if len (modified) > 1: > > > + print("The commit adds/modifies files in multiple > > packages:\n *", > > > + '\n * '.join(modified)) > > > + self.ok = False > > > + deleted = self.get_parent_packages (dec_files, commit, 'D') > > > + if len (deleted) > 1: > > > + print("The commit deletes files from multiple > > packages:\n *", > > > + '\n * '.join(deleted)) > > > + self.ok = False > > > + return self.ok > > > + > > > + 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 > > > + 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' ] > > > -- > > > 2.40.1.windows.1 > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115463): https://edk2.groups.io/g/devel/message/115463 Mute This Topic: https://groups.io/mt/104345509/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-