Topics

more development process failure [was: UefiPayloadPkg: Runtime MMCONF]


Yao, Jiewen
 

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@...; Dong, Guo
<guo.dong@...>
Cc: marcello.bauer@...; Kinney, Michael D
<michael.d.kinney@...>; 'Leif Lindholm (Nuvia address)'
<leif@...>; Doran, Mark <mark.doran@...>; 'Andrew Fish'
<afish@...>; Guptha, Soumya K <@SoumyaGuptha>
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@...>; devel@edk2.groups.io
抄送: marcello.bauer@...; Kinney, Michael D
<michael.d.kinney@...>; Leif Lindholm (Nuvia address)
<leif@...>; Doran, Mark <mark.doran@...>; Andrew Fish
<afish@...>; Guptha, Soumya K <@SoumyaGuptha>
主题: 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@...>
Cc: devel@edk2.groups.io; marcello.bauer@...; Kinney,
Michael D
<michael.d.kinney@...>; Leif Lindholm (Nuvia address)
<leif@...>; Doran, Mark <mark.doran@...>; Andrew Fish
<afish@...>; Guptha, Soumya K <@SoumyaGuptha>
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













Laszlo Ersek
 

Jiewen,

On 09/17/20 04:31, Yao, Jiewen wrote:
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?
while I agree that the current process is not really simple, I'd like to
point out some things:

- The current complexity exists because we are in a transition period,
and so we get to deal with both the workflow we're leaving (= the
mailing list based review) and the system we're adopting (= github).
This should not last forever. I don't know the exact schedule though.

- I think that lack of attention to detail (on the human side) takes a
relatively large chunk of the blame. The process at the moment is not
simple, but it's exercised every day, every week by some people, so if
somebody *wants*, they can get it right by following examples. Look at
recent patch series threads that have been merged, recent BZs that have
been closed, recent PRs that have been opened and merged.

It's a fallacy that adopting a 100% github.com-native patch review
workflow will solve all of these issues. There is no replacement for
human discipline and attention to detail. In the current process, I
*regularly* find pull requests (personal builds or maintainer push
attempts) on github.com that fail CI (or merging due to conflicts) and
then the submitter never bothers to close or refresh them. I have
cleaned up (closed) a *multitude* of such PRs.

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?
It won't help.

See the following bug report:

https://bugzilla.tianocore.org/show_bug.cgi?id=2963#c0

While it is technically not empty (the string in comment#0 has nonzero
length), it's practically *devoid of information*.

People that are annoyed that they are required to write sensible
summaries for patch sets and bug reports, will do anything and
everything to wiggle out of that requirement. They will create
single-sentence PR descriptions, which will allow them to pass the CI
check. And the community will be *worse off*, because we will have
complicated our CI logic, but the resultant historical records will be
just as useless.

2) Send email - can we let CI send email automatically? Or remind us to send email?
github.com *already* sends an email notification when a PR undergoes a
state change; that is, when it is merged, or else CI fails. The email is
*already* there, people just have to *act* upon it -- run a local "git
pull" again, see what the new commit range is, and send a response to
the original thread.

3) update Bugzilla - can we let CI update Bugzilla automatically? Or remind us to update bugzilla?
Automatically closing tickets is not implemented between github.com and
Bugzilla. It is implemented within github.com (merging a PR can
auto-close issue tracker tickets, if you format the commit message
corectly).

However, auto-closing is *wrong*. It occurs that multiple patch series
relate to a single ticket. In such cases, it's possible that 10+ patches
are merged for a single ticket, and the ticket should *still* not be
closed, because more patches (for the same ticket) are necessary. Only a
human can tell when the fix or the feature is considered complete
(according to their knowledge at that point in time).

4) Unicode char - can we add check in patchchecker, to reject predefined format violation?
There are many-many classes of unicode code points. It's not easy to
express "accept U+003A for punctuation, but do not accept U+FF1A".

It's easy to express "accept 7-bit ASCII only", but I think some people
might take issue with that, because then their names could not be placed
in subject lines in native form.


