On 08/30/19 10:43, Liming Gao wrote:

Mike:
I add my comments.

-----Original Message-----
From: rfc@edk2.groups.io [mailto:rfc@edk2.groups.io] On Behalf Of Michael
D Kinney
Sent: Friday, August 30, 2019 4:23 AM
To: devel@edk2.groups.io; rfc@edk2.groups.io
Subject: [edk2-rfc] [RFC] EDK II Continuous Integration Phase 1

Hello,

This is a proposal for a first step towards continuous
integration for all TianoCore repositories to help
improve to quality of commits and automate testing and
release processes for all EDK II packages and platforms.

This is based on work from a number of EDK II community
members that have provide valuable input and evaluations.

* Rebecca Cran <rebecca@...> Jenkins evaluation
* Laszlo Ersek <lersek@...> GitLab evaluation
* Philippe Mathieu-Daudé <philmd@...> GitLab evaluation
* Sean Brogan <sean.brogan@...> Azure Pipelines and HBFA
* Bret Barkelew <Bret.Barkelew@...> Azure Pipelines and HBFA
* Jiewen Yao <jiewen.yao@...> HBFA

The following link is a link to an EDK II WIKI page that
contains a summary of the work to date. Please provide
feedback in the EDK II mailing lists. The WIKI pages will
be updated with input from the entire EDK II community.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Continuous-
Integration

Proposal
========
Phase 1 of adding continuous integration is limited to the
edk2 repository. Additional repositories will be added later.

The following changes are proposed:
* Remove EDK II Maintainers write access to edk2 repository.
Only EDK II Administrators will continue to have write
access, and that should only be used to handle extraordinary
events.
* EDK II Maintainers use a GitHub Pull Request instead of push
to commit a patch series to the edk2 repository. There are
no other changes to the development and review process. The
patch series is prepared in an EDK II maintainer branch with
all commit message requirements met on each patch in the series.

Will the maintainer manually provide pull request after the patch passes the review?

Yes. The maintainer will prepare a local branch that is rebased to
master, and has all the mailing list feedback tags (Reviewed-by, etc)
applied. The maintainer also does all the local testing that they
usually do, just before the final "git push origin".

Then, the final "git push origin" action is replaced by:
(1) git push personal-repo topic-branch-pr
(2) manual creation of a GitHub.com Pull Request, for the topic branch
just pushed.

That is, the final "git push origin" action is replaced with the pushing
of a personal (ready-made) topic branch, and a GitHub.com Pull Request
against that branch. The verification and the final merging will be
performed by github.

Can the script scan the mail list and auto trig pull request once the patch gets
Reviewed-by or Acked-by from Package maintainers?

No, it can't. (And, at this stage, it should not.) The coordination
between submitters, maintainers, reviewers, and occasionally, stewards,
for determining when a patch series is ready to go, based on review
discussion, remains the same.

* The edk2 repository only accepts Pull Requests from members
of the EDK II Maintainers team. Pull Requests from anyone else
are rejected.
* Run pre-commit checks using Azure Pipelines

The maintainer manually trig pre-commit check or auto trig pre-commit?

After the maintainer pushes the ready-made branch to their personal
repo, and submits a PR against that, the PR will set off the checks. If
the checks pass, the topic branch is merged.

By default, pre-commit should be auto trigged based on pull request.

* If all pre-commit checks pass, then the patch series is auto
committed. The result of this commit must match the contents
and commit history that would have occurred using the previous
push operation.
* If any pre-commit checks fail, then notify the submitter.

Will Pre-commit check fail send the mail to the patch submitter?
The patch submitter need update the patch and go through review process again.

My understanding is that, if the testing in GitHub.com fails, the PR
will be rejected and closed. This will generate a notification email for
the maintainer that submitted the PR. The maintainer can then return to
the email thread, and report the CI failure, possibly with a link to the
failed / rejected PR.

Then, indeed, the submitter must rework the series and post a new
version to the list.

It's also possible (and polite) if the maintainer posts the PR link in
the mailing list thread right after submitting the PR. Then the
submitter can monitor the PR too. (Subscribe to it for notifications.)
As I understand it.

Furthermore,

A typical reason for a failure would be a merge conflict with
another pull request that was just processed.
* Limit pre-commit checks execution time to 10 minutes.
* Provide on-demand builds to EDK II Maintainers that to allow
EDK II Maintainers to submit a branch through for the same
set of pre-commit checks without submitting a pull request.

a maintainer could use this on-demand build & testing facility in the
course of review, to report findings early.

Thanks
Laszlo

## Pre-Commit Checks in Phase 1
* Run and pass PatchCheck.py with no errors

=====================================================

