From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 03 Sep 2019 06:20:00 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C95B2300C72A; Tue, 3 Sep 2019 13:19:59 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-140.ams2.redhat.com [10.36.116.140]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5A88A60471; Tue, 3 Sep 2019 13:19:58 +0000 (UTC) Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integration Phase 1 To: "Ni, Ray" , "rfc@edk2.groups.io" , "devel@edk2.groups.io" , "Gao, Liming" , "Kinney, Michael D" References: <4A89E2EF3DFEDB4C8BFDE51014F606A14E4E1317@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C2BD07D@SHSMSX104.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: <8b35c38b-f914-42b6-e589-871ec92da713@redhat.com> Date: Tue, 3 Sep 2019 15:19:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C2BD07D@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Tue, 03 Sep 2019 13:19:59 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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 be= tween 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 develo= pers? 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 >=20 >> -----Original Message----- >> From: rfc@edk2.groups.io On Behalf Of Laszlo Ersek >> Sent: Friday, August 30, 2019 5:58 AM >> To: devel@edk2.groups.io; Gao, Liming ; rfc@edk2.= groups.io; Kinney, Michael D >> >> Subject: Re: [edk2-rfc] [edk2-devel] [RFC] EDK II Continuous Integratio= n 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 Mic= hael >>>> 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 Jenkins evaluation >>>> * Laszlo Ersek GitLab evaluation >>>> * Philippe Mathieu-Daud=C3=A9 GitLab evaluation >>>> * Sean Brogan Azure Pipelines and HBFA >>>> * Bret Barkelew Azure Pipelines and HBF= A >>>> * 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-Conti= nuous- >>>> Integration >>>> >>>> Proposal >>>> =3D=3D=3D=3D=3D=3D=3D=3D >>>> 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 pass= es 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 pushin= g >> 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 proces= s 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 fo= r >> 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 th= e >> 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 >>>> >>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D >>>> >>>> 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=3D2050) >>> 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. >> >>=20 >=20