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,

Using commit message does provide granularity down to a single commit,
which may be better than PR label which applies across all commits
in a series.

However, a flag in the commit message can be set by the author who may
not be a maintainer and maintainers would then need to review commit
messages for incorrect use of those flags.

Only maintainers/admins can set labels, so from a permission management
perspective the PR label has an advantage.

Additional comments below.  There are ways to support bisect and meet
the proposed checks. The suggestion uses techniques that Laszlo helped
me with in the past when working on issues like there. I have seen more
complex scenarios than the examples listed below, and have been able to
figure out a path through.

I would agree it is extra work to think about these when working on
the code changes and extra work to reformulate the patches when these
conditions are encountered. 

I just want to be clear on the objections.  It is not about if the patches
can be organized to follow this proposal.  The objection is about the 
extra work required to reformulate patches.

Thanks,

Mike

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Wednesday, February 14, 2024 1:54 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>; 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 Wed, 14 Feb 2024 at 18:16, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> > Hi Ard and Leif,
> >
> > Today in CI we do not have fine grain control to ignore specific
> > CI check failures when merging.
> >
> 
> I have been asking for this for a while now.
> 
> > 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.
> >
> 
> I turned off some CI checks in packages entirely, simply because I
> cannot override a single CI fail from the dashboard.
> 
> > 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.
> 
> Making this informative only for all PRs likely defeats the purpose.
> 
> > 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
> >
> 
> I'd lean towards a), as it will be logged in the commit history. It
> also requires some extra work to respin the PR, and this should make
> abuse of this feature less likely in cases where the change can easily
> just be split up instead.
> 
> > 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.
> >
> 
> I didn't look at all the patches in the ticket, but there are at least
> two which cannot be split up without breaking the build.
> 
> 103fa647d159e3d76be2634d2653c2d215dd0d46
> ArmPkg: Replace CoreId and ClusterId with Mpidr in ARM_CORE_INFO struct

This one could follow the proposal and be bisectable.  Laszlo provide
guidance on this in the past

* Patch #1 to update ArmPkg .h files adding 2 new fields leaving old field in place
* Patch #2 to update ArmPlatformPkg to stop using old field and use 2 new fields
* Patch #3 to update ArmPkg .h file to remove unused field
> 
> 4c55f6394fafe0494ec24e7c05cb68c938d7852d
> MdePkg: IORT header update for IORT Rev E.d spec

This one could also be split into 2 patches.  However, the C code in IortGenerator.c 
does not follow best practices for use of Reserved fields.  Since we do want to support
converting Reserved fields to defined fields over time, it is better to fill the structures
with the EFI_ACPI_RESERVED_X values first using SetMem() and then assign values to the
defined fields.  Because Reserved fields are being initialized by name, it does take 
an extra patch fix address that issue first:

* Patch #1 to update IortGenerator.c to remove direct init of Reserved* fields and use
  SetMem() on the struct.
* Patch #2 to update MdePkg with new .h file content. This one is backwards compatible
  It adds new structs and converts Reserved fields to defined fields.
* Patch #3 to update IortGenerator.c to use the new fields



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