I know the new tool/CI cannot be built in one day. And we do improvement step by step.
The *real* problem is with the attitude. If a developer cares *only*
until their patches are merged, then no tooling will *ever* fix this
issue. People need to start caring about the afterlife of their work.
When you throw a party, or join one, you stay around for the cleanup
afterwards, don't you?

When you call a contractor to fix something in or around the house, do
you expect them to clean up when they're done, or are you happy cleaning
after them?


The exact same bad attitude is the reason that

- we have botched error paths in programming languages like C,

- we have programming languages and libraries that attempt (and *fail*)
to clean up resources on errors, "on behalf" of the programmer -- I'm
referring to exceptions and destructors in C++, for example.

Both of these are symptoms that people *refuse* to deal with the
"boring" aspects of the job.


Just accept that the party isn't finished until the house and the garden
are tidied up, and the furniture is restored to original order.

Laszlo


Yao, Jiewen
 

Laszlo
I like your description to compare the process with the programming language and software design. We have to clean up the resource.

Please allow me to point out, this is the exactly the issue we are having today.

1) Take buffer overflow as an example. It has been there for 30 years. We have enough books and papers talking about it. Today, people are still making mistakes. Why? Because C language is not type safe, there is NO enforcement.
That is why many people like other type safe language. If you are trying to make mistake, the compiler will tell you that you are making mistake.

2) Take resource leak as an example. The programming language invented garbage collection. The operating system auto cleaned up application resource after execution.

3) People has wrong check in which may break the system. What is why the world invented smoke test and continuous integration.

*Those are where the inventions come*, to treat human as human being, to prevent people making mistake or teach them automatically. :-)

I agree with the "mythical man-month" that there is no silver bulletin.
I tend to agree with you on the attitude.
I am trying to figure if we can do better to help other people (maintainer or contributor). If we really really can do nothing, that is OK.
I am not sure if is a best way to resolve the problem to just complain in the email.

I think you can understand my point. I will stop here.

Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Thursday, September 17, 2020 2:32 PM
To: Yao, Jiewen <jiewen.yao@...>; devel@edk2.groups.io;
gaoliming@...; Dong, Guo <guo.dong@...>
Cc: marcello.bauer@...; Kinney, Michael D
<michael.d.kinney@...>; 'Leif Lindholm (Nuvia address)'
<leif@...>; Doran, Mark <mark.doran@...>; 'Andrew Fish'
<afish@...>; Guptha, Soumya K <@SoumyaGuptha>
Subject: Re: [edk2-devel] more development process failure [was:
UefiPayloadPkg: Runtime MMCONF]

Jiewen,

On 09/17/20 04:31, Yao, Jiewen wrote:
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?
while I agree that the current process is not really simple, I'd like to
point out some things:

- The current complexity exists because we are in a transition period,
and so we get to deal with both the workflow we're leaving (= the
mailing list based review) and the system we're adopting (= github).
This should not last forever. I don't know the exact schedule though.

- I think that lack of attention to detail (on the human side) takes a
relatively large chunk of the blame. The process at the moment is not
simple, but it's exercised every day, every week by some people, so if
somebody *wants*, they can get it right by following examples. Look at
recent patch series threads that have been merged, recent BZs that have
been closed, recent PRs that have been opened and merged.

It's a fallacy that adopting a 100% github.com-native patch review
workflow will solve all of these issues. There is no replacement for
human discipline and attention to detail. In the current process, I
*regularly* find pull requests (personal builds or maintainer push
attempts) on github.com that fail CI (or merging due to conflicts) and
then the submitter never bothers to close or refresh them. I have
cleaned up (closed) a *multitude* of such PRs.

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?
It won't help.

See the following bug report:

https://bugzilla.tianocore.org/show_bug.cgi?id=2963#c0

While it is technically not empty (the string in comment#0 has nonzero
length), it's practically *devoid of information*.

People that are annoyed that they are required to write sensible
summaries for patch sets and bug reports, will do anything and
everything to wiggle out of that requirement. They will create
single-sentence PR descriptions, which will allow them to pass the CI
check. And the community will be *worse off*, because we will have
complicated our CI logic, but the resultant historical records will be
just as useless.

