diff mbox series

[kirkstone,1/2] insane: Improve patch warning/error handling

Message ID 20250815094509.3034151-1-philip.lorenz@bmw.de
State Deferred
Delegated to: Steve Sakoman
Headers show
Series [kirkstone,1/2] insane: Improve patch warning/error handling | expand

Commit Message

Philip Lorenz Aug. 15, 2025, 9:45 a.m. UTC
From: Richard Purdie <richard.purdie@linuxfoundation.org>

Currently, whilst patch errors or warnings are shown, the errors don't stop builds.
The configuration isn't very configurable from WARN_QA and ERROR_QA either.

This patch:
 * Uses the standard mechanisms to handle the patch fuzz warnings/errors
 * Makes Upstream-Status checking configurable from WARN/ERROR_QA
 * Allows that checking to be used with non-core layers
 * Makes patch-fuzz an error by default

(From OE-Core rev: 76a685bfcf927593eac67157762a53259089ea8a)

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
(cherry picked from commit 3c3fd6a65e8103f74ae382d196d486b31a168b39)

The backported commit was modified to not mark "patch-fuzz" as an error
by default (which retains compatibility with kirkstone behaviour).

Signed-off-by: Philip Lorenz <philip.lorenz@bmw.de>
---
 meta/classes/insane.bbclass | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comments

Steve Sakoman Aug. 19, 2025, 4:42 p.m. UTC | #1
This patch set is causing warnings for various non-oe-core layers used
in autobuilder testing:

meta-intel
https://autobuilder.yoctoproject.org/valkyrie/?#/builders/41/builds/2086/steps/12/logs/warnings

meta-virtualization
https://autobuilder.yoctoproject.org/valkyrie/?#/builders/89/builds/2099/steps/12/logs/warnings
https://autobuilder.yoctoproject.org/valkyrie/?#/builders/89/builds/2099/steps/16/logs/warnings

meta-agl
https://autobuilder.yoctoproject.org/valkyrie/?#/builders/55/builds/2088/steps/13/logs/warnings

meta-gplv2
https://autobuilder.yoctoproject.org/valkyrie/?#/builders/33/builds/1299/steps/11/logs/warnings

I'm not inclined to take this patch set since it is adding a lot of
noise to my testing.

If you submit patches for those layers to deal with the above issues,
or work with the layer maintainers to fix them I am happy to
reconsider.

Thanks!

Steve

