From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 7D4A7AC0FC8 for ; Sun, 18 Feb 2024 09:35:52 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=vPVdGY/Kut+QKs1YcKYkI8FXLF23MyT5GdUxe2HaFDE=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1708248951; v=1; b=DIzqogOQuhp58I6LjQoDB3pxDR41Vkl7uEyRI73aPo149G9VQp5mqrL1ui1ap2hmpR/jIR7v 2ug6aiLAvwpIWr55Kx+jLOt1m/x7ph3LqJpf+GIngS0fec+lAJdMXVqPz7agI0oTTUqIKz5OMgi nv+n8qmLRvfZqziHqM4j+yuo= X-Received: by 127.0.0.2 with SMTP id OGFWYY7687511xQQ3sjixz2O; Sun, 18 Feb 2024 01:35:51 -0800 X-Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by mx.groups.io with SMTP id smtpd.web10.15110.1708248950166478447 for ; Sun, 18 Feb 2024 01:35:50 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id A7790CE0C44 for ; Sun, 18 Feb 2024 09:35:46 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 86131C43399 for ; Sun, 18 Feb 2024 09:35:45 +0000 (UTC) X-Received: by mail-lf1-f44.google.com with SMTP id 2adb3069b0e04-511570b2f49so3794050e87.1 for ; Sun, 18 Feb 2024 01:35:45 -0800 (PST) X-Gm-Message-State: mnoZAK9MNg7kyi8Y51ZH4GBex7686176AA= X-Google-Smtp-Source: AGHT+IE5T98pO13BYRQAsBu01M4lf5LuK8O6ndYuGsrNs++bDYmwhFfwixTp539mjVfhFU8ntUt+ALwX8dtPqFhcPuc= X-Received: by 2002:a05:6512:3135:b0:512:b275:652 with SMTP id p21-20020a056512313500b00512b2750652mr135508lfd.16.1708248943469; Sun, 18 Feb 2024 01:35:43 -0800 (PST) MIME-Version: 1.0 References: <20240214011751.2529-1-michael.d.kinney@intel.com> In-Reply-To: From: "Ard Biesheuvel" Date: Sun, 18 Feb 2024 10:35:31 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [Patch 1/1] BaseTools/Scripts/PatchCheck: Error if commit modifies multiple packages To: devel@edk2.groups.io, michael.d.kinney@intel.com Cc: Leif Lindholm , Rebecca Cran , Liming Gao , "Feng, Bob C" , "Chen, Christine" , Michael Kubacki Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=DIzqogOQ; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=kernel.org (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Hello Mike, Thanks for this proposal. The tag in the commit log works fine for me. This leaves the issue you raised that a contributor might add this tag themselves when sending the patch but this is something we should be able to catch in review. -- Ard. On Sun, 18 Feb 2024 at 04:36, Michael D Kinney wrote: > > Hi Ard, > > I did some experiments with the 2 methods discussed earlier in > this thread to disable the specific check being discussed in this > patch. Details of the experiments and analysis of the results > are included at the end of the email. > > 1) Use GitHub PR Label > 2) Add a special tags to a commit message > > My conclusion is that using a PR Label to control CI actions is > not a good approach due to some behaviors in GitHub and that I > recommend that special tags be used in commit messages as a method > to enable/disable specific CI check options. There is actually > precedence for use of commit message tags in GitHub to skip CI > workflow runs: > > https://docs.github.com/en/actions/managing-workflow-runs/skipping-workflow-runs > > The proposal is to define a tag name 'Continuous-integration-options:' > that can appear on one or more lines of the commit message and the tag > value is a list of one or more CI check options. This defines a mechanism > that can be extended as needed. For the specific check under discussion > here, I am proposing an option value of 'PatchCheck.ignore-multi-package' > An example commit message line to disable the multiple package check > for the one commit would be: > > Continuous-integration-options: PatchCheck.ignore-multi-package > > When PatchCheck evaluates the commit message, if this tag/value is > present, then the multiple package check is still run, but the log > shows results as WARNING messages, and no errors are raised and CI > receives a PASS result. > > Please provide feedback on this updated proposal. > > Thanks, > > Mike > > ======================================================== > > GitHub PR Label Experiments > ============================ > An experiment was done to use a GitHub PR Label to enable/disable the > multiple package check. PatchCheck was ported to a GitHub Action for > this experiment and a label with name 'ignore-multi-package' is used > to disable the PatchCheck multiple packages check. PatchCheck is updated > to support a --ignore-multi-option flag that is passed into PatchCheck > when the label is set. > > Description Result Label Set PR Link > ------------------ ------- --------- ---------------------------------------- > Modify 1 package PASS NO https://github.com/mdkinney/edk2/pull/15 > Modify 1 package PASS YES https://github.com/mdkinney/edk2/pull/15 > Modify 2 packages FAIL NO https://github.com/mdkinney/edk2/pull/16 > Modify 2 packages PASS YES https://github.com/mdkinney/edk2/pull/17 > > Main Branch: > * https://github.com/mdkinney/edk2/tree/CiEnabled > Patch Check with --ignore-multi-package option: > * https://github.com/mdkinney/edk2/blob/CiEnabled/BaseTools/Scripts/PatchCheck.py > GitHub Action Files: > * https://github.com/mdkinney/edk2/blob/CiEnabled/.github/workflows/patchcheck.yml > * https://github.com/mdkinney/edk2/blob/CiEnabled/.github/workflows/patchcheck_call.yml > > The expected results were achieved where the label has no impact on a > PR with commits that only modifies a single package, and the label being > set can change a PR from FAIL->PASS if one or more of the commits in the > PR modify multiple packages. > > The disadvantages of this approach are: > 1) The scope of the label is the entire PR for all commits instead of > the specific commits that require the check to be skipped. > 2) The information to skip a specific check is only in the GitHub PR > in the form of a label being set. The git commit history has no information > on the label settings. This can cause side effects if downstream consumers > use some of the edk2 CI checkers and may then see unexpected CI failures. > 3) CI is run again when commits are merged with a 'push' action. It was > not verified, but the label information is not provided when a push > is performed, so the result of using labels to change CI behavior > may be a PASS condition when the PR is being reviewed, and a FAIL > condition when the commits from the PR are merged. This is not an expected > or acceptable state of the main branch. > 4) The GitHub action must add 'labeled' and 'unlabeled' PR types for > the GitHub action to run when a label is added or removed. There is > no mechanism to filter based on which specific label was added or > removed. This means CI must be invoked each time a label is modified > in a PR, even for labels that are not related to CI behavior. This is > not efficient for CI and implies that this mechanism does not scale > well if several of these CI option labels are added over time. > * Experiments were run to filter based in label inside the GitHub > Action itself. This *only* worked if a CI related label is modified. > If a non-CI related label is modified, the GitHub Action is skipped, > and the result is PASS even if the result of the last run was FAIL. > This would allow merges of PRs with known issues. > * The other option is to remove 'labeled' and 'unlabeled' events from > the GitHub Action. This can work, but has an unexpected behavior from > the maintainer perspective. Setting a label does not re-run CI. > Instead, the label has to be set, then either the PR must be closed > and re-opened, or additional commits have to be added, or commits > must be force pushed in order for the CI check to be re-run with > the label changes. This also implies that that PASS/FAIL state of > the PR can be out of sync with the current label settings if those > additional actions are not performed after the labels are modified. > > Commit Message Tag Experiments > ============================== > An experiment was done to use a commit message tag/value to disable the > multiple package check on only the commits that contain a matching > tag/value. The tag/value used is: > > Continuous-Integration-Options: PatchCheck.ignore-multi-package > > Description Result Tag Present PR Link > ------------------ ------- ----------- ---------------------------------------- > Modify 1 package PASS NO https://github.com/mdkinney/edk2/pull/30 > Modify 2 packages FAIL NO https://github.com/mdkinney/edk2/pull/28 > Modify 2 packages PASS YES https://github.com/mdkinney/edk2/pull/29 > > Main Branch: > * https://github.com/mdkinney/edk2/tree/CommitMessageCiFilter > Patch Check with Continuous-Integration-Options tag parsing > * https://github.com/mdkinney/edk2/blob/CommitMessageCiFilter/BaseTools/Scripts/PatchCheck.py > GitHub Action File: > * https://github.com/mdkinney/edk2/blob/CommitMessageCiFilter/.github/workflows/patchcheck.yml > > The expected results were achieved when adding the tag/value to the > commit message of a commit that modifies multiple packages changes > the PR from FAIL->PASS. > > The disadvantages of this approach are: > 1) Adds CI specific tags to commit messages and commit history > 2) Requires author or maintainer to update commit message with a > CI specific tag/value if a multiple package check fails and is > considered a false positive. > > The advantages of this approach are: > 1) The scope of the tag/value is a single commit. Not the entire PR. > 2) The information about what CI checks to skip are part of the > commit message and the commit history so the information can be > reused if needed in downstream CI. > 3) The information is in the commit message, so it is available for both > PR CI checks and 'push' CI checks. > 4) No new behavior from a maintainer perspective on how/when CI checks > are run and when the status if CI checks are available. > > ================================================================== > > > -----Original Message----- > > From: Kinney, Michael D > > Sent: Wednesday, February 14, 2024 4:49 PM > > To: Ard Biesheuvel ; devel@edk2.groups.io > > Cc: Leif Lindholm ; Rebecca Cran > > ; Liming Gao ; Feng, Bob C > > ; Chen, Christine ; Michael > > Kubacki ; Kinney, Michael D > > > > Subject: RE: [edk2-devel] [Patch 1/1] BaseTools/Scripts/PatchCheck: > > Error if commit modifies multiple packages > > > > Hi Ard, > > > > I agree we do not want to code to be worse. Which I interpret > > in this context as introducing extra commits that reduce the > > readability of the commits. > > > > We also need to consider the ease of review of patches and clear > > responsibility for providing reviews. Especially when we have > > different reviewers/maintainers for different packages. It makes > > it easier to review a patch when patches do not cross package > > boundaries. Otherwise, what does a Reviewed-by mean if you > > only have context for a portion of the patch? Does that mean > > that reviewer is confident in all the changes including those > > they may not have any experience with? > > > > That means there is some tension between readability of commits > > and code review of commits. > > > > The other topic brought up in this discussion is bisect. > > > > I understand the value of bisect to help identify the source > > of a bug or behavior change. > > > > However, the current EDK II CI system does not test at the > > granularity of single commits. Instead, it tests the entire > > patch series in a PR. > > > > I have also observed some maintainers combining multiple > > patch series into a single PR. > > > > I also suspect that there are many patch series in the edk2 > > commit history that would not work if bisect was run across > > them today. > > > > What this means in practice with the current edk2 repository > > history is that we do not know if every commit can be built > > and run. We only know that the EDK II CI tests that are run > > against PRs have passed. > > > > We do expect build/run at the granularity of a PR merge today. > > However, there is no information in the git history to know > > where the PR merges occur. If that extra bit of information > > was available, then bisect could select the commits at PR > > merge boundaries to look for bug/behavior change. The history > > of merged PRs is in github using the following query. > > > > https://github.com/tianocore/edk2/pulls?q=is%3Apr+is%3Aclosed+is%3Amerg > > ed > > > > I imagine the set of sha hashes at PR boundaries could be extracted > > from this query and bisect could select from that set of sha hashes > > that are a subset of the total set of sha hashes. Biset would > > identify the specific PR merge where the bug or behavior change was > > introduced. > > > > Given the current state of the edk2 repo history, the concern about > > splitting up commits on package boundaries that may cause a bisect > > failure at a commit boundary within a single patch series may not be > > something that needs to be considered. > > > > A refinement to the proposal is to require patches to not cross > > package boundaries and authors simply need to split into multiple > > commits based on package boundaries without considering bisect within > > a single patch series. That prevents adding extra patches that make > > the changes hard to understand and has the additional benefit of > > making the patch series easier to review and clear ownership for > > reviewing each patch. > > > > Best regards, > > > > Mike > > > > > -----Original Message----- > > > From: Ard Biesheuvel > > > Sent: Wednesday, February 14, 2024 3:48 PM > > > To: devel@edk2.groups.io; Kinney, Michael D > > > > > > Cc: Leif Lindholm ; Rebecca Cran > > > ; Liming Gao ; Feng, Bob > > C > > > ; Chen, Christine ; > > Michael > > > Kubacki > > > Subject: Re: [edk2-devel] [Patch 1/1] BaseTools/Scripts/PatchCheck: > > > Error if commit modifies multiple packages > > > > > > On Thu, 15 Feb 2024 at 00:27, Michael D Kinney > > > wrote: > > > > > > > > Hi Ard, > > > > > > > > Using commit message does provide granularity down to a single > > > commit, > > > > which may be better than PR label which applies across all commits > > > > in a series. > > > > > > > > However, a flag in the commit message can be set by the author who > > > may > > > > not be a maintainer and maintainers would then need to review > > commit > > > > messages for incorrect use of those flags. > > > > > > > > Only maintainers/admins can set labels, so from a permission > > > management > > > > perspective the PR label has an advantage. > > > > > > > > Additional comments below. There are ways to support bisect and > > meet > > > > the proposed checks. The suggestion uses techniques that Laszlo > > > helped > > > > me with in the past when working on issues like there. I have seen > > > more > > > > complex scenarios than the examples listed below, and have been > > able > > > to > > > > figure out a path through. > > > > > > > > I would agree it is extra work to think about these when working on > > > > the code changes and extra work to reformulate the patches when > > these > > > > conditions are encountered. > > > > > > > > I just want to be clear on the objections. It is not about if the > > > patches > > > > can be organized to follow this proposal. The objection is about > > the > > > > extra work required to reformulate patches. > > > > > > > > > > No. > > > > > > The objection is fundamentally about having to appease the CI even if > > > doing so is unreasonable. I don't mind a bit of extra work. I do mind > > > having to make code changes that make the code worse just to tick a > > CI > > > box. (This is why I disabled uncrustify for the ARM packages: many > > > header files which were perfectly legible before were converted into > > a > > > jumble of alphabet soup because uncrustify removed all of the > > > indentation) > > > > > > It is about having the discretion to deviate from the rules in the > > odd > > > case where the cure is worse than the disease. > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115567): https://edk2.groups.io/g/devel/message/115567 Mute This Topic: https://groups.io/mt/104345509/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-