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 F2446942161 for ; Wed, 14 Feb 2024 16:32:34 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=/PXws3Thpj6vPeaedXxVSrb/f4Pt4JFJViOoxPPZ3Is=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:CC:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1707928353; v=1; b=ugGA4IYjBAXUX1UExrXzq5Pju7sGS2YGsG/rUbYZ2Uk0f57FQefSQI38rvYp0XMKqxCp6RiZ XjGZhIgLyZdBooC6IaIU4eM/KATm/cKspQR+0+w4nN/NojQ1F/xDQZScU4VLeb5G7or1ML5BFXv cdGnEELQWDiKPFyjdgznFr9U= X-Received: by 127.0.0.2 with SMTP id KeCqYY7687511xMeBIizcbtU; Wed, 14 Feb 2024 08:32:33 -0800 X-Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by mx.groups.io with SMTP id smtpd.web11.45925.1707928352654870056 for ; Wed, 14 Feb 2024 08:32:32 -0800 X-Received: from pps.filterd (m0279871.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.24/8.17.1.24) with ESMTP id 41EELbI4029241; Wed, 14 Feb 2024 16:32:22 GMT X-Received: from nasanppmta05.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3w88gq35qw-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 14 Feb 2024 16:32:21 +0000 (GMT) X-Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA05.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 41EGWK5a007625 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 14 Feb 2024 16:32:20 GMT X-Received: from [10.111.132.144] (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.40; Wed, 14 Feb 2024 08:32:18 -0800 Message-ID: Date: Wed, 14 Feb 2024 16:32:16 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [Patch 1/1] BaseTools/Scripts/PatchCheck: Error if commit modifies multiple packages To: Ard Biesheuvel , "Kinney, Michael D" CC: "devel@edk2.groups.io" , Rebecca Cran , Liming Gao , "Feng, Bob C" , "Chen, Christine" , Michael Kubacki References: <20240214011751.2529-1-michael.d.kinney@intel.com> From: "Leif Lindholm" In-Reply-To: X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-GUID: YuG6ZMe5QrRuLSW_dyOxCJ8gUUpHiepa X-Proofpoint-ORIG-GUID: YuG6ZMe5QrRuLSW_dyOxCJ8gUUpHiepa 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,quic_llindhol@quicinc.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: o92bScugY7TWCwrFpbc4Fqenx7686176AA= Content-Language: en-GB Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=ugGA4IYj; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=quicinc.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 2024-02-14 15:59, Ard Biesheuvel wrote: > (cc Leif) >=20 > 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. >> >=20 > Hi Mike, >=20 > 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. I agree. I think it's really good to have this test, and I agree with this=20 guideline, but maintainers need the ability to override for the small=20 subset of cases where the guideline creates more problems than it solves. Regards, Leif > Thanks, > Ard. >=20 >=20 >=20 >>> -----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=3D4679 >>>> >>>> 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=3DAM -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=3DD -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 =3D self.read_dec_files_from_git() >>>> commits =3D self.read_commit_list_from_git(rev_spec, >>> max_count) >>>> if len(commits) =3D=3D 1 and Verbose.level > Verbose.ONELINE= : >>>> commits =3D [ rev_spec ] >>>> @@ -681,9 +682,57 @@ class CheckGitCommits: >>>> self.ok &=3D EmailAddressCheck(email, 'Committer').ok >>>> patch =3D self.read_patch_from_git(commit) >>>> self.ok &=3D CheckOnePatch(commit, patch).ok >>>> + self.ok &=3D 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 =3D self.read_files_modified_from_git (commit, >>> filter) >>>> + parents =3D set() >>>> + for file in filelist: >>>> + dec_found =3D False >>>> + for dec_file in dec_files: >>>> + if os.path.commonpath([dec_file, file]): >>>> + dec_found =3D 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 =3D 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 =3D False >>>> + deleted =3D 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 =3D False >>>> + return self.ok >>>> + >>>> + def read_dec_files_from_git(self): >>>> + # run git ls-files *.dec >>>> + out =3D 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 =3D self.run_git('diff-tree', '--no-commit-id', '--name- >>> only', >>>> + '--diff-filter=3D' + 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 =3D [ 'rev-list', '--abbrev-commit', '--no-walk' ] >>>> -- >>>> 2.40.1.windows.1 >>>> >>>> >>>> >>>>=20 >>>> >>>> -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115465): https://edk2.groups.io/g/devel/message/115465 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-