From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id A1091AC1187 for ; Fri, 10 Nov 2023 12:44:09 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=gypzg7xEDblZDNO6eCvfotV7+GhQDLr4YENmizU666Q=; c=relaxed/simple; d=groups.io; h=Date:From:To:CC:Subject:Message-ID:References:MIME-Version:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Disposition; s=20140610; t=1699620248; v=1; b=k1jM15fOW9pcnsmoAiwZ+B34WOjmtxpCoIdbacxMRkZdIUUExPIKOQf+FuZup/wQYOUPzDVr r/wyrpQOmSUFqdQ36SknZLwHK6bvPATWEyw8wK9vErObwyXPdfr4CVuoFmG5Z0h4WkR7gSxcun2 0RuZc1hMcan4M/QvuVQ8hCXw= X-Received: by 127.0.0.2 with SMTP id IUgrYY7687511xP6u5uXxnSs; Fri, 10 Nov 2023 04:44:08 -0800 X-Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by mx.groups.io with SMTP id smtpd.web11.26452.1699620245614363111 for ; Fri, 10 Nov 2023 04:44:07 -0800 X-Received: from pps.filterd (m0279872.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3AABxJje032719; Fri, 10 Nov 2023 12:43:56 GMT X-Received: from nasanppmta01.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3u99e91dxf-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 10 Nov 2023 12:43:55 +0000 X-Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA01.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 3AAChsVJ009323 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 10 Nov 2023 12:43:54 GMT X-Received: from qc-i7.hemma.eciton.net (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.39; Fri, 10 Nov 2023 04:43:52 -0800 Date: Fri, 10 Nov 2023 12:43:49 +0000 From: "Leif Lindholm" To: Michael D Kinney CC: , Rebecca Cran , Liming Gao , Bob Feng , Yuwei Chen Subject: Re: [edk2-devel] [edk2-stable202311][Patch 1/1] BaseTools/Scripts: Handle reviewer only case in GetMaintainer.py Message-ID: References: <20231108204323.1292-1-michael.d.kinney@intel.com> MIME-Version: 1.0 In-Reply-To: <20231108204323.1292-1-michael.d.kinney@intel.com> X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-GUID: NvdjDQG4nXs8ygRAiSvMYG4lGf5vFgyh X-Proofpoint-ORIG-GUID: NvdjDQG4nXs8ygRAiSvMYG4lGf5vFgyh Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,quic_llindhol@quicinc.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: HSJUbJu2vy1pcyPSbu6lhUCtx7686176AA= Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=k1jM15fO; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=quicinc.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 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 > Cc: Liming Gao > Cc: Bob Feng > Cc: Yuwei Chen > Cc: Leif Lindholm > Signed-off-by: Michael D Kinney > --- > 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/ > print('"%s": no maintainers found, looking for default' % path) > if level == 0: > - maintainers = get_maintainers('', sections, level=level + 1) > + maintainers, tmp_review, tmp_lists = get_maintainers('', sections, level=level + 1) > + reviewers += tmp_review > + lists += tmp_lists > else: > print("No 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] -=-=-=-=-=-=-=-=-=-=-=-