[PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg

[PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

Attention: Reviewed-by is still needed from some package maintainers.
* 0002-ArmVirtPkg-Add-Platform-CI-and-configuration-for-Cor.patch
  * Laszlo Ersek <lersek@redhat.com>
  * Ard Biesheuvel <ard.biesheuvel@arm.com>
  * Leif Lindholm <leif@nuviainc.com>
* 0003-EmulatorPkg-Add-Platform-CI-and-configuration-for-Co.patch
  * Jordan Justen <jordan.l.justen@intel.com>
  * Andrew Fish <afish@apple.com>
  * Ray Ni <ray.ni@intel.com>
* 0004-OvmfPkg-Add-Platform-CI-and-configuration-for-Core-C.patch
  * Jordan Justen <jordan.l.justen@intel.com>
  * Laszlo Ersek <lersek@redhat.com>
  * Ard Biesheuvel <ard.biesheuvel@arm.com>

The following 7 patches add support for Azure DevOps based "Platform CI"
for ArmVirtPkg, OvmfPkg, and EmulatorPkg.

The patch set also adds ArmVirtPkg, OvmfPkg, and EmulatorPkg to Core CI
for the code evaluation tests (not compiling).

*Note about patch #7*:
On final commit to master, patch 7 will be committed after the builds are
defined for each package. These cannot be defined until patches 1-6 are
committed to master and the builds created in Azure Dev Ops. Once created,
the links in the ReadMe.rst file will be corrected and then patch #7 will
be committed. 

Changes for V3:
* Package Platform CI ReadMe’s have been moved to the PlatformCI folder
  and are in markdown format. Build status is not included in this
  readme but instead all combined in the edk2 readme to bring top level
  visibility to these builds since they will be required to pass for
  PR completion.
* Patch #7 added to convert edk2 readme to RST and add table of platform
  CI status badges.

Changes for V2:
* PlatformBuild.py, iasl dependency, and azurepipeline files have been
  moved into a PlatformCI folder within the respective package. 
* PlatformBuild.py - RequiredSubmodules function configured to read from
  edk2 .gitmodules to lower required updates.
* ReadMe files have been switched to reStructuredText to make Build Status
  table less distracting when viewing plaintext.

Branch: https://github.com/spbrogan/edk2/tree/PlatformAndCoreCIForOvmfArmVirtEmulatorPackages_v10

Please send feedback to the mailing list and do not leave feedback directly
on github.

On a separate note, shallow threading might not work on this patch series
due to changes made by the SMTP server. Please bear with me while I am
investigating if this can be changed.

Cc: Andrew Fish <afish@apple.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Signed-off-by: Sean Brogan <sean.brogan@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Sean Brogan (7):
  .azurepipelines: Add Platform CI template
  ArmVirtPkg: Add Platform CI and configuration for Core CI
  EmulatorPkg: Add Platform CI and configuration for Core CI
  OvmfPkg: Add Platform CI and configuration for Core CI
  .pytool: Update CI Settings to support Emulator, ArmVirt, and Ovmf
    packages
  .azurepipelines: Update Core CI build matrix to include platforms
  ReadMe: Convert to rst and add Platform CI Status

 .azurepipelines/ReadMe.md                                 |  50 +++
 .azurepipelines/templates/ReadMe.md                       |  59 ++++
 .azurepipelines/templates/platform-build-run-steps.yml    | 134 ++++++++
 .azurepipelines/templates/pr-gate-build-job.yml           |   5 +
 .pytool/CISettings.py                                     |   7 +-
 .pytool/Plugin/SpellCheck/cspell.base.yaml                |  14 +
 .pytool/Readme.md                                         |  10 +-
 ArmVirtPkg/ArmVirtPkg.ci.yaml                             | 103 ++++++
 ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml     |  89 +++++
 ArmVirtPkg/PlatformCI/PlatformBuild.py                    | 276 +++++++++++++++
 ArmVirtPkg/PlatformCI/ReadMe.md                           | 125 +++++++
 ArmVirtPkg/PlatformCI/iasl_ext_dep.yaml                   |  21 ++
 EmulatorPkg/EmulatorPkg.ci.yaml                           |  85 +++++
 EmulatorPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml    |  95 ++++++
 EmulatorPkg/PlatformCI/.azurepipelines/Windows-VS2019.yml |  85 +++++
 EmulatorPkg/PlatformCI/PlatformBuild.py                   | 272 +++++++++++++++
 EmulatorPkg/PlatformCI/ReadMe.md                          | 128 +++++++
 OvmfPkg/OvmfPkg.ci.yaml                                   |  83 +++++
 OvmfPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml        | 133 ++++++++
 OvmfPkg/PlatformCI/.azurepipelines/Windows-VS2019.yml     | 138 ++++++++
 OvmfPkg/PlatformCI/PlatformBuild.py                       | 254 ++++++++++++++
 OvmfPkg/PlatformCI/ReadMe.md                              | 137 ++++++++
 OvmfPkg/PlatformCI/iasl_ext_dep.yaml                      |  21 ++
 ReadMe.rst                                                | 354 ++++++++++++++++++++
 Readme.md                                                 | 235 -------------
 25 files changed, 2670 insertions(+), 243 deletions(-)
 create mode 100644 .azurepipelines/ReadMe.md
 create mode 100644 .azurepipelines/templates/ReadMe.md
 create mode 100644 .azurepipelines/templates/platform-build-run-steps.yml
 create mode 100644 ArmVirtPkg/ArmVirtPkg.ci.yaml
 create mode 100644 ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml
 create mode 100644 ArmVirtPkg/PlatformCI/PlatformBuild.py
 create mode 100644 ArmVirtPkg/PlatformCI/ReadMe.md
 create mode 100644 ArmVirtPkg/PlatformCI/iasl_ext_dep.yaml
 create mode 100644 EmulatorPkg/EmulatorPkg.ci.yaml
 create mode 100644 EmulatorPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml
 create mode 100644 EmulatorPkg/PlatformCI/.azurepipelines/Windows-VS2019.yml
 create mode 100644 EmulatorPkg/PlatformCI/PlatformBuild.py
 create mode 100644 EmulatorPkg/PlatformCI/ReadMe.md
 create mode 100644 OvmfPkg/OvmfPkg.ci.yaml
 create mode 100644 OvmfPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml
 create mode 100644 OvmfPkg/PlatformCI/.azurepipelines/Windows-VS2019.yml
 create mode 100644 OvmfPkg/PlatformCI/PlatformBuild.py
 create mode 100644 OvmfPkg/PlatformCI/ReadMe.md
 create mode 100644 OvmfPkg/PlatformCI/iasl_ext_dep.yaml
 create mode 100644 ReadMe.rst
 delete mode 100644 Readme.md

-- 
2.16.3.windows.1

Re: [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg

[Laszlo Ersek]

On 04/24/20 23:31, michael.kubacki@outlook.com wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Attention: Reviewed-by is still needed from some package maintainers.
> * 0002-ArmVirtPkg-Add-Platform-CI-and-configuration-for-Cor.patch
>   * Laszlo Ersek <lersek@redhat.com>
>   * Ard Biesheuvel <ard.biesheuvel@arm.com>
>   * Leif Lindholm <leif@nuviainc.com>

I don't understand. Your posting

[edk2-devel] [PATCH v3 2/7] ArmVirtPkg: Add Platform CI and
configuration for Core CI

already -- and, to my understanding, correctly -- includes

Acked-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

That means both Ard and myself are OK with the patch. What else is
needed from us?

> * 0004-OvmfPkg-Add-Platform-CI-and-configuration-for-Core-C.patch
>   * Jordan Justen <jordan.l.justen@intel.com>
>   * Laszlo Ersek <lersek@redhat.com>
>   * Ard Biesheuvel <ard.biesheuvel@arm.com>

Same here:

[edk2-devel] [PATCH v3 4/7] OvmfPkg: Add Platform CI and configuration
for Core CI

I think these patches are ready to be merged.


Who's going to merge the series?

Thanks!
Laszlo

Re: [edk2-devel] [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg

[Sean]

I think this was my fault.

I was under the impression that a patch needed one of developers listed 
in the (m) or (r) section of maintainers.txt to provide a reviewed-by. 
My new understanding is an ack from the (m) plus anyone providing a 
reviewed-by is enough.

Thanks
Sean



On 4/28/2020 5:59 AM, Laszlo Ersek wrote:
> On 04/24/20 23:31, michael.kubacki@outlook.com wrote:
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Attention: Reviewed-by is still needed from some package maintainers.
>> * 0002-ArmVirtPkg-Add-Platform-CI-and-configuration-for-Cor.patch
>>    * Laszlo Ersek <lersek@redhat.com>
>>    * Ard Biesheuvel <ard.biesheuvel@arm.com>
>>    * Leif Lindholm <leif@nuviainc.com>
> 
> I don't understand. Your posting
> 
> [edk2-devel] [PATCH v3 2/7] ArmVirtPkg: Add Platform CI and
> configuration for Core CI
> 
> already -- and, to my understanding, correctly -- includes
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> That means both Ard and myself are OK with the patch. What else is
> needed from us?
> 
>> * 0004-OvmfPkg-Add-Platform-CI-and-configuration-for-Core-C.patch
>>    * Jordan Justen <jordan.l.justen@intel.com>
>>    * Laszlo Ersek <lersek@redhat.com>
>>    * Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> Same here:
> 
> [edk2-devel] [PATCH v3 4/7] OvmfPkg: Add Platform CI and configuration
> for Core CI
> 
> I think these patches are ready to be merged.
> 
> 
> Who's going to merge the series?
> 
> Thanks!
> Laszlo
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg

[Michael Kubacki]

Given no remaining opens and the approvals given, I went ahead and 
merged the series through patch #6.

Thanks,
Michael

On 4/28/2020 5:59 AM, Laszlo Ersek wrote:
> On 04/24/20 23:31, michael.kubacki@outlook.com wrote:
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Attention: Reviewed-by is still needed from some package maintainers.
>> * 0002-ArmVirtPkg-Add-Platform-CI-and-configuration-for-Cor.patch
>>    * Laszlo Ersek <lersek@redhat.com>
>>    * Ard Biesheuvel <ard.biesheuvel@arm.com>
>>    * Leif Lindholm <leif@nuviainc.com>
> 
> I don't understand. Your posting
> 
> [edk2-devel] [PATCH v3 2/7] ArmVirtPkg: Add Platform CI and
> configuration for Core CI
> 
> already -- and, to my understanding, correctly -- includes
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> That means both Ard and myself are OK with the patch. What else is
> needed from us?
> 
>> * 0004-OvmfPkg-Add-Platform-CI-and-configuration-for-Core-C.patch
>>    * Jordan Justen <jordan.l.justen@intel.com>
>>    * Laszlo Ersek <lersek@redhat.com>
>>    * Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> Same here:
> 
> [edk2-devel] [PATCH v3 4/7] OvmfPkg: Add Platform CI and configuration
> for Core CI
> 
> I think these patches are ready to be merged.
> 
> 
> Who's going to merge the series?
> 
> Thanks!
> Laszlo
> 
> 
> 
> 

Re: [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg

[Michael Kubacki]

Patches #1 - #6 merged as commit range 4fcfd089aa31..099dfbb29d8b via 
https://github.com/tianocore/edk2/pull/553.

As noted in the v3 cover letter, patch #7 merged separately in commit 
64ab457d1f21 via https://github.com/tianocore/edk2/pull/555.

Thanks,
Michael

On 4/24/2020 2:31 PM, michael.kubacki@outlook.com wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Attention: Reviewed-by is still needed from some package maintainers.
> * 0002-ArmVirtPkg-Add-Platform-CI-and-configuration-for-Cor.patch
>    * Laszlo Ersek <lersek@redhat.com>
>    * Ard Biesheuvel <ard.biesheuvel@arm.com>
>    * Leif Lindholm <leif@nuviainc.com>
> * 0003-EmulatorPkg-Add-Platform-CI-and-configuration-for-Co.patch
>    * Jordan Justen <jordan.l.justen@intel.com>
>    * Andrew Fish <afish@apple.com>
>    * Ray Ni <ray.ni@intel.com>
> * 0004-OvmfPkg-Add-Platform-CI-and-configuration-for-Core-C.patch
>    * Jordan Justen <jordan.l.justen@intel.com>
>    * Laszlo Ersek <lersek@redhat.com>
>    * Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> The following 7 patches add support for Azure DevOps based "Platform CI"
> for ArmVirtPkg, OvmfPkg, and EmulatorPkg.
> 
> The patch set also adds ArmVirtPkg, OvmfPkg, and EmulatorPkg to Core CI
> for the code evaluation tests (not compiling).
> 
> *Note about patch #7*:
> On final commit to master, patch 7 will be committed after the builds are
> defined for each package. These cannot be defined until patches 1-6 are
> committed to master and the builds created in Azure Dev Ops. Once created,
> the links in the ReadMe.rst file will be corrected and then patch #7 will
> be committed.
> 
> Changes for V3:
> * Package Platform CI ReadMe’s have been moved to the PlatformCI folder
>    and are in markdown format. Build status is not included in this
>    readme but instead all combined in the edk2 readme to bring top level
>    visibility to these builds since they will be required to pass for
>    PR completion.
> * Patch #7 added to convert edk2 readme to RST and add table of platform
>    CI status badges.
> 
> Changes for V2:
> * PlatformBuild.py, iasl dependency, and azurepipeline files have been
>    moved into a PlatformCI folder within the respective package.
> * PlatformBuild.py - RequiredSubmodules function configured to read from
>    edk2 .gitmodules to lower required updates.
> * ReadMe files have been switched to reStructuredText to make Build Status
>    table less distracting when viewing plaintext.
> 
> Branch: https://github.com/spbrogan/edk2/tree/PlatformAndCoreCIForOvmfArmVirtEmulatorPackages_v10
> 
> Please send feedback to the mailing list and do not leave feedback directly
> on github.
> 
> On a separate note, shallow threading might not work on this patch series
> due to changes made by the SMTP server. Please bear with me while I am
> investigating if this can be changed.
> 
> Cc: Andrew Fish <afish@apple.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Signed-off-by: Sean Brogan <sean.brogan@microsoft.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Sean Brogan (7):
>    .azurepipelines: Add Platform CI template
>    ArmVirtPkg: Add Platform CI and configuration for Core CI
>    EmulatorPkg: Add Platform CI and configuration for Core CI
>    OvmfPkg: Add Platform CI and configuration for Core CI
>    .pytool: Update CI Settings to support Emulator, ArmVirt, and Ovmf
>      packages
>    .azurepipelines: Update Core CI build matrix to include platforms
>    ReadMe: Convert to rst and add Platform CI Status
> 
>   .azurepipelines/ReadMe.md                                 |  50 +++
>   .azurepipelines/templates/ReadMe.md                       |  59 ++++
>   .azurepipelines/templates/platform-build-run-steps.yml    | 134 ++++++++
>   .azurepipelines/templates/pr-gate-build-job.yml           |   5 +
>   .pytool/CISettings.py                                     |   7 +-
>   .pytool/Plugin/SpellCheck/cspell.base.yaml                |  14 +
>   .pytool/Readme.md                                         |  10 +-
>   ArmVirtPkg/ArmVirtPkg.ci.yaml                             | 103 ++++++
>   ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml     |  89 +++++
>   ArmVirtPkg/PlatformCI/PlatformBuild.py                    | 276 +++++++++++++++
>   ArmVirtPkg/PlatformCI/ReadMe.md                           | 125 +++++++
>   ArmVirtPkg/PlatformCI/iasl_ext_dep.yaml                   |  21 ++
>   EmulatorPkg/EmulatorPkg.ci.yaml                           |  85 +++++
>   EmulatorPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml    |  95 ++++++
>   EmulatorPkg/PlatformCI/.azurepipelines/Windows-VS2019.yml |  85 +++++
>   EmulatorPkg/PlatformCI/PlatformBuild.py                   | 272 +++++++++++++++
>   EmulatorPkg/PlatformCI/ReadMe.md                          | 128 +++++++
>   OvmfPkg/OvmfPkg.ci.yaml                                   |  83 +++++
>   OvmfPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml        | 133 ++++++++
>   OvmfPkg/PlatformCI/.azurepipelines/Windows-VS2019.yml     | 138 ++++++++
>   OvmfPkg/PlatformCI/PlatformBuild.py                       | 254 ++++++++++++++
>   OvmfPkg/PlatformCI/ReadMe.md                              | 137 ++++++++
>   OvmfPkg/PlatformCI/iasl_ext_dep.yaml                      |  21 ++
>   ReadMe.rst                                                | 354 ++++++++++++++++++++
>   Readme.md                                                 | 235 -------------
>   25 files changed, 2670 insertions(+), 243 deletions(-)
>   create mode 100644 .azurepipelines/ReadMe.md
>   create mode 100644 .azurepipelines/templates/ReadMe.md
>   create mode 100644 .azurepipelines/templates/platform-build-run-steps.yml
>   create mode 100644 ArmVirtPkg/ArmVirtPkg.ci.yaml
>   create mode 100644 ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml
>   create mode 100644 ArmVirtPkg/PlatformCI/PlatformBuild.py
>   create mode 100644 ArmVirtPkg/PlatformCI/ReadMe.md
>   create mode 100644 ArmVirtPkg/PlatformCI/iasl_ext_dep.yaml
>   create mode 100644 EmulatorPkg/EmulatorPkg.ci.yaml
>   create mode 100644 EmulatorPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml
>   create mode 100644 EmulatorPkg/PlatformCI/.azurepipelines/Windows-VS2019.yml
>   create mode 100644 EmulatorPkg/PlatformCI/PlatformBuild.py
>   create mode 100644 EmulatorPkg/PlatformCI/ReadMe.md
>   create mode 100644 OvmfPkg/OvmfPkg.ci.yaml
>   create mode 100644 OvmfPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml
>   create mode 100644 OvmfPkg/PlatformCI/.azurepipelines/Windows-VS2019.yml
>   create mode 100644 OvmfPkg/PlatformCI/PlatformBuild.py
>   create mode 100644 OvmfPkg/PlatformCI/ReadMe.md
>   create mode 100644 OvmfPkg/PlatformCI/iasl_ext_dep.yaml
>   create mode 100644 ReadMe.rst
>   delete mode 100644 Readme.md
> 

Re: [edk2-devel] [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg

[Laszlo Ersek]

On 04/28/20 18:35, Sean Brogan wrote:
> I think this was my fault.
> 
> I was under the impression that a patch needed one of developers listed
> in the (m) or (r) section of maintainers.txt to provide a reviewed-by.
> My new understanding is an ack from the (m) plus anyone providing a
> reviewed-by is enough.

It depends on the maintainer, too.

Personally I give R-b if I carefully review the patch and am pleased
with it.

I give A-b if I review the patch for general sanity, but don't dig into
the details. I can also give A-b if someone I trust to do a good review
in the subject technical area provides an R-b, regardless of whether
they are an "R" or an otherwise un-designated contributor. With "R"
folks the chance is higher for me to see such an R-b posted in the first
place, of course.

I do think an "M" person should provide "at least" an A-b, even if they
delegate the actual detailed review to someone else.

So yes, I think your understanding "is correct" (meaning, selfishly,
that it mostly matches mine, anyway :))

Thanks
Laszlo

Re: [edk2-devel] [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg

[Liming Gao]

Laszlo:


Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, April 30, 2020 2:04 AM
> To: Sean Brogan <spbrogan@outlook.com>; devel@edk2.groups.io; michael.kubacki@outlook.com
> Cc: Andrew Fish <afish@apple.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Leif Lindholm <leif@nuviainc.com>; Gao, Liming <liming.gao@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>; Sean Brogan <sean.brogan@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg
> 
> On 04/28/20 18:35, Sean Brogan wrote:
> > I think this was my fault.
> >
> > I was under the impression that a patch needed one of developers listed
> > in the (m) or (r) section of maintainers.txt to provide a reviewed-by.
> > My new understanding is an ack from the (m) plus anyone providing a
> > reviewed-by is enough.
> 
> It depends on the maintainer, too.
> 
> Personally I give R-b if I carefully review the patch and am pleased
> with it.
> 
> I give A-b if I review the patch for general sanity, but don't dig into
> the details. I can also give A-b if someone I trust to do a good review
> in the subject technical area provides an R-b, regardless of whether
> they are an "R" or an otherwise un-designated contributor. With "R"
> folks the chance is higher for me to see such an R-b posted in the first
> place, of course.
> 
> I do think an "M" person should provide "at least" an A-b, even if they
> delegate the actual detailed review to someone else.
> 
I don't think there is such requirement to maintainer now. If you think this is required, 
You can give the proposal to add this requirement in Maintainers.txt.

Thanks
Liming
> So yes, I think your understanding "is correct" (meaning, selfishly,
> that it mostly matches mine, anyway :))
> 
> Thanks
> Laszlo

Re: [edk2-devel] [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg

[Laszlo Ersek]

On 04/30/20 03:18, Gao, Liming wrote:
> Laszlo:
> 
> 
> Thanks
> Liming
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Thursday, April 30, 2020 2:04 AM
>> To: Sean Brogan <spbrogan@outlook.com>; devel@edk2.groups.io; michael.kubacki@outlook.com
>> Cc: Andrew Fish <afish@apple.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
>> Justen, Jordan L <jordan.l.justen@intel.com>; Leif Lindholm <leif@nuviainc.com>; Gao, Liming <liming.gao@intel.com>; Kinney,
>> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>; Sean Brogan <sean.brogan@microsoft.com>
>> Subject: Re: [edk2-devel] [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg
>>
>> On 04/28/20 18:35, Sean Brogan wrote:
>>> I think this was my fault.
>>>
>>> I was under the impression that a patch needed one of developers listed
>>> in the (m) or (r) section of maintainers.txt to provide a reviewed-by.
>>> My new understanding is an ack from the (m) plus anyone providing a
>>> reviewed-by is enough.
>>
>> It depends on the maintainer, too.
>>
>> Personally I give R-b if I carefully review the patch and am pleased
>> with it.
>>
>> I give A-b if I review the patch for general sanity, but don't dig into
>> the details. I can also give A-b if someone I trust to do a good review
>> in the subject technical area provides an R-b, regardless of whether
>> they are an "R" or an otherwise un-designated contributor. With "R"
>> folks the chance is higher for me to see such an R-b posted in the first
>> place, of course.
>>
>> I do think an "M" person should provide "at least" an A-b, even if they
>> delegate the actual detailed review to someone else.
>>
> I don't think there is such requirement to maintainer now. If you think this is required, 
> You can give the proposal to add this requirement in Maintainers.txt.

I feel that the current language is expressive enough:

  M: Package Maintainer: Cc address for patches and questions. Responsible
     for reviewing and pushing package changes to source control.
  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.

See "Responsible for reviewing" vs "help [...] review".

NB, I don't want to force other maintainers to ACK explicitly, if they
consider their co-reviewers' feedback definitive / authoritative. It's
just that, when *only* "R" approval has been posted, and the "M" folks
with jurisdiction don't react at all (no feedback, also not merging the
patch), a *different* maintainer that wants to help out may not be able
to decide whether he/she should wait for more ("M") feedback, or just
run with the "R" feedback. An explicit ACK from the owner "M" guys
clears this up at once.

Thanks
Laszlo

Re: [edk2-devel] [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg

[Leif Lindholm]

On Thu, Apr 30, 2020 at 13:24:57 +0200, Laszlo Ersek wrote:
> On 04/30/20 03:18, Gao, Liming wrote:
> > Laszlo:
> > 
> > 
> > Thanks
> > Liming
> >> -----Original Message-----
> >> From: Laszlo Ersek <lersek@redhat.com>
> >> Sent: Thursday, April 30, 2020 2:04 AM
> >> To: Sean Brogan <spbrogan@outlook.com>; devel@edk2.groups.io; michael.kubacki@outlook.com
> >> Cc: Andrew Fish <afish@apple.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
> >> Justen, Jordan L <jordan.l.justen@intel.com>; Leif Lindholm <leif@nuviainc.com>; Gao, Liming <liming.gao@intel.com>; Kinney,
> >> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>; Sean Brogan <sean.brogan@microsoft.com>
> >> Subject: Re: [edk2-devel] [PATCH v3 0/7] Edk2 Platform and Core CI for ArmVirtPkg, EmulatorPkg, and OvmfPkg
> >>
> >> On 04/28/20 18:35, Sean Brogan wrote:
> >>> I think this was my fault.
> >>>
> >>> I was under the impression that a patch needed one of developers listed
> >>> in the (m) or (r) section of maintainers.txt to provide a reviewed-by.
> >>> My new understanding is an ack from the (m) plus anyone providing a
> >>> reviewed-by is enough.
> >>
> >> It depends on the maintainer, too.
> >>
> >> Personally I give R-b if I carefully review the patch and am pleased
> >> with it.
> >>
> >> I give A-b if I review the patch for general sanity, but don't dig into
> >> the details. I can also give A-b if someone I trust to do a good review
> >> in the subject technical area provides an R-b, regardless of whether
> >> they are an "R" or an otherwise un-designated contributor. With "R"
> >> folks the chance is higher for me to see such an R-b posted in the first
> >> place, of course.
> >>
> >> I do think an "M" person should provide "at least" an A-b, even if they
> >> delegate the actual detailed review to someone else.
> >>
> > I don't think there is such requirement to maintainer now. If you think this is required, 
> > You can give the proposal to add this requirement in Maintainers.txt.
> 
> I feel that the current language is expressive enough:
> 
>   M: Package Maintainer: Cc address for patches and questions. Responsible
>      for reviewing and pushing package changes to source control.
>   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.
> 
> See "Responsible for reviewing" vs "help [...] review".
> 
> NB, I don't want to force other maintainers to ACK explicitly, if they
> consider their co-reviewers' feedback definitive / authoritative. It's
> just that, when *only* "R" approval has been posted, and the "M" folks
> with jurisdiction don't react at all (no feedback, also not merging the
> patch), a *different* maintainer that wants to help out may not be able
> to decide whether he/she should wait for more ("M") feedback, or just
> run with the "R" feedback. An explicit ACK from the owner "M" guys
> clears this up at once.

Agreed.

As a (probably unnexessary) counterpoint, I'll just add that in
instances where the Reviewer clearly is better placed to determine
what is correct and the Maintainer is mainly driving the bus, it could
be misleading for the Maintainer to add an A-b (whereas actually
pushing the patch is a very clear explicit approval).

/
    Leif