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

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

On 2023-11-10 16:34, Kinney, Michael D wrote:
> 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?

Ideally, yes.
It will be helpful if I need to try to understand these changes again in 
future :)

> 2) Is this change approved for edk2-stable202311?

Yes.

Regards,

Leif

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