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 6ABD4AC1477 for ; Fri, 10 Nov 2023 16:36:19 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=MzL2FFk51RoIabKWeaSbw+CJ8RlKI19h1ABotZ1/xKo=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:CC:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1699634178; v=1; b=pz5/I+drJRdkbmvgHPjmCCXMeQtZK+8YmLt3MHBevmNHvSXFcoc6UsBIN9r6xh5a7VPSoGGX R34NrCexOrZePykLSgIao50Csvpwy905YWi1MvzUQ+wkNi30ptKvjDiNNTL/uyJmm1pMqoC9CMQ +M8w7V/pssQ6mEh+f4QOdFKc= X-Received: by 127.0.0.2 with SMTP id J6yYYY7687511xvUXBINbK5m; Fri, 10 Nov 2023 08:36:18 -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.32282.1699634177148836705 for ; Fri, 10 Nov 2023 08:36:17 -0800 X-Received: from pps.filterd (m0279873.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3AAEtcvH028172; Fri, 10 Nov 2023 16:36:08 GMT X-Received: from nasanppmta04.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3u9pq8g80f-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 10 Nov 2023 16:36:07 +0000 X-Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA04.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 3AAGa69r002481 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 10 Nov 2023 16:36:06 GMT X-Received: from [10.111.128.58] (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 08:36:04 -0800 Message-ID: <30ea71eb-b118-4e0b-b946-120c22b268c2@quicinc.com> Date: Fri, 10 Nov 2023 16:35:58 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [edk2-stable202311][Patch 1/1] BaseTools/Scripts: Handle reviewer only case in GetMaintainer.py To: "Kinney, Michael D" , "devel@edk2.groups.io" CC: Rebecca Cran , "Gao, Liming" , "Feng, Bob C" , "Chen, Christine" References: <20231108204323.1292-1-michael.d.kinney@intel.com> From: "Leif Lindholm" In-Reply-To: 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-ORIG-GUID: Ho7jPqKjY3AEAHF78hxNj636Nc9HJAfD X-Proofpoint-GUID: Ho7jPqKjY3AEAHF78hxNj636Nc9HJAfD 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: 0rwOtf6eAAAcmN8WB4rIqnm8x7686176AA= Content-Language: en-GB Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="pz5/I+dr"; 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 2023-11-10 16:34, Kinney, Michael D wrote: > Hi Leif, >=20 > Agree with your points. I was trying to make minimal changes to address > the reviewers with no maintainers case. Returning a dictionary would mak= e > more sense. >=20 > A couple questions: >=20 > 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=20 future :) > 2) Is this change approved for edk2-stable202311? Yes. Regards, Leif > Thanks, >=20 > Mike >=20 >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Leif >> Lindholm >> Sent: Friday, November 10, 2023 4:44 AM >> To: Kinney, Michael D >> Cc: devel@edk2.groups.io; Rebecca Cran ; Gao, >> Liming ; Feng, Bob C ; >> Chen, Christine >> 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=3D4593 >>> >>> 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 =3D [] >>> + reviewers =3D [] >>> lists =3D [] >>> nowarn_status =3D ['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 +=3D address >>> else: >>> - lists +=3D [address] >>> + maintainers +=3D [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 +=3D address >>> + else: >>> + reviewers +=3D [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 +=3D [address] >>> >>> - return maintainers, lists >>> + return maintainers, reviewers, lists >>> >>> def get_maintainers(path, sections, level=3D0): >>> """For 'path', iterates over all sections, returning >> maintainers >>> for matching ones.""" >>> maintainers =3D [] >>> + reviewers =3D [] >>> lists =3D [] >>> for section in sections: >>> - tmp_maint, tmp_lists =3D get_section_maintainers(path, >> section) >>> - if tmp_maint: >>> - maintainers +=3D tmp_maint >>> - if tmp_lists: >>> - lists +=3D tmp_lists >>> + tmp_maint, tmp_review, tmp_lists =3D >> get_section_maintainers(path, section) >>> + maintainers +=3D tmp_maint >>> + reviewers +=3D tmp_review >>> + lists +=3D 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 =3D=3D 0: >>> - maintainers =3D get_maintainers('', sections, >> level=3Dlevel + 1) >>> + maintainers, tmp_review, tmp_lists =3D >> get_maintainers('', sections, level=3Dlevel + 1) >>> + reviewers +=3D tmp_review >>> + lists +=3D 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__ =3D=3D '__main__': >>> else: >>> FILES =3D get_modified_files(REPO, ARGS) >>> >>> - ADDRESSES =3D [] >>> - >>> + # Accumulate a sorted list of addresses >>> + ADDRESSES =3D set([]) >>> for file in FILES: >>> print(file) >>> - addresslist =3D get_maintainers(file, SECTIONS) >>> - if addresslist: >>> - ADDRESSES +=3D addresslist >>> + maintainers, reviewers, lists =3D get_maintainers(file, >> SECTIONS) >>> + ADDRESSES |=3D set(maintainers + reviewers + lists) >>> + ADDRESSES =3D 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 =3D address.split('>', 1)[0] + '>' >>> print(' %s' % address) >>> -- >>> 2.40.1.windows.1 >>> >> >> >>=20 >> >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-