Topics

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


Laszlo Ersek
 

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


Guo Dong
 

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?

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
 

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





Guo Dong
 

Thanks Laszlo for all these information.
It looks it is not well communicated on the merge requirements.
This is the first time for me to merge a patch set and I had thought
it is same with merging a single patch (no cover letter).
As UefiPayloadPkg maintainer, most time I worked on bootloaders.
It would be great if we could have a single page to documents the
rule and steps for maintainers.

And it would be great for maintainers if EDK2 could move to github
code review, and merge from the pull request directly once it passed
the review. We don't need do any extra things for the patch merge.

Thanks,
Guo

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Wednesday, September 16, 2020 11:14 AM
To: Dong, Guo <guo.dong@...>; devel@edk2.groups.io
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]

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
 

On 09/16/20 23:51, Dong, Guo wrote:

Thanks Laszlo for all these information.
It looks it is not well communicated on the merge requirements.
This is the first time for me to merge a patch set and I had thought
it is same with merging a single patch (no cover letter).
As UefiPayloadPkg maintainer, most time I worked on bootloaders.
It would be great if we could have a single page to documents the
rule and steps for maintainers.

And it would be great for maintainers if EDK2 could move to github
code review, and merge from the pull request directly once it passed
the review. We don't need do any extra things for the patch merge.
Moving to github entirely is indeed the end-goal. I don't know the
schedule however.

Note that moving the review activities to github will not per se solve
these problems. People will still have to compose good PR descriptions,
and they will still have to update and cross-reference BZ tickets. (We
are not going to abandon BZ for github.com's issue tracker -- we used
the github.com issue tracker in the past for edk2, and it proved lacking.)

Thanks,
Laszlo



Thanks,
Guo

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Wednesday, September 16, 2020 11:14 AM
To: Dong, Guo <guo.dong@...>; devel@edk2.groups.io
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]

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








Marcello Sylvester Bauer
 

Dear Laszlo,

I'm really sorry for violating the development process. Reading the email thread
that follows made me realize that I have to work on my submission work follow
before creating more patch series.

I'm looking forward to seeing this process moving completely to GitHub. No empty description
of mine will appear in the future.

PS: We are currently working on a CI integration for testing UefiPayloadPkg functionality on real
hardware. It is still WIP but this could be part of the testing process in future.

thanks,
Marcello


On Wed, Sep 16, 2020 at 10:57 AM Laszlo Ersek <lersek@...> wrote:
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








--
[Marcello Sylvester Bauer] 



9elements Agency GmbH, Kortumstraße 19-21, 44787 Bochum, Germany

Sitz der Gesellschaft: Bochum
Handelsregister: Amtsgericht Bochum, HRB 17519
Geschäftsführung: Sebastian Deutsch, Eray Basar


Laszlo Ersek
 

Hi Marcello,

On 09/21/20 11:57, Marcello Sylvester Bauer wrote:
Dear Laszlo,

I'm really sorry for violating the development process.
my criticism was targeted solely at edk2 maintainers, not at
contributors. As far as I'm aware, you did nothing wrong. I kept you
CC'd because it's bad netiquette to drop thread participants mid-thread.

I'm sorry for confusing you.

Thanks
Laszlo