2) Send email - can we let CI send email automatically? Or remind us to send
email?

github.com *already* sends an email notification when a PR undergoes a
state change; that is, when it is merged, or else CI fails. The email is
*already* there, people just have to *act* upon it -- run a local "git
pull" again, see what the new commit range is, and send a response to
the original thread.

3) update Bugzilla - can we let CI update Bugzilla automatically? Or remind us
to update bugzilla?

Automatically closing tickets is not implemented between github.com and
Bugzilla. It is implemented within github.com (merging a PR can
auto-close issue tracker tickets, if you format the commit message
corectly).

However, auto-closing is *wrong*. It occurs that multiple patch series
relate to a single ticket. In such cases, it's possible that 10+ patches
are merged for a single ticket, and the ticket should *still* not be
closed, because more patches (for the same ticket) are necessary. Only a
human can tell when the fix or the feature is considered complete
(according to their knowledge at that point in time).

4) Unicode char - can we add check in patchchecker, to reject predefined
format violation?

There are many-many classes of unicode code points. It's not easy to
express "accept U+003A for punctuation, but do not accept U+FF1A".

It's easy to express "accept 7-bit ASCII only", but I think some people
might take issue with that, because then their names could not be placed
in subject lines in native form.


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

The *real* problem is with the attitude. If a developer cares *only*
until their patches are merged, then no tooling will *ever* fix this
issue. People need to start caring about the afterlife of their work.
When you throw a party, or join one, you stay around for the cleanup
afterwards, don't you?

When you call a contractor to fix something in or around the house, do
you expect them to clean up when they're done, or are you happy cleaning
after them?


The exact same bad attitude is the reason that

- we have botched error paths in programming languages like C,

- we have programming languages and libraries that attempt (and *fail*)
to clean up resources on errors, "on behalf" of the programmer -- I'm
referring to exceptions and destructors in C++, for example.

Both of these are symptoms that people *refuse* to deal with the
"boring" aspects of the job.


Just accept that the party isn't finished until the house and the garden
are tidied up, and the furniture is restored to original order.

Laszlo





Laszlo Ersek
 

On 09/17/20 09:31, Yao, Jiewen wrote:
Laszlo
I like your description to compare the process with the programming language and software design. We have to clean up the resource.

Please allow me to point out, this is the exactly the issue we are having today.

1) Take buffer overflow as an example. It has been there for 30 years. We have enough books and papers talking about it. Today, people are still making mistakes. Why? Because C language is not type safe, there is NO enforcement.
That is why many people like other type safe language. If you are trying to make mistake, the compiler will tell you that you are making mistake.
This will sound more convincing once we have Rust (or something similar)
in a mainstream OS kernel or mainstream firmware.

2) Take resource leak as an example. The programming language invented garbage collection. The operating system auto cleaned up application resource after execution.
Same comment as above. I think garbage collection is frequently
considered too opaque for low-level applications (unpredictable
performance and RAM penalties etc).

3) People has wrong check in which may break the system. What is why the world invented smoke test and continuous integration.
Yes, I agree.

*Those are where the inventions come*, to treat human as human being, to prevent people making mistake or teach them automatically. :-)
Thanks, I'm grateful that you use the word "teach" here. I'll reference
it below.

I agree with the "mythical man-month" that there is no silver bulletin.
I tend to agree with you on the attitude.
I am trying to figure if we can do better to help other people (maintainer or contributor). If we really really can do nothing, that is OK.
I am not sure if is a best way to resolve the problem to just complain in the email.
"Just complaining in email" is in fact my attempt to "teach" people. Not
automatically, but by pointing out examples that I consider good.

Automatisms are already in place for broadcasting good practices.
Bugzilla actions and github actions are propagated via email. People
just need to be receptive and look at the list traffic.

I've contributed to Wiki articles. But asking for more documentation and
more automatisms is just too convenient an excuse. People can and
*should* learn by example, especially if they're seasoned in a project
(not newcomers). Asking for more documentation and more automatisms puts
the burden on people *different* from those that need to improve their
discipline.

