Re: [edk2-devel] more development process failure [was: UefiPayloadPkg: Runtime MMCONF]

["Yao, Jiewen" <jiewen.yao@intel.com>]

Hi Laszlo, Liming, and Mike

I appreciate your effort to setup rule and document this *complex* EDK II Development Process.

I am thinking if we can have a way (a tool) to mandate these process and check if there is any violation. If people makes mistake, he/she knows he/she is making mistake and can correct immediately, instead of letting mistake happens and getting blame later. In such way, we can prevent issue from happening.

We have old maintainer leaving, new maintainers joining. That is the reality. We can have training for everyone. But we are still human. There are many bugs need to be fixed in the code. How can we expect a perfect process that everyone follows strictly without any violation?

If we only have few violation, it is OK to stay with it.
But if we continuously have violation, we need retrospect to ask, *why*? Why there is such a process to cause so many violation?
And can we do better? A simpler process? A better tool?

I also feel sorry that Laszlo need check by his eye on every PR and catch the violation for us. And I also feel sorry to blame some people who is contributing his/her time to help to maintain the code, review the code, check in the code.
We both feel frustrated. We are all coming her to enable new features or fix bugs to make EDKII better.

I would like to ask: Is that technically possible to enhance the CI to catch that earlier, as Laszlo point out:
1) Add patch 0 to PR - can we let CI reject empty description PR?
2) Send email - can we let CI send email automatically? Or remind us to send email?
3) update Bugzilla - can we let CI update Bugzilla automatically? Or remind us to update bugzilla?
4) Unicode char - can we add check in patchchecker, to reject predefined format violation?

I know the new tool/CI cannot be built in one day. And we do improvement step by step.

