From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web10.19229.1601380802932656203 for ; Tue, 29 Sep 2020 05:00:03 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ByDckabo; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) Dkim-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1601380802; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=jtOQ5U3ZMYUuu6oN+amqVQplcdQSvOfcsLi+WBdEn7o=; b=ByDckabotfQrbEY0P2SSBm+TkI3w+Co3Ug6u83DIbi8d88ekayoR72+cL2dGl6YxJBTefW K5w1bcB3HsgtZrU9tL3/PNArjvODENT8u4NZ93KoxFbgVfQ2OuWo55GtVOcwkzEu4CqiVK D2t2VCm16TrmazT3FmufxWP5uBBPFLw= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-569-Uaz93poyMRaD-7sycR7VpQ-1; Tue, 29 Sep 2020 07:59:58 -0400 X-MC-Unique: Uaz93poyMRaD-7sycR7VpQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A0D4880EDA1; Tue, 29 Sep 2020 11:59:55 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-40.ams2.redhat.com [10.36.113.40]) by smtp.corp.redhat.com (Postfix) with ESMTP id E60565C1BD; Tue, 29 Sep 2020 11:59:52 +0000 (UTC) Subject: Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF] To: devel@edk2.groups.io, guo.dong@intel.com, Andrew Fish , "Ni, Ray" , "Kinney, Michael D" Cc: "Yao, Jiewen" , "gaoliming@byosoft.com.cn" , "marcello.bauer@9elements.com" , "Leif Lindholm (Nuvia address)" , "Doran, Mark" , "Guptha, Soumya K" References: <20200818082421.6168-1-marcello.bauer@9elements.com> <11b4d671-7c5e-0ef3-0d2f-13ef605f1eaf@redhat.com> <000e01d68c94$bb92d920$32b88b60$@byosoft.com.cn> <31e807dc-6217-f3b6-995b-ab10f4ce789e@redhat.com> <98fd8c76-0815-88b9-8d63-f2092efc0997@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 29 Sep 2020 13:59:52 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/29/20 06:13, Guo Dong wrote: > >> On 09/26/20 02:34, Dong, Guo wrote: >>> >>> Sorry to have a long email thread since my merge and thanks all for >>> the comments. >>> In general, I still feel current process is a little complicated for >>> the maintainers who don't daily work on EDK2 like me. I have less >>> than %5 of time spent on open source EDK2 UefiPayloadPkg since I focus >>> on bootloaders. It would be great if I could spend the time mainly on >>> code review instead of the process as of now. >> >> I think this is a 100% reasonable request; it would mean that your "M" >> role should be replaced with an "R" role under "UefiPayloadPkg", and >> then your co-maintainers (Maurice and Benjamin) would be responsible for >> pushing the patches that you review. >> >>> >>> Even after I read >>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development- >> Process#the-maintainer-process-for-the-edk-ii-project >>> as Liming pointed out, Some info is still not clear for me. E.g. >>> what's the purpose for putting cover letter to patch set pull request >>> (it looks we could not trace to this PR from code)? is it mandatory or >>> optional? >> >> There are two questions to consider here, actually. >> >> I do not insist that the PRs have sensible descriptions, at this stage. >> >> However, if *some* maintainers are expected to populate the PRs with >> sensible descriptions, then *all* of them should. >> >> So the reason I'm asking you to add sensible descriptions to PRs, at >> this stage, is not because I'm 100% committed to duplicating information >> there. Instead, the reason is that Mike has asked me to do so, and >> therefore you (and everyone else) should do so as well. >> > From the EDK II Development Process Mike edited, there is no such > requirement to add description for the patches that have no BZ. > I think you should ask Mike to update the process so that every > Maintainer could follow the same steps if Mike ever asked you to > do it. It doesn't make sense to blame others using this kind of > "hidden rule" if it is "really" required. Fine. Mike: can you please include the PR subject and description requirements to the "EDK II Development Process" article in the Wiki? Thank you. >> Alternatively (a perfectly valid alternative), we should remove this PR >> description requirement for everybody. That works too. >> >>> What if there is no cover letter in the patch set in patch #0 summary? >> >> That's generally (not always though!) a bad sign in itself. Either the >> cover letter of the patch set, or the bugzilla report, should contain a >> good, relatively high-level description of the issue (or feature), and >> the changes implemented to address it. At this point, *that* description >> should be copied into the PR. >> >> If *neither* the BZ ticket *nor* the patch series cover letter contains >> this kind of summary / overview, then *that* is a big problem, and >> should be remedied. >> >>> For the patch I merged, >>> I am still not very sure what info I should put there. >> >> The cover letter >> >> [edk2-devel] [PATCH v5 0/3] UefiPayloadPkg: Runtime MMCONF >> >> seems to say that the patch set adds support for "arbitrary platforms >> with different or even no MMCONF space" to UefiPayloadPkg. Additionally, >> it fixes a crash on platforms not exposing 256 buses. >> >> That's the info. >> >>> >>> I don't know why Laszlo mentioned BZ for my merge since there is no BZ >>> mentioned in the patchset. >> >> I finished with the following paragraph: >> >> "(Please note also that I absolutely don't need a BZ for every >> contribution. My request is only that *if* there is a BZ, then handle it >> thoroughly.)" >> >> I discussed BZs in general. >> >>> And I also don't know why Laszlo mentioned to send email after the >>> patch is merged since I don't find this requirement in the development >>> process. >> >> How else is a contributor supposed learn of their patch series being >> merged? >> >> Are they supposed to pull the master twice daily, and hope that their >> patches show up eventually? >> >> I mean, the patches you merge originate from the list. Where else is the >> best place to report back to the submitter (and to the rest of the >> community) than under the original patch thread? >> > As I replied to Liming, the EDK II Development Process mentioned this " > Email notifications for pull requests, pushes, and check status results > are enabled by watching the EDK II repository (https://github.com/ > tianocore/edk2). " So that you could get email notification if these > status if you want. Counter-arguments: (1) The email that github itself generates about having merged a PR is pure trash. For example, it does not mention the new commit range that is the result of the merge. Consequently, it does not help people when they try to figure out later what patches they need to backport or evaluate for their downstream products. (2) The email that github sends is not threaded together with the original posting / patch discussion on the list. There is no connection between them. This is why it would be so important to copy sensible information (such as mailing list archive URLs) into the PR description, so that we have some sort of connection between the PR and the patch series thread. > Again, you had better ask Mike to add this step > in the process if you think it is required. OK. Mike, can you please document in the same Wiki article that maintainers should follow up with a final message in the patch series thread, once the PR is merged? Thanks. > >>> I don't think it is doable to ask all the maintainers to monitor EDK2 >>> mail list on how others are doing since there are so many emails every >>> day, especially there is no any patch for UefiPayloadPkg for several >>> months. >> >> I strongly disagree. If you are listed as a maintainer, that implies you >> *care* what happens in the community. If you don't (or cannot) care >> about workflow, then the "M" role is not a good fit for you. >> > I don't think we have requirements to ask maintainers to read all EDK2 > emails Agreed. > or know all the things in the community from EDK II > Development Process or from Maintainers.txt. I disagree. As a maintainer (a person with push access to the master branch, where your acts will affect *every* edk2 consumer), you are responsible for keeping an eye on workflow-related discussions. > Not sure where you get it. It's obvious. > From my view point, the package "M" only need *care* the package > they maintained following the "M" role defined by the EDK2 documents. The master branch (the git history) of the project is a shared resource. If UefiPayloadPkg were maintained in a different repository, you could follow whatever workflow you liked. "pushing package changes to source control" in Maintainers.txt means that a maintainer doesn't live in a bubble. You as a maintainer modify the master branch and thereby impact every edk2 consumer. And it's not merely a "functional impact"; it means things like "how readable and informative is this commit message to others -- including those that are not (yet) UefiPayloadPkg stake-holders". > It is good to care other packages if having time, but there is no such > requirement. The point is not to watch other packages; the point is to watch the workflow-related discussions. > >>> I hope we could simplify the process and have a clear steps in the >>> process soon. So that the maintainers could focus on the actual code >>> review. >> >> Please see "Maintainers.txt": >> >> M: Package Maintainer: Cc address for patches and questions. Responsible >> for reviewing and pushing package changes to source control. >> >> If you are a Maintainer, that means you are responsible for pushing >> changes, and for parts of the workflow that come with that. It's a >> service to the community. If you don't care about that, then the "R" >> role is more appropriate: >> >> R: Package Reviewer: Cc address for patches and questions. Reviewers help >> maintainers review code, but don't have push access. A designated Package >> Reviewer is reasonably familiar with the Package (or some modules >> thereof), and/or provides testing or regression testing for the Package >> (or some modules thereof), in certain platforms and environments. >> > That's what I am doing now to review and merge the patch following these edk2 > defined requirements. I don't understand. You are responding to the "R" role description with "that's what I'm doing", and then you say "review and merge". That doesn't add up. Merging does *not* belong to the "R" role. Merging belongs to the "M" role, but the "M" role requires you to pay more attention to the list than you seem to prefer to. Laszlo