Re: [edk2-devel] [edk2-stable202311][Patch 1/1] BaseTools/Scripts: Handle reviewer only case in GetMaintainer.py

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

Hi Leif,

Agree with your points.  I was trying to make minimal changes to address
the reviewers with no maintainers case.  Returning a dictionary would make
more sense.

A couple questions:

1) Do you want to see this patch broken up into a series, with the
   logic fix, reviewers with no maintainers feature, and code clean
   up in separate patches?

2) Is this change approved for edk2-stable202311?

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif
> Lindholm
> Sent: Friday, November 10, 2023 4:44 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Rebecca Cran <rebecca@bsdio.com>; Gao,
> Liming <gaoliming@byosoft.com.cn>; Feng, Bob C <bob.c.feng@intel.com>;
> Chen, Christine <yuwei.chen@intel.com>
> Subject: Re: [edk2-devel] [edk2-stable202311][Patch 1/1]
> BaseTools/Scripts: Handle reviewer only case in GetMaintainer.py
> 
> On Wed, Nov 08, 2023 at 12:43:23 -0800, Michael D Kinney wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4593
> >
> > If a package only has reviewers and no maintainers, then also
> > return the <default> maintainers.
> >
> > Update get_maintainers() to return maintainers, reviews, and
> > lists separately instead of a single merged list to allow this
> > module to be used by other scripts and distinguish types.
> >
> > Sort the list of output addresses alphabetically.
> >
> > Fix logic bug where maintainers was incorrectly added to lists.
> >
> > 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: Leif Lindholm <quic_llindhol@quicinc.com>
> > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> > ---
> >  BaseTools/Scripts/GetMaintainer.py | 42 ++++++++++++++++++---------
> ---
> >  1 file changed, 26 insertions(+), 16 deletions(-)
> >
> > diff --git a/BaseTools/Scripts/GetMaintainer.py
> b/BaseTools/Scripts/GetMaintainer.py
> > index d1e042c0afe4..b33546b10f21 100644
> > --- a/BaseTools/Scripts/GetMaintainer.py
> > +++ b/BaseTools/Scripts/GetMaintainer.py
> > @@ -76,6 +76,7 @@ def get_section_maintainers(path, section):
> >      """Returns a list with email addresses to any M: and R: entries
> >         matching the provided path in the provided section."""
> >      maintainers = []
> > +    reviewers = []
> >      lists = []
> >      nowarn_status = ['Supported', 'Maintained']
> >
> > @@ -83,12 +84,18 @@ def get_section_maintainers(path, section):
> >          for status in section['status']:
> >              if status not in nowarn_status:
> >                  print('WARNING: Maintained status for "%s" is
> \'%s\'!' % (path, status))
> > -        for address in section['maintainer'], section['reviewer']:
> > +        for address in section['maintainer']:
> >              # Convert to list if necessary
> >              if isinstance(address, list):
> >                  maintainers += address
> >              else:
> > -                lists += [address]
> > +                maintainers += [address]
> 
> That's a bugfix. Ought to be separate.
> (Cleverly hidden by concatentaing the results when we didn't care
> about keeping them separate other than for seeing if we'd found any
> humans.)
> 
> > +        for address in section['reviewer']:
> > +            # Convert to list if necessary
> > +            if isinstance(address, list):
> > +                reviewers += address
> > +            else:
> > +                reviewers += [address]
> >          for address in section['list']:
> >              # Convert to list if necessary
> >              if isinstance(address, list):
> > @@ -96,32 +103,34 @@ def get_section_maintainers(path, section):
> >              else:
> >                  lists += [address]
> >
> > -    return maintainers, lists
> > +    return maintainers, reviewers, lists
> >
> >  def get_maintainers(path, sections, level=0):
> >      """For 'path', iterates over all sections, returning
> maintainers
> >         for matching ones."""
> >      maintainers = []
> > +    reviewers = []
> >      lists = []
> >      for section in sections:
> > -        tmp_maint, tmp_lists = get_section_maintainers(path,
> section)
> > -        if tmp_maint:
> > -            maintainers += tmp_maint
> > -        if tmp_lists:
> > -            lists += tmp_lists
> > +        tmp_maint, tmp_review, tmp_lists =
> get_section_maintainers(path, section)
> > +        maintainers += tmp_maint
> > +        reviewers += tmp_review
> > +        lists += tmp_lists
> 
> Minor niggle at coding style cleanup as part of functional rework.
> 
> >
> >      if not maintainers:
> >          # If no match found, look for match for (nonexistent) file
> >          # REPO.working_dir/<default>
> >          print('"%s": no maintainers found, looking for default' %
> path)
> >          if level == 0:
> > -            maintainers = get_maintainers('<default>', sections,
> level=level + 1)
> > +            maintainers, tmp_review, tmp_lists =
> get_maintainers('<default>', sections, level=level + 1)
> > +            reviewers += tmp_review
> > +            lists += tmp_lists
> >          else:
> >              print("No <default> maintainers set for project.")
> >          if not maintainers:
> >              return None
> >
> > -    return maintainers + lists
> 
> Apart from the niggles mentioned above, I agree that this is a
> reasonable way of adding the required functionality without completely
> rewriting the existing code. (It does make me feel there must be a
> better way of writing it than I did, though.)
> 
> > +    return maintainers, reviewers, lists
> >
> >  def parse_maintainers_line(line):
> >      """Parse one line of Maintainers.txt, returning any match group
> and its key."""
> > @@ -182,15 +191,16 @@ if __name__ == '__main__':
> >      else:
> >          FILES = get_modified_files(REPO, ARGS)
> >
> > -    ADDRESSES = []
> > -
> > +    # Accumulate a sorted list of addresses
> > +    ADDRESSES = set([])
> >      for file in FILES:
> >          print(file)
> > -        addresslist = get_maintainers(file, SECTIONS)
> > -        if addresslist:
> > -            ADDRESSES += addresslist
> > +        maintainers, reviewers, lists = get_maintainers(file,
> SECTIONS)
> > +        ADDRESSES |= set(maintainers + reviewers + lists)
> > +    ADDRESSES = list(ADDRESSES)
> > +    ADDRESSES.sort()
> >
> > -    for address in list(OrderedDict.fromkeys(ADDRESSES)):
> > +    for address in ADDRESSES:
> 
> But the above doesn't seem to have any impact on the generated output
> at all. So I guess this is to enable the github work to utilise
> get_maintainers() directly while maintaining the separation of
> maintainer/reviewer/list?
> 
> It feels to me like that change would be more clear as a separate
> commit from the one that breaks out reviewers from maintainers.
> I don't have a strong preference for the ordering.
> 
> And it would probably also be less fragile (w.r.t. future edits) if
> the end result returned a dict instead of three lists.
> 
> /
>     Leif
> 
> >          if '<' in address and '>' in address:
> >              address = address.split('>', 1)[0] + '>'
> >          print('  %s' % address)
> > --
> > 2.40.1.windows.1
> >
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111040): https://edk2.groups.io/g/devel/message/111040
Mute This Topic: https://groups.io/mt/102472591/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-