Thank you
Yao Jiewen


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
> Sent: Thursday, September 17, 2020 9:49 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Dong, Guo
> <guo.dong@intel.com>
> Cc: marcello.bauer@9elements.com; Kinney, Michael D
> <michael.d.kinney@intel.com>; 'Leif Lindholm (Nuvia address)'
> <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; 'Andrew Fish'
> <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com>
> Subject: 回复: [edk2-devel] more development process failure [was:
> UefiPayloadPkg: Runtime MMCONF]
> 
> Guo:
> On pull request, https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-
> Development-Process#the-maintainer-process-for-the-edk-ii-project section 7
> gives the requirement.
> If <new-integration-branch> is a patch series, then copy the patch #0 summary
> into the pull request description.
> 
> Laszlo:
> I think we can enhance wiki page
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-
> Process#the-maintainer-process-for-the-edk-ii-project to add another step to
> reply the patch mail with the merged pull request or commit after PR is merged.
> 
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: bounce+27952+65339+4905953+8761045@groups.io
> > <bounce+27952+65339+4905953+8761045@groups.io> 代表 Laszlo Ersek
> > 发送时间: 2020年9月17日 2:14
> > 收件人: Dong, Guo <guo.dong@intel.com>; devel@edk2.groups.io
> > 抄送: marcello.bauer@9elements.com; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)
> > <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish
> > <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com>
> > 主题: Re: [edk2-devel] more development process failure [was:
> > UefiPayloadPkg: Runtime MMCONF]
> >
> > On 09/16/20 19:30, Dong, Guo wrote:
> > >
> > > Hi Laszlo,
> > >
> > > The patchset includes 3 patches, and all of them had been reviewed by
> > package owners.
> > > The patch submitter has a pull request
> > https://github.com/tianocore/edk2/pull/885, I rebased the patch to latest
> > master, and merged it by adding reviewed-by found from emails.
> > > I also make sure it passed all the checks before I put "push" button there.
> > then retrigger a new build with "push" button.
> > >
> > > I am not sure what is missing. If there is any other requirements, should
> > they be captured during code review or tool check?
> >
> > - The description field of <https://github.com/tianocore/edk2/pull/932/>
> > is empty. It's difficult to tell where the patches come from -- where
> > they were posted and reviewed. A copy of the cover letter should have
> > been included here, plus preferably a link to the v5 mailing list thread
> > (the one that got merged in the end).
> >
> > - It was not confirmed in the v5 mailing list thread that the series had
> > been merged. The confirmation should have included at least one of: (a)
> > the github PR link, (b) the git commit range. (Preferably: both.)
> >
> > It's not the eventual git commits that I'm complaining about, but the
> > lack of communication with the community, and the lack of record for
> > posterity.
> >
> > Myself, I used to consider github PRs a means merely for replacing our
> > earlier direct "git push" commands -- with a CI build + mergify. So, as
> > a maintainer, I would myself queue up several patch sets in a single
> > "batch" PR, add some links to BZs and the mailing list, and let it fly.
> > But then Mike told me this was really wrong, and we should clearly
> > associate any given PR with a specific patch set on the list.
> >
> > This meant an *immense* workload increase for me, in particular because
> > I tend to merge patch sets for *other* people and subsystems too (after
> > they pass review), that is, for such subsystems that I do not
> > co-maintain. In particular during the feature freeze periods.
> >
> > So what really rubs me the wrong way is that, if I am expected to keep
> > all of this meta-data nice and tidy, why aren't some other maintainers?
> > It's a double standard.
> >
> > I can live with either *all of us* ignoring PR tidiness, or *all of us*
> > doing our best to keep everything nicely cross-referenced.
> >
> > But right now I spend significant time and effort on keeping
> > communication and records complete and clean in *all three of* bugzilla,
> > github, and mailing list, whereas a good subset of the maintainers
> > couldn't care less in *either* of those communication channels.
> >
> > For your reference, here's a random PR I submitted and merged for others:
> >
> >   https://github.com/tianocore/edk2/pull/904
> >
> > Observe in PR#904:
> >
> > - title carries cover letter subject
> > - description carries cover letter body
> > - description has a pointer to the BZ, and a link to the cover letter in
> > the mailing list archive (two links in fact, in different archives)
> >
> > And then here's my report back on the list:
> >
> >   https://edk2.groups.io/g/devel/message/64644
> >
> > And my BZ comment to the same effect (also closing the BZ as
> > RESOLVED|FIXED):
> >
> >   https://bugzilla.tianocore.org/show_bug.cgi?id=2376#c9
> >   https://edk2.groups.io/g/bugs/message/12777
> >
> >
> > I don't insist on the particular information content of github PRs, as
> > -- at this stage -- they really are not more than just a way to set off
> > CI, before pushing/merging a series.
> >
> > What I do insist on is that all of us maintainers (people with
> > permission to set the "push" label) be subject to the same expectations
> > when it comes to creating pull requests.
> >
> > (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.)
> >
> > Laszlo
> >
> >
> > >
> > > Thanks,
> > > Guo
> > >
> > >> -----Original Message-----
> > >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> > >> Ersek
> > >> Sent: Wednesday, September 16, 2020 1:57 AM
> > >> To: Dong, Guo <guo.dong@intel.com>
> > >> Cc: devel@edk2.groups.io; marcello.bauer@9elements.com; Kinney,
> > Michael D
> > >> <michael.d.kinney@intel.com>; Leif Lindholm (Nuvia address)
> > >> <leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; Andrew Fish
> > >> <afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com>
> > >> Subject: [edk2-devel] more development process failure [was:
> > UefiPayloadPkg:
> > >> Runtime MMCONF]
> > >>
> > >> Guo,
> > >>
> > >> On 08/18/20 10:24, Marcello Sylvester Bauer wrote:
> > >>> Support arbitrary platforms with different or even no MMCONF space.
> > >>> Fixes crash on platforms not exposing 256 buses.
> > >>>
> > >>> Tested on:
> > >>> * AMD Stoney Ridge
> > >>>
> > >>> Branch: https://github.com/9elements/edk2-1/tree/UefiPayloadPkg-
> > >> MMCONF
> > >>> PR: https://github.com/tianocore/edk2/pull/885
> > >>>
> > >>> v5:
> > >>> * MdePkg
> > >>>   - support variable size MMCONF in all PciExpressLibs
> > >>>   - use (UINTX)-1 as return values for invalid Pci addresses
> > >>
> > >> Okay, so we got more of the same development process violations here, as
> > >> I've just reported at <https://edk2.groups.io/g/devel/message/65313>.
> > >>
> > >> See this new pull request:
> > >>
> > >>   https://github.com/tianocore/edk2/pull/932/
> > >>
> > >> "No description provided."
> > >>
> > >> You should be embarrassed.
> > >>
> > >> Laszlo
> > >>
> > >>
> > >>
> > >>
> > >>
> > >
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 
> 
>