The following are some additional pre-commit check ideas
that could be quickly added once the initial version using
PatchCheck.py is fully functional. Please provide feedback
on the ones you like and additional ones you think may
improve the quality of the commits to the edk2 repository.

## Proposed Pre-Commit Checks in Phase 2
* Verify Reviewed-by and Acked-by tags are present with
correct maintainer email addresses
* Verify no non-ASCII characters in modified files
* Verify no binary files in set of modified files

Now, Edk2 has few binary files, like logo.bmp.
I see one BZ to request logo bmp update.
(BZ https://bugzilla.tianocore.org/show_bug.cgi?id=2050)
So, we need the exception way for it.

* Verify package dependency rules in modified files

## Proposed Pre-Commit Checks in Phase 3
* Run ECC on modified files
* Verify modified modules/libs build
* Run available host based tests (HBFA) against modified
modules/libs

I request boot test on Emulator and Ovmf in the daily and weekly scope.
Daily can cover boot to Shell.
Weekly can cover more boot functionality.

Can someone draw a flow chart of who does what to help clarify?

It sounds like maintainers are going to be the very important bridges between CI system and the patch owners (developers). Important it is I agree but boring it is as well I have to say.

Are maintainers still needed to be picked up/chosen/promoted from developers?

On 09/03/19 05:39, Ni, Ray wrote:

Can someone draw a flow chart of who does what to help clarify?

I don't see this as a huge change, relative to the current process.

Before, it's always been up to the subsys maintainer to apply & rebase
the patches locally, pick up the feedback tags, and run at least *some*
build tests before pushing.

After, the final git push is not to "origin" but to a personal branch on
github.com, and then a github PR is needed. If that fails, notify the
submitter. That's all, as far as I can see.

It sounds like maintainers are going to be the very important bridges between CI system and the patch owners (developers). Important it is I agree but boring it is as well I have to say.

Oh, it *is* absolutely boring.

Are maintainers still needed to be picked up/chosen/promoted from developers?

Would you trust a person to apply, build-test, and push your UefiCpuPkg
patches, if that person had *zero* UefiCpuPkg development experience?
(Or even zero edk2 development experience?)

Maintainers don't have to be intimately familiar with the subsystem that
they are pushing a given set of patches for, but every bit of knowledge
helps.

So the answer is "technically no, but you'll regret it".

Maintainership is *always* a chore. It always takes away resources from
development activities, for the maintainer.

In most projects, people alternate in a given maintainer role -- they
keep replacing each other. The variance is mostly in how frequent this
alternation is.

In some projects, the role belongs to a group, and people cover the
maintainer responsibilities in very quick succession. In other projects,
people replace each other in maintainer roles when the current
maintainer gets tired of the work, and steps down. Then someone
volunteers to assume the role.

Either way, the new subsys maintainer is almost always a seasoned
developer in the subsystem.

Thanks,
Laszlo

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Friday, August 30, 2019 5:58 AM
To: devel@edk2.groups.io; Gao, Liming <liming.gao@...>; rfc@edk2.groups.io; Kinney, Michael D
<michael.d.kinney@...>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

On 08/30/19 10:43, Liming Gao wrote:

Mike:
I add my comments.

-----Original Message-----
From: rfc@edk2.groups.io [mailto:rfc@edk2.groups.io] On Behalf Of Michael
D Kinney
Sent: Friday, August 30, 2019 4:23 AM
To: devel@edk2.groups.io; rfc@edk2.groups.io
Subject: [edk2-rfc] [RFC] EDK II Continuous Integration Phase 1

Hello,

This is a proposal for a first step towards continuous
integration for all TianoCore repositories to help
improve to quality of commits and automate testing and
release processes for all EDK II packages and platforms.

This is based on work from a number of EDK II community
members that have provide valuable input and evaluations.

* Rebecca Cran <rebecca@...> Jenkins evaluation
* Laszlo Ersek <lersek@...> GitLab evaluation
* Philippe Mathieu-Daudé <philmd@...> GitLab evaluation
* Sean Brogan <sean.brogan@...> Azure Pipelines and HBFA
* Bret Barkelew <Bret.Barkelew@...> Azure Pipelines and HBFA
* Jiewen Yao <jiewen.yao@...> HBFA

The following link is a link to an EDK II WIKI page that
contains a summary of the work to date. Please provide
feedback in the EDK II mailing lists. The WIKI pages will
be updated with input from the entire EDK II community.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Continuous-
Integration

Proposal
========
Phase 1 of adding continuous integration is limited to the
edk2 repository. Additional repositories will be added later.

The following changes are proposed:
* Remove EDK II Maintainers write access to edk2 repository.
Only EDK II Administrators will continue to have write
access, and that should only be used to handle extraordinary
events.
* EDK II Maintainers use a GitHub Pull Request instead of push
to commit a patch series to the edk2 repository. There are
no other changes to the development and review process. The
patch series is prepared in an EDK II maintainer branch with
all commit message requirements met on each patch in the series.

Will the maintainer manually provide pull request after the patch passes the review?

Yes. The maintainer will prepare a local branch that is rebased to
master, and has all the mailing list feedback tags (Reviewed-by, etc)
applied. The maintainer also does all the local testing that they
usually do, just before the final "git push origin".

Then, the final "git push origin" action is replaced by:
(1) git push personal-repo topic-branch-pr
(2) manual creation of a GitHub.com Pull Request, for the topic branch
just pushed.

That is, the final "git push origin" action is replaced with the pushing
of a personal (ready-made) topic branch, and a GitHub.com Pull Request
against that branch. The verification and the final merging will be
performed by github.

Can the script scan the mail list and auto trig pull request once the patch gets
Reviewed-by or Acked-by from Package maintainers?

No, it can't. (And, at this stage, it should not.) The coordination
between submitters, maintainers, reviewers, and occasionally, stewards,
for determining when a patch series is ready to go, based on review
discussion, remains the same.

* The edk2 repository only accepts Pull Requests from members
of the EDK II Maintainers team. Pull Requests from anyone else
are rejected.
* Run pre-commit checks using Azure Pipelines

The maintainer manually trig pre-commit check or auto trig pre-commit?

After the maintainer pushes the ready-made branch to their personal
repo, and submits a PR against that, the PR will set off the checks. If
the checks pass, the topic branch is merged.

By default, pre-commit should be auto trigged based on pull request.

* If all pre-commit checks pass, then the patch series is auto
committed. The result of this commit must match the contents
and commit history that would have occurred using the previous
push operation.
* If any pre-commit checks fail, then notify the submitter.

Will Pre-commit check fail send the mail to the patch submitter?
The patch submitter need update the patch and go through review process again.

My understanding is that, if the testing in GitHub.com fails, the PR
will be rejected and closed. This will generate a notification email for
the maintainer that submitted the PR. The maintainer can then return to
the email thread, and report the CI failure, possibly with a link to the
failed / rejected PR.

Then, indeed, the submitter must rework the series and post a new
version to the list.

It's also possible (and polite) if the maintainer posts the PR link in
the mailing list thread right after submitting the PR. Then the
submitter can monitor the PR too. (Subscribe to it for notifications.)
As I understand it.

Furthermore,

A typical reason for a failure would be a merge conflict with
another pull request that was just processed.
* Limit pre-commit checks execution time to 10 minutes.
* Provide on-demand builds to EDK II Maintainers that to allow
EDK II Maintainers to submit a branch through for the same
set of pre-commit checks without submitting a pull request.

a maintainer could use this on-demand build & testing facility in the
course of review, to report findings early.

Thanks
Laszlo

## Pre-Commit Checks in Phase 1
* Run and pass PatchCheck.py with no errors

=====================================================

The following are some additional pre-commit check ideas
that could be quickly added once the initial version using
PatchCheck.py is fully functional. Please provide feedback
on the ones you like and additional ones you think may
improve the quality of the commits to the edk2 repository.

## Proposed Pre-Commit Checks in Phase 2
* Verify Reviewed-by and Acked-by tags are present with
correct maintainer email addresses
* Verify no non-ASCII characters in modified files
* Verify no binary files in set of modified files

Now, Edk2 has few binary files, like logo.bmp.
I see one BZ to request logo bmp update.
(BZ https://bugzilla.tianocore.org/show_bug.cgi?id=2050)
So, we need the exception way for it.

* Verify package dependency rules in modified files

## Proposed Pre-Commit Checks in Phase 3
* Run ECC on modified files
* Verify modified modules/libs build
* Run available host based tests (HBFA) against modified
modules/libs

I request boot test on Emulator and Ovmf in the daily and weekly scope.
Daily can cover boot to Shell.
Weekly can cover more boot functionality.

On 09/03/19 18:41, Ni, Ray wrote:

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Tuesday, September 3, 2019 6:20 AM
To: Ni, Ray <ray.ni@...>; rfc@edk2.groups.io; devel@edk2.groups.io; Gao, Liming <liming.gao@...>; Kinney,
Michael D <michael.d.kinney@...>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1

On 09/03/19 05:39, Ni, Ray wrote:

Can someone draw a flow chart of who does what to help clarify?

I don't see this as a huge change, relative to the current process.

Before, it's always been up to the subsys maintainer to apply & rebase
the patches locally, pick up the feedback tags, and run at least *some*
build tests before pushing.

After, the final git push is not to "origin" but to a personal branch on
github.com, and then a github PR is needed. If that fails, notify the
submitter. That's all, as far as I can see.

It sounds like maintainers are going to be the very important bridges between CI system and the patch owners (developers).

Important it is I agree but boring it is as well I have to say.

Oh, it *is* absolutely boring.

Are maintainers still needed to be picked up/chosen/promoted from developers?

Would you trust a person to apply, build-test, and push your UefiCpuPkg
patches, if that person had *zero* UefiCpuPkg development experience?
(Or even zero edk2 development experience?)

Can we change the process a bit?
1. maintainers created pull requests on behave of the patch owners
2. patch owners can be notified automatically if pull requests fail

I believe this can work if:
- the submitter has a github account
- the maintainer knows the submitter's account name
- the maintainer manually subscribes the submitter to the PR
(I have never tried this myself)

3. patch owners update the pull requests
(I am not familiar to pull requests. I assume the rights of modifying pull requests and creating pull requests are separated. Are they?)

PRs should not be updated. If there is a CI failure, the PR should be
rejected. If that happens automatically, that's great. If not, then
someone must close the PR manually. If the patch series submitter can do
that, that's great (I have no idea if that works!) If only the PR owner
(= maintainer) can close the PR, then they should close it.

Either way, the patch author will have to submit a v2 to the list. When
that is sufficiently reviewed, the same process starts (new PR opened by
maintainer).

So, maintainers only need to initiate the pull requests.

I hope this can actually work. We need to test this out first.

It assumes when pull requests are initiated, everyone at least agrees the justifications of the changes are valid and the ways the changes are done.

I agree fully about this. If the CI check completes, then the changes
are pushed to master automatically! (Only if the push is a fast-forward
-- otherwise the PR is rejected again. GitHub will not be allowed to
merge or rebase withoout supervision.)

Thanks
Laszlo

Laszlo/Mike,

The idea that the maintainer must create the PR is fighting the optimized github PR flow. Github and PRs process is optimized for letting everyone contribute from "their" fork while still protecting and putting process in place for the "upstream".

Why not use github to assign maintainers to each package or filepath and then let contributors submit their own PR to edk2 after the mailing list has approved. Then the PR has a policy that someone that is the maintainer of all files changed must approve before the PR can be completed (or you could even set it so that maintainer must complete PR). This would have the benefit of less monotonous work for the maintainers and on rejected PRs the contributor could easily update their branch and resubmit their PR.

Thanks
Sean

On 09/03/19 19:09, Sean Brogan wrote:

Laszlo/Mike,

The idea that the maintainer must create the PR is fighting the
optimized github PR flow. Github and PRs process is optimized for
letting everyone contribute from "their" fork while still protecting
and putting process in place for the "upstream".

Why not use github to assign maintainers to each package or filepath
and then let contributors submit their own PR to edk2 after the
mailing list has approved. Then the PR has a policy that someone that
is the maintainer of all files changed must approve before the PR can
be completed (or you could even set it so that maintainer must
complete PR). This would have the benefit of less monotonous work
for the maintainers and on rejected PRs the contributor could easily
update their branch and resubmit their PR.

I'll let Mike respond.

Thanks
Laszlo

On 2019-09-03 10:41, Ni, Ray wrote:

Can we change the process a bit?
1. maintainers created pull requests on behave of the patch owners
2. patch owners can be notified automatically if pull requests fail
3. patch owners update the pull requests
(I am not familiar to pull requests. I assume the rights of modifying pull requests and creating pull requests are separated. Are they?)

So, maintainers only need to initiate the pull requests. It assumes when pull requests are initiated, everyone at least agrees the justifications of the changes are valid and the ways the changes are done.

In other projects I've worked on, patch owners initiate the pre-commit
build/test, and either comment on the review (ideally with link to the
results) about its status, or have the CI system comment automatically
on the review page/thread.

Once the maintainer has signed off on the patch, it can then be sent for
gatekeeping, which is either a manual process whereby the repo owner (or
subsystem maintainer) checks the review, runs any additional tests they
want etc. - or via a 'land' command (for example with Review Board it's
"rbt land") which automates the checks and pushes the changeset.


I understand that with Azure Pipelines and how it integrates with
Github, along with the fact that this project doesn't use pull requests
or any other review automation, means things are probably going to be
pretty different to most other projects for now.


--

Rebecca Cran

Sean,

There may be many ways to improve the process and reduce
the work maintainers perform. So these are ideas we can
explore further going forward. I will add the concept
of a non-maintainer opening the PR to the Wiki.

In order to get the improvements to code quality from CI
enabled as quickly as possible, I recommend we limit PRs
to maintainers.

Thanks,

Mike