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

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

Hi Ard and Leif,

Today in CI we do not have fine grain control to ignore specific
CI check failures when merging.

CI checks can either be informative only in the log or blocking.

For cases where a commit really should be broken up into multiple
commits, the informative approach in a log can be easily missed 
by authors/reviewers/maintainers.

For the suggestion to override a CI check, we would need a way 
to re-run CI with input from a maintainer to relax specific CI
check(s).  The default could be all checks enabled. Some checks
can be enabled/disabled at the package scope through ci.yaml 
file settings. This specific check is for the contents of one
of more commits under review, so the ci.yaml at package scope
does not apply.

A couple ideas

1) Make this initial version of this check informative only and
   figure out how to make the results more visible to the 
   author/reviewers/maintainers.
2) Investigate a mechanism for a maintainer to disable a specific
   check and re-run CI with that check disabled.
   a) Perhaps a flag in the commit message
   b) Perhaps a label in the PR

It would also be helpful if a few examples from the edk2 commit
history where this proposed CI check extension would report a 
failure and it would not be possible to reorganize the commits into
a passing condition. That would help support the hard requirement
for the need to bypass the check.

Best regards,

Mike

> -----Original Message-----
> From: Leif Lindholm <quic_llindhol@quicinc.com>
> Sent: Wednesday, February 14, 2024 8:32 AM
> To: Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Rebecca Cran <rebecca@bsdio.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Feng, Bob C <bob.c.feng@intel.com>; Chen,
> Christine <yuwei.chen@intel.com>; Michael Kubacki
> <mikuback@linux.microsoft.com>
> Subject: Re: [edk2-devel] [Patch 1/1] BaseTools/Scripts/PatchCheck:
> Error if commit modifies multiple packages
> 
> On 2024-02-14 15:59, Ard Biesheuvel wrote:
> > (cc Leif)
> >
> > On Wed, 14 Feb 2024 at 16:51, Kinney, Michael D
> > <michael.d.kinney@intel.com> 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.
> 
> I agree.
> I think it's really good to have this test, and I agree with this
> guideline, but maintainers need the ability to override for the small
> subset of cases where the guideline creates more problems than it
> solves.
> 
> Regards,
> 
> Leif
> 
> > Thanks,
> > Ard.
> >
> >
> >
> >>> -----Original Message-----
> >>> From: Ard Biesheuvel <ardb@kernel.org>
> >>> Sent: Tuesday, February 13, 2024 11:12 PM
> >>> To: devel@edk2.groups.io; Kinney, Michael D
> >>> <michael.d.kinney@intel.com>
> >>> Cc: Rebecca Cran <rebecca@bsdio.com>; Liming Gao
> >>> <gaoliming@byosoft.com.cn>; Feng, Bob C <bob.c.feng@intel.com>;
> Chen,
> >>> Christine <yuwei.chen@intel.com>; Michael Kubacki
> >>> <mikuback@linux.microsoft.com>
> >>> 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
> >>> <michael.d.kinney@intel.com> 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
> >>> <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>
> >>>> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> >>>
> >>> 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
> <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' ]
> >>>> --
> >>>> 2.40.1.windows.1
> >>>>
> >>>>
> >>>>
> >>>> 
> >>>>
> >>>>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115467): https://edk2.groups.io/g/devel/message/115467
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]
-=-=-=-=-=-=-=-=-=-=-=-