-----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.kin...@intel.com>
Cc: devel@edk2.groups.io; Rebecca Cran <rebe...@bsdio.com>; Gao,
Liming <gaolim...@byosoft.com.cn>; Feng, Bob C <bob.c.f...@intel.com>;
Chen, Christine <yuwei.c...@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 <rebe...@bsdio.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Cc: Bob Feng <bob.c.f...@intel.com>
Cc: Yuwei Chen <yuwei.c...@intel.com>
Cc: Leif Lindholm <quic_llind...@quicinc.com>
Signed-off-by: Michael D Kinney <michael.d.kin...@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
