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

["Leif Lindholm" <quic_llindhol@quicinc.com>]

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 (#111036): https://edk2.groups.io/g/devel/message/111036
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]
-=-=-=-=-=-=-=-=-=-=-=-