On Fri, Aug 15, 2025 at 2:53 AM Philip Lorenz via
lists.openembedded.org <philip.lorenz=bmw.de@lists.openembedded.org>
wrote:
>
> From: Richard Purdie <richard.purdie@linuxfoundation.org>
>
> Currently, whilst patch errors or warnings are shown, the errors don't stop builds.
> The configuration isn't very configurable from WARN_QA and ERROR_QA either.
>
> This patch:
>  * Uses the standard mechanisms to handle the patch fuzz warnings/errors
>  * Makes Upstream-Status checking configurable from WARN/ERROR_QA
>  * Allows that checking to be used with non-core layers
>  * Makes patch-fuzz an error by default
>
> (From OE-Core rev: 76a685bfcf927593eac67157762a53259089ea8a)
>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> (cherry picked from commit 3c3fd6a65e8103f74ae382d196d486b31a168b39)
>
> The backported commit was modified to not mark "patch-fuzz" as an error
> by default (which retains compatibility with kirkstone behaviour).
>
> Signed-off-by: Philip Lorenz <philip.lorenz@bmw.de>
> ---
>  meta/classes/insane.bbclass | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index f4b4c05e3d8..495bdda652c 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -28,6 +28,7 @@ WARN_QA ?= " libdir xorg-driver-abi \
>              missing-update-alternatives native-last missing-ptest \
>              license-exists license-no-generic license-syntax license-format \
>              license-incompatible license-file-missing obsolete-license \
> +            patch-status-noncore \
>              "
>  ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
>              perms dep-cmp pkgvarcheck perm-config perm-line perm-link \
> @@ -1182,24 +1183,27 @@ python do_qa_patch() {
>              msg += "    devtool modify %s\n" % d.getVar('PN')
>              msg += "    devtool finish --force-patch-refresh %s <layer_path>\n\n" % d.getVar('PN')
>              msg += "Don't forget to review changes done by devtool!\n"
> -            if bb.utils.filter('ERROR_QA', 'patch-fuzz', d):
> -                bb.error(msg)
> -            elif bb.utils.filter('WARN_QA', 'patch-fuzz', d):
> -                bb.warn(msg)
> -            msg = "Patch log indicates that patches do not apply cleanly."
> +            msg += "\nPatch log indicates that patches do not apply cleanly."
>              oe.qa.handle_error("patch-fuzz", msg, d)
>
>      # Check if the patch contains a correctly formatted and spelled Upstream-Status
>      import re
>      from oe import patch
>
> +    allpatches = False
> +    if bb.utils.filter('ERROR_QA', 'patch-status-noncore', d) or bb.utils.filter('WARN_QA', 'patch-status-noncore', d):
> +        allpatches = True
> +
>      coremeta_path = os.path.join(d.getVar('COREBASE'), 'meta', '')
>      for url in patch.src_patches(d):
>         (_, _, fullpath, _, _, _) = bb.fetch.decodeurl(url)
>
>         # skip patches not in oe-core
> +       patchtype = "patch-status-core"
>         if not os.path.abspath(fullpath).startswith(coremeta_path):
> -           continue
> +           patchtype = "patch-status-noncore"
> +           if not allpatches:
> +               continue
>
>         kinda_status_re = re.compile(r"^.*upstream.*status.*$", re.IGNORECASE | re.MULTILINE)
>         strict_status_re = re.compile(r"^Upstream-Status: (Pending|Submitted|Denied|Accepted|Inappropriate|Backport|Inactive-Upstream)( .+)?$", re.MULTILINE)
> @@ -1212,9 +1216,13 @@ python do_qa_patch() {
>
>             if not match_strict:
>                 if match_kinda:
> -                   bb.error("Malformed Upstream-Status in patch\n%s\nPlease correct according to %s :\n%s" % (fullpath, guidelines, match_kinda.group(0)))
> +                   msg = "Malformed Upstream-Status in patch\n%s\nPlease correct according to %s :\n%s" % (fullpath, guidelines, match_kinda.group(0))
> +                   oe.qa.handle_error(patchtype, msg, d)
>                 else:
> -                   bb.error("Missing Upstream-Status in patch\n%s\nPlease add according to %s ." % (fullpath, guidelines))
> +                   msg = "Missing Upstream-Status in patch\n%s\nPlease add according to %s ." % (fullpath, guidelines)
> +                   oe.qa.handle_error(patchtype, msg, d)
> +
> +    oe.qa.exit_if_errors(d)
>  }
>
>  python do_qa_configure() {
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#221879): https://lists.openembedded.org/g/openembedded-core/message/221879
> Mute This Topic: https://lists.openembedded.org/mt/114715380/3620601
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [steve@sakoman.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Philip Lorenz Aug. 20, 2025, 7:52 a.m. UTC | #2
Hi Steve,

On 19.08.25 18:42, Steve Sakoman wrote:
> This patch set is causing warnings for various non-oe-core layers used
> in autobuilder testing:
>
> meta-intel
> https://autobuilder.yoctoproject.org/valkyrie/?#/builders/41/builds/2086/steps/12/logs/warnings
>
> meta-virtualization
> https://autobuilder.yoctoproject.org/valkyrie/?#/builders/89/builds/2099/steps/12/logs/warnings
> https://autobuilder.yoctoproject.org/valkyrie/?#/builders/89/builds/2099/steps/16/logs/warnings
>
> meta-agl
> https://autobuilder.yoctoproject.org/valkyrie/?#/builders/55/builds/2088/steps/13/logs/warnings
>
> meta-gplv2
> https://autobuilder.yoctoproject.org/valkyrie/?#/builders/33/builds/1299/steps/11/logs/warnings
>
> I'm not inclined to take this patch set since it is adding a lot of
> noise to my testing.
>
> If you submit patches for those layers to deal with the above issues,
> or work with the layer maintainers to fix them I am happy to
> reconsider.

One alternative idea: I can also drop the addition of 
patch-status-noncore to WARN_QA which avoids raising these warnings for 
layers (which would have been the better choice to begin with as this 
ultimately avoids any behaviour changes in the default configuration). 
Given that the warning merely informs about missing metadata in patch 
files and this missing metadata was already added to the layers when the 
QA check was merged to master I'd argue that patching all those layers 
will just introduce noise with very limited benefit. Does removing it 
from WARN_QA sound sensible to you?

The rationale for backporting this to kirkstone is to make sure that 
patch-fuzz QA issues, when classified as errors, actually abort the task 
(and therefore no longer create a sstate cache entry). Otherwise the QA 
issue will be bypassed as long as the sstate cache entry remains 
available and only reappear once the offending recipe is rebuilt.

Thanks,

Philip
diff mbox series

Patch

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index f4b4c05e3d8..495bdda652c 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -28,6 +28,7 @@  WARN_QA ?= " libdir xorg-driver-abi \
             missing-update-alternatives native-last missing-ptest \
             license-exists license-no-generic license-syntax license-format \
             license-incompatible license-file-missing obsolete-license \
+            patch-status-noncore \
             "
 ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
             perms dep-cmp pkgvarcheck perm-config perm-line perm-link \
@@ -1182,24 +1183,27 @@  python do_qa_patch() {
             msg += "    devtool modify %s\n" % d.getVar('PN')
             msg += "    devtool finish --force-patch-refresh %s <layer_path>\n\n" % d.getVar('PN')
             msg += "Don't forget to review changes done by devtool!\n"
-            if bb.utils.filter('ERROR_QA', 'patch-fuzz', d):
-                bb.error(msg)
-            elif bb.utils.filter('WARN_QA', 'patch-fuzz', d):
-                bb.warn(msg)
-            msg = "Patch log indicates that patches do not apply cleanly."
+            msg += "\nPatch log indicates that patches do not apply cleanly."
             oe.qa.handle_error("patch-fuzz", msg, d)
 
     # Check if the patch contains a correctly formatted and spelled Upstream-Status
     import re
     from oe import patch
 
+    allpatches = False
+    if bb.utils.filter('ERROR_QA', 'patch-status-noncore', d) or bb.utils.filter('WARN_QA', 'patch-status-noncore', d):
+        allpatches = True
+
     coremeta_path = os.path.join(d.getVar('COREBASE'), 'meta', '')
     for url in patch.src_patches(d):
        (_, _, fullpath, _, _, _) = bb.fetch.decodeurl(url)
 
        # skip patches not in oe-core
+       patchtype = "patch-status-core"
        if not os.path.abspath(fullpath).startswith(coremeta_path):
-           continue
+           patchtype = "patch-status-noncore"
+           if not allpatches:
+               continue
 
        kinda_status_re = re.compile(r"^.*upstream.*status.*$", re.IGNORECASE | re.MULTILINE)
        strict_status_re = re.compile(r"^Upstream-Status: (Pending|Submitted|Denied|Accepted|Inappropriate|Backport|Inactive-Upstream)( .+)?$", re.MULTILINE)
@@ -1212,9 +1216,13 @@  python do_qa_patch() {
 
            if not match_strict:
                if match_kinda:
-                   bb.error("Malformed Upstream-Status in patch\n%s\nPlease correct according to %s :\n%s" % (fullpath, guidelines, match_kinda.group(0)))
+                   msg = "Malformed Upstream-Status in patch\n%s\nPlease correct according to %s :\n%s" % (fullpath, guidelines, match_kinda.group(0))
+                   oe.qa.handle_error(patchtype, msg, d)
                else:
-                   bb.error("Missing Upstream-Status in patch\n%s\nPlease add according to %s ." % (fullpath, guidelines))
+                   msg = "Missing Upstream-Status in patch\n%s\nPlease add according to %s ." % (fullpath, guidelines)
+                   oe.qa.handle_error(patchtype, msg, d)
+
+    oe.qa.exit_if_errors(d)
 }
 
 python do_qa_configure() {