It basically means, "I refuse to improve my behavior until *you* find
the time to implement the tools and documentations for me to improve my
behavior". Similarly, "I refuse to handle errors until you give me
exceptions and destructors".

I don't think it's fair to *demand* inventions. Because, I perceive the
goals that I'm advocating for as widely-held values. If we accept them
as such, then the burden should be on people that break those values,
not on the advocates.

(If, on the other hand, we do not have a consensus that these values are
universal, that's OK as well: then I can start ignoring the information
content in the PRs that *I* submit, and save myself significant amounts
of time. See again: double standards.)

I think you can understand my point. I will stop here.
Yes, I understand. My general response is that inventions are nice and
they should be utilized if someone comes forward and offers them. On the
other hand, demanding inventions (tooling, documentation etc), i.e.,
demanding that the *other* party spend the effort first, as a condition
for changing our attitude, is not fair.

Laszlo


Ni, Ray
 

Laszlo,
I support your idea of having a meaningful description for BZ, for commit message, for code comments.

Thinking from 1 or 2 years from now, the simple message we created may help nothing to remind me or others why the changes were made.

We cannot reply on people's memories and even the people that have the memories may left the community.

So, documentation is necessary.

But I remember that our development process document in WIKI doesn't require anything on the commit message perspective.

IMO, at least the commit message should contain:
* current status or reason(fail, lack of a feature, bad coding style)
* impact of the change or result (fail to pass, feature enabled, coding style improved)

Can someone help to emphasize the requirement in the WIKI?

Thanks,
Ray


Andrew Fish
 

Ray,

Here is my take and I think this is what Laszlo’s comments don’t make clear. I agree with you that the rote process steps should be very clear, but I think what Laszlo is pointing out that best practices and current thinking evolve over time and are hard to capture in words.

So I think our challenge is to step back and write process documentation that make it easy to get started., but also make it easy to get the feedback of what the community think is best practices. I kind of feel right now that our process documents are biased towards people who work on on the edk2 day to day, and that makes it hard to bring on new members.

I think the path forward is making it easy for the developers to take the right steps to get started so the community can give them feed back. This feedback is ultimately the current thinking of the community, but we also need think about how to make it easy to get started. So how do we document the process steps, that make it easy for the community to offer comments on the changes?

One of the ways we solved this kind of problem at Apple, is we make the person we just hired the owner of the getting started guide. I think the goal is how do we make it easy for a new contributor to get he value of the feedback and knowledge of the community of experts.

But I think the solution is not to document everything we do, but to document the path of least resistance to join the community. And that is going to require people not in the community reviewing the documentation. We are too biased and can not do it by our selves.

Thanks,

Andrew Fish

On Sep 17, 2020, at 9:39 PM, Ni, Ray <ray.ni@...> wrote:

Laszlo,
I support your idea of having a meaningful description for BZ, for commit message, for code comments.

Thinking from 1 or 2 years from now, the simple message we created may help nothing to remind me or others why the changes were made.

We cannot reply on people's memories and even the people that have the memories may left the community.

So, documentation is necessary.

But I remember that our development process document in WIKI doesn't require anything on the commit message perspective.

IMO, at least the commit message should contain:
* current status or reason(fail, lack of a feature, bad coding style)
* impact of the change or result (fail to pass, feature enabled, coding style improved)

Can someone help to emphasize the requirement in the WIKI?

Thanks,
Ray





Guo Dong
 

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.

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?
What if there is no cover letter in the patch set in patch #0 summary? For the patch I merged,
I am still not very sure what info I should put there.

I don't know why Laszlo mentioned BZ for my merge since there is no BZ mentioned in the patchset.
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. 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 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.

Thanks,
Guo


Laszlo Ersek
 

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.

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?

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 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.

Thanks
Laszlo


Guo Dong
 

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.

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. Again, you had better ask Mike to add this step
in the process if you think it is required.

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 or know all the things in the community from EDK II
Development Process or from Maintainers.txt. Not sure where you get it.
From my view point, the package "M" only need *care* the package
they maintained following the "M" role defined by the EDK2 documents.
It is good to care other packages if having time, but there is no such
requirement.

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.

Thanks
Laszlo


Laszlo Ersek
 

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