[PATCH 1/3] insane: Improve patch warning/error handling

[PATCH 1/3] insane: Improve patch warning/error handling

[Richard Purdie]

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
 * Enables warnings for missing Upstream-Status in non-core layer patches by default

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 meta/classes-global/insane.bbclass | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/meta/classes-global/insane.bbclass b/meta/classes-global/insane.bbclass
index ada8a7ef4e4..e1295f85392 100644
--- a/meta/classes-global/insane.bbclass
+++ b/meta/classes-global/insane.bbclass
@@ -29,11 +29,12 @@
 WARN_QA ?= " libdir xorg-driver-abi buildpaths \
             textrel incompatible-license files-invalid \
             infodir build-deps src-uri-bad symlink-to-sysroot multilib \
-            invalid-packageconfig host-user-contaminated uppercase-pn patch-fuzz \
+            invalid-packageconfig host-user-contaminated uppercase-pn \
             mime mime-xdg unlisted-pkg-lics unhandled-features-check \
             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 \
@@ -44,6 +45,7 @@ ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
             already-stripped installed-vs-shipped ldflags compile-host-path \
             install-host-path pn-overrides unknown-configure-option \
             useless-rpaths rpaths staticdev empty-dirs \
+            patch-fuzz patch-status-core\
             "
 # Add usrmerge QA check based on distro feature
 ERROR_QA:append = "${@bb.utils.contains('DISTRO_FEATURES', 'usrmerge', ' usrmerge', '', d)}"
@@ -1334,24 +1336,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)
@@ -1364,9 +1369,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() {
-- 
2.37.2

[PATCH 2/3] pseudo: Update to pull in linux-libc-headers race fix

[Richard Purdie]

Update to pull in:

    pseudo.c: Avoid patch mismatch errors for NAMELESS file entries

    In rare cases we see failures, often in linux-libc-headers for things like:

    |   INSTALL /XXX/linux-libc-headers/6.1-r0/image/usr/include
    | abort()ing pseudo client by server request. See https://wiki.yoctoproject.org/wiki/Pseudo_Abort for more details on this.

    Pseudo log:
    path mismatch [2 links]: ino 46662476 db 'NAMELESS FILE' req '/XXX/linux-libc-headers/6.1-r0/image/usr'.
    Setup complete, sending SIGUSR1 to pid 3630890.

    Whilst this doesn't easily reproduce, the issue is that multiple different processes are
    likely working on the directory and the creation in pseudo might not match accesses
    made by other processes.

    Ultimately, the "NAMELESS FILE" is harmless and pseudo will reconcile things
    so rather than error out, we should ignore this case.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 meta/recipes-devtools/pseudo/pseudo_git.bb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/recipes-devtools/pseudo/pseudo_git.bb b/meta/recipes-devtools/pseudo/pseudo_git.bb
index 1a708066f73..c9386c3f090 100644
--- a/meta/recipes-devtools/pseudo/pseudo_git.bb
+++ b/meta/recipes-devtools/pseudo/pseudo_git.bb
@@ -13,7 +13,7 @@ SRC_URI:append:class-nativesdk = " \
     file://older-glibc-symbols.patch"
 SRC_URI[prebuilt.sha256sum] = "ed9f456856e9d86359f169f46a70ad7be4190d6040282b84c8d97b99072485aa"
 
-SRCREV = "c9670c27ff67ab899007ce749254b16091577e55"
+SRCREV = "cc1f6167cb5065daba1462056e2dce8ff72aa855"
 S = "${WORKDIR}/git"
 PV = "1.9.0+git${SRCPV}"
 
-- 
2.37.2

[PATCH 3/3] pseudo: Switch back to the master branch

[Richard Purdie]

OE is the main user of pseudo and we've had the changes in the oe-core branch
around long enough that we're going to run with them. Swicth back to directly
using the master branch.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 meta/recipes-devtools/pseudo/pseudo_git.bb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/recipes-devtools/pseudo/pseudo_git.bb b/meta/recipes-devtools/pseudo/pseudo_git.bb
index c9386c3f090..553f0254ee4 100644
--- a/meta/recipes-devtools/pseudo/pseudo_git.bb
+++ b/meta/recipes-devtools/pseudo/pseudo_git.bb
@@ -1,6 +1,6 @@
 require pseudo.inc
 
-SRC_URI = "git://git.yoctoproject.org/pseudo;branch=oe-core \
+SRC_URI = "git://git.yoctoproject.org/pseudo \
            file://0001-configure-Prune-PIE-flags.patch \
            file://fallback-passwd \
            file://fallback-group \
-- 
2.37.2

Re: [OE-core] [PATCH 3/3] pseudo: Switch back to the master branch

[Luca Ceresoli]

Hello Richard,

On Wed, 18 Jan 2023 14:22:21 +0000
"Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:

> OE is the main user of pseudo and we've had the changes in the oe-core branch
> around long enough that we're going to run with them. Swicth back to directly
> using the master branch.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  meta/recipes-devtools/pseudo/pseudo_git.bb | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meta/recipes-devtools/pseudo/pseudo_git.bb b/meta/recipes-devtools/pseudo/pseudo_git.bb
> index c9386c3f090..553f0254ee4 100644
> --- a/meta/recipes-devtools/pseudo/pseudo_git.bb
> +++ b/meta/recipes-devtools/pseudo/pseudo_git.bb
> @@ -1,6 +1,6 @@
>  require pseudo.inc
>  
> -SRC_URI = "git://git.yoctoproject.org/pseudo;branch=oe-core \
> +SRC_URI = "git://git.yoctoproject.org/pseudo \

Triggers "URL: ... does not set any branch parameter. The future is
uncertain and the end is always near" ;)

I'm fixing it on my testing branch for the moment.

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [OE-core] [PATCH 3/3] pseudo: Switch back to the master branch

[Richard Purdie]

On Wed, 2023-01-18 at 17:46 +0100, Luca Ceresoli wrote:
> Hello Richard,
> 
> On Wed, 18 Jan 2023 14:22:21 +0000
> "Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:
> 
> > OE is the main user of pseudo and we've had the changes in the oe-core branch
> > around long enough that we're going to run with them. Swicth back to directly
> > using the master branch.
> > 
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> >  meta/recipes-devtools/pseudo/pseudo_git.bb | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/meta/recipes-devtools/pseudo/pseudo_git.bb b/meta/recipes-devtools/pseudo/pseudo_git.bb
> > index c9386c3f090..553f0254ee4 100644
> > --- a/meta/recipes-devtools/pseudo/pseudo_git.bb
> > +++ b/meta/recipes-devtools/pseudo/pseudo_git.bb
> > @@ -1,6 +1,6 @@
> >  require pseudo.inc
> >  
> > -SRC_URI = "git://git.yoctoproject.org/pseudo;branch=oe-core \
> > +SRC_URI = "git://git.yoctoproject.org/pseudo \
> 
> Triggers "URL: ... does not set any branch parameter. The future is
> uncertain and the end is always near" ;)
> 
> I'm fixing it on my testing branch for the moment.

Thanks, that will teach me to quickly add another patch when cleaning
things up to send!

Cheers,

Richard

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Luca Ceresoli]

Hello Richard,

On Wed, 18 Jan 2023 14:22:19 +0000
"Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:

> 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
>  * Enables warnings for missing Upstream-Status in non-core layer patches by default
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  meta/classes-global/insane.bbclass | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/meta/classes-global/insane.bbclass b/meta/classes-global/insane.bbclass
> index ada8a7ef4e4..e1295f85392 100644
> --- a/meta/classes-global/insane.bbclass
> +++ b/meta/classes-global/insane.bbclass
> @@ -29,11 +29,12 @@
>  WARN_QA ?= " libdir xorg-driver-abi buildpaths \
>              textrel incompatible-license files-invalid \
>              infodir build-deps src-uri-bad symlink-to-sysroot multilib \
> -            invalid-packageconfig host-user-contaminated uppercase-pn patch-fuzz \
> +            invalid-packageconfig host-user-contaminated uppercase-pn \
>              mime mime-xdg unlisted-pkg-lics unhandled-features-check \
>              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 \

AB testing with this patch applied revealed a few Upstream-Status
warnings:

- 5 in meta-agl-core:
  https://autobuilder.yoctoproject.org/typhoon/#/builders/120/builds/2272/steps/13/logs/stdio

- 3 in meta-intel:
  https://autobuilder.yoctoproject.org/typhoon/#/builders/100/builds/3968/steps/13/logs/stdio

Jan-Simon, Anuj, can you look at those patches and maybe fix the Upstream-Status?

Richard, this patch didn't trigger any problems in my test branch. Now
I'm going to remove it from my testing branch in order to avoid
warnings on issues we already know, unless you prefer me too keep it
and ignore the above mentioned warnings until they are fixed in the
respective layers.

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Richard Purdie]

Hi Anuj/Bruce/Jan-Simon/Scott,

Please see the links below. This patch, pending for master triggers a
few warnings in a few layers as there are a handful of patches with no
Upstream-Status present in them.

I know this kind of thing is annoying but it is probably worth starting
to give the wider community the hint that they should document this,
and if they do document it, it might remind them to try and submit them
too. You're not the target audience but as layers being tested on the
autobuilder you get to lead by example! :)

If anyone doesn't want to tweak things to fix this, we can disable the
warnings for that specific test but I would prefer not to, I think this
is the right thing to encourage. The numbers of issues are low and
easily avoided one way or another.

On Thu, 2023-01-19 at 10:24 +0100, Luca Ceresoli wrote:
> On Wed, 18 Jan 2023 14:22:19 +0000
> "Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:
> 
> > 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
> >  * Enables warnings for missing Upstream-Status in non-core layer patches by default
> > 
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> >  meta/classes-global/insane.bbclass | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/meta/classes-global/insane.bbclass b/meta/classes-global/insane.bbclass
> > index ada8a7ef4e4..e1295f85392 100644
> > --- a/meta/classes-global/insane.bbclass
> > +++ b/meta/classes-global/insane.bbclass
> > @@ -29,11 +29,12 @@
> >  WARN_QA ?= " libdir xorg-driver-abi buildpaths \
> >              textrel incompatible-license files-invalid \
> >              infodir build-deps src-uri-bad symlink-to-sysroot multilib \
> > -            invalid-packageconfig host-user-contaminated uppercase-pn patch-fuzz \
> > +            invalid-packageconfig host-user-contaminated uppercase-pn \
> >              mime mime-xdg unlisted-pkg-lics unhandled-features-check \
> >              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 \
> 
> AB testing with this patch applied revealed a few Upstream-Status
> warnings:
> 
> - 5 in meta-agl-core:
>   https://autobuilder.yoctoproject.org/typhoon/#/builders/120/builds/2272/steps/13/logs/stdio
> 
> - 3 in meta-intel:
>   https://autobuilder.yoctoproject.org/typhoon/#/builders/100/builds/3968/steps/13/logs/stdio
> 
> Jan-Simon, Anuj, can you look at those patches and maybe fix the Upstream-Status?
> 
> Richard, this patch didn't trigger any problems in my test branch. Now
> I'm going to remove it from my testing branch in order to avoid
> warnings on issues we already know, unless you prefer me too keep it
> and ignore the above mentioned warnings until they are fixed in the
> respective layers.

There are also some in meta-virt:

https://autobuilder.yoctoproject.org/typhoon/#/builders/128/builds/1168/steps/13/logs/warnings
https://autobuilder.yoctoproject.org/typhoon/#/builders/128/builds/1168/steps/21/logs/warnings

Cheers,

Richard

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Bruce Ashfield]

On Thu, Jan 19, 2023 at 4:57 AM Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> Hi Anuj/Bruce/Jan-Simon/Scott,
>
> Please see the links below. This patch, pending for master triggers a
> few warnings in a few layers as there are a handful of patches with no
> Upstream-Status present in them.
>
> I know this kind of thing is annoying but it is probably worth starting
> to give the wider community the hint that they should document this,
> and if they do document it, it might remind them to try and submit them
> too. You're not the target audience but as layers being tested on the
> autobuilder you get to lead by example! :)
>
> If anyone doesn't want to tweak things to fix this, we can disable the
> warnings for that specific test but I would prefer not to, I think this
> is the right thing to encourage. The numbers of issues are low and
> easily avoided one way or another.

I'd prefer the upstream-status check to be disabled for meta-virtualization.

This isn't something that I'm strictly enforcing, and not something that I
want to start strictly enforcing.

Cheers,

Bruce

>
> On Thu, 2023-01-19 at 10:24 +0100, Luca Ceresoli wrote:
> > On Wed, 18 Jan 2023 14:22:19 +0000
> > "Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:
> >
> > > 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
> > >  * Enables warnings for missing Upstream-Status in non-core layer patches by default
> > >
> > > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > > ---
> > >  meta/classes-global/insane.bbclass | 27 ++++++++++++++++++---------
> > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/meta/classes-global/insane.bbclass b/meta/classes-global/insane.bbclass
> > > index ada8a7ef4e4..e1295f85392 100644
> > > --- a/meta/classes-global/insane.bbclass
> > > +++ b/meta/classes-global/insane.bbclass
> > > @@ -29,11 +29,12 @@
> > >  WARN_QA ?= " libdir xorg-driver-abi buildpaths \
> > >              textrel incompatible-license files-invalid \
> > >              infodir build-deps src-uri-bad symlink-to-sysroot multilib \
> > > -            invalid-packageconfig host-user-contaminated uppercase-pn patch-fuzz \
> > > +            invalid-packageconfig host-user-contaminated uppercase-pn \
> > >              mime mime-xdg unlisted-pkg-lics unhandled-features-check \
> > >              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 \
> >
> > AB testing with this patch applied revealed a few Upstream-Status
> > warnings:
> >
> > - 5 in meta-agl-core:
> >   https://autobuilder.yoctoproject.org/typhoon/#/builders/120/builds/2272/steps/13/logs/stdio
> >
> > - 3 in meta-intel:
> >   https://autobuilder.yoctoproject.org/typhoon/#/builders/100/builds/3968/steps/13/logs/stdio
> >
> > Jan-Simon, Anuj, can you look at those patches and maybe fix the Upstream-Status?
> >
> > Richard, this patch didn't trigger any problems in my test branch. Now
> > I'm going to remove it from my testing branch in order to avoid
> > warnings on issues we already know, unless you prefer me too keep it
> > and ignore the above mentioned warnings until they are fixed in the
> > respective layers.
>
> There are also some in meta-virt:
>
> https://autobuilder.yoctoproject.org/typhoon/#/builders/128/builds/1168/steps/13/logs/warnings
> https://autobuilder.yoctoproject.org/typhoon/#/builders/128/builds/1168/steps/21/logs/warnings
>
> Cheers,
>
> Richard
>
>
>


-- 
- Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end
- "Use the force Harry" - Gandalf, Star Trek II

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Richard Purdie]

On Thu, 2023-01-19 at 08:55 -0500, Bruce Ashfield wrote:
> On Thu, Jan 19, 2023 at 4:57 AM Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > 
> > Hi Anuj/Bruce/Jan-Simon/Scott,
> > 
> > Please see the links below. This patch, pending for master triggers a
> > few warnings in a few layers as there are a handful of patches with no
> > Upstream-Status present in them.
> > 
> > I know this kind of thing is annoying but it is probably worth starting
> > to give the wider community the hint that they should document this,
> > and if they do document it, it might remind them to try and submit them
> > too. You're not the target audience but as layers being tested on the
> > autobuilder you get to lead by example! :)
> > 
> > If anyone doesn't want to tweak things to fix this, we can disable the
> > warnings for that specific test but I would prefer not to, I think this
> > is the right thing to encourage. The numbers of issues are low and
> > easily avoided one way or another.
> 
> I'd prefer the upstream-status check to be disabled for meta-virtualization.
> 
> This isn't something that I'm strictly enforcing, and not something that I
> want to start strictly enforcing.

I understand and will see what I can do to change the configurations, I
was hoping to avoid that.

Just as a heads up, there is an open action for the TSC to discuss
which QA checks shouldn't be bypassed for compatible layer status.

Cheers,

Richard

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Bruce Ashfield]

On Thu, Jan 19, 2023 at 9:40 AM Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> On Thu, 2023-01-19 at 08:55 -0500, Bruce Ashfield wrote:
> > On Thu, Jan 19, 2023 at 4:57 AM Richard Purdie
> > <richard.purdie@linuxfoundation.org> wrote:
> > >
> > > Hi Anuj/Bruce/Jan-Simon/Scott,
> > >
> > > Please see the links below. This patch, pending for master triggers a
> > > few warnings in a few layers as there are a handful of patches with no
> > > Upstream-Status present in them.
> > >
> > > I know this kind of thing is annoying but it is probably worth starting
> > > to give the wider community the hint that they should document this,
> > > and if they do document it, it might remind them to try and submit them
> > > too. You're not the target audience but as layers being tested on the
> > > autobuilder you get to lead by example! :)
> > >
> > > If anyone doesn't want to tweak things to fix this, we can disable the
> > > warnings for that specific test but I would prefer not to, I think this
> > > is the right thing to encourage. The numbers of issues are low and
> > > easily avoided one way or another.
> >
> > I'd prefer the upstream-status check to be disabled for meta-virtualization.
> >
> > This isn't something that I'm strictly enforcing, and not something that I
> > want to start strictly enforcing.
>
> I understand and will see what I can do to change the configurations, I
> was hoping to avoid that.
>
> Just as a heads up, there is an open action for the TSC to discuss
> which QA checks shouldn't be bypassed for compatible layer status.

Understood.

I'll strongly argue that if a layer wants to carry some technical debt
due to the nature of the upstream communities they work within and the
debt is within the layer .. that is something they should be able to
choose.

Cheers,

Bruce

>
> Cheers,
>
> Richard



-- 
- Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end
- "Use the force Harry" - Gandalf, Star Trek II

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Richard Purdie]

On Thu, 2023-01-19 at 09:52 -0500, Bruce Ashfield wrote:
> On Thu, Jan 19, 2023 at 9:40 AM Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > 
> > On Thu, 2023-01-19 at 08:55 -0500, Bruce Ashfield wrote:
> > > On Thu, Jan 19, 2023 at 4:57 AM Richard Purdie
> > > <richard.purdie@linuxfoundation.org> wrote:
> > > > 
> > > > Hi Anuj/Bruce/Jan-Simon/Scott,
> > > > 
> > > > Please see the links below. This patch, pending for master triggers a
> > > > few warnings in a few layers as there are a handful of patches with no
> > > > Upstream-Status present in them.
> > > > 
> > > > I know this kind of thing is annoying but it is probably worth starting
> > > > to give the wider community the hint that they should document this,
> > > > and if they do document it, it might remind them to try and submit them
> > > > too. You're not the target audience but as layers being tested on the
> > > > autobuilder you get to lead by example! :)
> > > > 
> > > > If anyone doesn't want to tweak things to fix this, we can disable the
> > > > warnings for that specific test but I would prefer not to, I think this
> > > > is the right thing to encourage. The numbers of issues are low and
> > > > easily avoided one way or another.
> > > 
> > > I'd prefer the upstream-status check to be disabled for meta-virtualization.
> > > 
> > > This isn't something that I'm strictly enforcing, and not something that I
> > > want to start strictly enforcing.
> > 
> > I understand and will see what I can do to change the configurations, I
> > was hoping to avoid that.
> > 
> > Just as a heads up, there is an open action for the TSC to discuss
> > which QA checks shouldn't be bypassed for compatible layer status.
> 
> Understood.
> 
> I'll strongly argue that if a layer wants to carry some technical debt
> due to the nature of the upstream communities they work within and the
> debt is within the layer .. that is something they should be able to
> choose.

To be clear, you can put Upstream-Status: Pending or Inappropriate and
move on, it doesn't mean they have to be submitted upstream, just that
the status is recorded.

Cheers,

Richard

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Ross Burton]

On 19 Jan 2023, at 13:55, Bruce Ashfield via lists.openembedded.org <bruce.ashfield=gmail.com@lists.openembedded.org> wrote:
> I'd prefer the upstream-status check to be disabled for meta-virtualization.
> 
> This isn't something that I'm strictly enforcing, and not something that I
> want to start strictly enforcing.

With my meta-arm layer maintainer hat on, I’d like to briefly evangelise for this check. We’ve had patch status tracking in meta-arm for a while and it has a lot of advantages for the minimal friction it adds.

Yes, there’s the initial hit of spamming Upstream-Status tags into all the existing patches.  This is fairly simple - especially if you assume Pending and review them At Some Point - but the act of running through the patches and marking backports, patches that will never be sent upstream, and patches that _should_ be sent upstream is useful for everyone, both the layer maintainers and other users of the layer.

If upstream are … tricky to engage with, it’s useful to be able to mark a patch as struggling it’s way upstream and link to the relevant tickets/merge requests.

However, the biggest win for me is the leverage you have over patch contributors:

- Thanks for the contribution.  I see you’ve marked this patch as Pending but it’s a good upstreamable fix, can you send it upstream quickly so we’re not carrying a patch forever? The patch can then be marked as submitted with a link to the pull request.
- This patch is a backport, please mark it as such and include the SHA so we know when it is in a release
- You said six months ago that you were going to refactor this ugly patch, how is that going?

(the last is definitely my favourite)

Just my 2c,
Ross

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Bruce Ashfield]

On Thu, Jan 19, 2023 at 9:56 AM Ross Burton <Ross.Burton@arm.com> wrote:
>
> On 19 Jan 2023, at 13:55, Bruce Ashfield via lists.openembedded.org <bruce.ashfield=gmail.com@lists.openembedded.org> wrote:
> > I'd prefer the upstream-status check to be disabled for meta-virtualization.
> >
> > This isn't something that I'm strictly enforcing, and not something that I
> > want to start strictly enforcing.
>
> With my meta-arm layer maintainer hat on, I’d like to briefly evangelise for this check. We’ve had patch status tracking in meta-arm for a while and it has a lot of advantages for the minimal friction it adds.
>
> Yes, there’s the initial hit of spamming Upstream-Status tags into all the existing patches.  This is fairly simple - especially if you assume Pending and review them At Some Point - but the act of running through the patches and marking backports, patches that will never be sent upstream, and patches that _should_ be sent upstream is useful for everyone, both the layer maintainers and other users of the layer.
>
> If upstream are … tricky to engage with, it’s useful to be able to mark a patch as struggling it’s way upstream and link to the relevant tickets/merge requests.
>
> However, the biggest win for me is the leverage you have over patch contributors:
>
> - Thanks for the contribution.  I see you’ve marked this patch as Pending but it’s a good upstreamable fix, can you send it upstream quickly so we’re not carrying a patch forever? The patch can then be marked as submitted with a link to the pull request.
> - This patch is a backport, please mark it as such and include the SHA so we know when it is in a release
> - You said six months ago that you were going to refactor this ugly patch, how is that going?

That's actually leverage that I don't need over the contributors :)
The complexity of the projects and system level testing required are
the biggest barriers (and I'm working on that). I use my maintainer
karma to push back on larger direction items and features.

I'm not set on a machine parsable format of logging information about
upstream contributions, so I sometimes do ask (and log) the status, or
ask for upgrades, but it is all pretty flexible (I am routinely
annoyed when I can't remember (some sort of mental block) where the
capital letters go in upstream-status and I get a warning ;))

But the point is well taken! I'm just suggesting that one-size or
one-workflow suits all may not be the best initial approach (and I'm
not suggesting that others are suggesting one-size fits all either!).

I do mostly use (or end up with) 'inappropriate' in the patches, and
can do that, but it tends to be when the recipes are next updated, or
otherwise cause a problem. I've started doing that now (editing the
existing patches), but again, I won't insist that everyone does that
on submission, so not having the warning/error disabled is just an
extra load placed on me.

I'm not really a fan of recipes that use tarballs (when a proper
revision control system is available), but I don't force that on all
contributions where I'm the maintainer (ooops, I've started bikeshed
about recipe preferences and pet peeves !! :)

Bruce

>
> (the last is definitely my favourite)
>
> Just my 2c,
> Ross


--
- Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end
- "Use the force Harry" - Gandalf, Star Trek II

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Alexander Kanavin]

On Fri, 20 Jan 2023 at 15:26, Bruce Ashfield <bruce.ashfield@gmail.com> wrote:
> but again, I won't insist that everyone does that
> on submission, so not having the warning/error disabled is just an
> extra load placed on me.

I'm not sure I understand this. If the appropriate upstream-status is
enforced by bitbake, then any contributor has to get it right before
actually sending for review. Or otherwise it means they never built
what they're sending.

Where is the extra load happening?

Alex

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Bruce Ashfield]

On Fri, Jan 20, 2023 at 2:10 PM Alexander Kanavin
<alex.kanavin@gmail.com> wrote:
>
> On Fri, 20 Jan 2023 at 15:26, Bruce Ashfield <bruce.ashfield@gmail.com> wrote:
> > but again, I won't insist that everyone does that
> > on submission, so not having the warning/error disabled is just an
> > extra load placed on me.
>
> I'm not sure I understand this. If the appropriate upstream-status is
> enforced by bitbake, then any contributor has to get it right before
> actually sending for review. Or otherwise it means they never built
> what they're sending.
>
> Where is the extra load happening?
>

Because I'm simply not going to insist on it in all the patches. I
need all the contributions I can get, and I'm not going to
pedantically insist on that.

meta-virt is not oe-core, I do the lifting. Therefore, if bitbake
errors, I have to fix it.

Other layers can nitpick all they want, but I'm not going to adopt that policy.

Bruce

> Alex



-- 
- Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end
- "Use the force Harry" - Gandalf, Star Trek II

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Alexander Kanavin]

On Fri, 20 Jan 2023 at 20:29, Bruce Ashfield <bruce.ashfield@gmail.com> wrote:
> Because I'm simply not going to insist on it in all the patches. I
> need all the contributions I can get, and I'm not going to
> pedantically insist on that.
>
> meta-virt is not oe-core, I do the lifting. Therefore, if bitbake
> errors, I have to fix it.

But you do not need to insist on the needed metadata or fix it after
the fact. Bitbake will do the insisting for you, when contributors
test the change locally *before* they send it to you. If bitbake
errors on your side, this means they never built their contribution,
and you should raise a concern for that reason, and not for the
missing metadata.

Alex

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Richard Purdie]

On Fri, 2023-01-20 at 20:38 +0100, Alexander Kanavin wrote:
> On Fri, 20 Jan 2023 at 20:29, Bruce Ashfield <bruce.ashfield@gmail.com> wrote:
> > Because I'm simply not going to insist on it in all the patches. I
> > need all the contributions I can get, and I'm not going to
> > pedantically insist on that.
> > 
> > meta-virt is not oe-core, I do the lifting. Therefore, if bitbake
> > errors, I have to fix it.
> 
> But you do not need to insist on the needed metadata or fix it after
> the fact. Bitbake will do the insisting for you, when contributors
> test the change locally *before* they send it to you. If bitbake
> errors on your side, this means they never built their contribution,
> and you should raise a concern for that reason, and not for the
> missing metadata.

It isn't that simple since this is a configurable QA warning, all it
takes is one layer/distro to disable it and it is disabled for all
layers that user works on.

This is why "core" is a separate config to "noncore" but we can't have
a config for every layer and even if we did, people would still turn it
off.

If it is turned off, it means people send patches and Bruce has to fix
them, or ask them to resubmit which is extra overhead to the
maintainer.

I've been thinking about this and if I do make it the default, it will
mean warnings show up on other CI systems and layer maintainers will
get patches or complaints about the warnings. I'm not sure I really
want to get into this.

I do think it is something the project should be doing but I don't want
to burn out our existing maintainers. Since there isn't wide community
buy in, I suspect I should just drop the idea.

Cheers,

Richard

RE: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Peter Kjellerstedt]

> -----Original Message-----
> From: openembedded-core@lists.openembedded.org <openembedded-
> core@lists.openembedded.org> On Behalf Of Richard Purdie
> Sent: den 21 januari 2023 00:01
> To: Alexander Kanavin <alex.kanavin@gmail.com>; Bruce Ashfield
> <bruce.ashfield@gmail.com>
> Cc: Ross Burton <Ross.Burton@arm.com>; OE-core <openembedded-
> core@lists.openembedded.org>
> Subject: Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error
> handling
> 
> On Fri, 2023-01-20 at 20:38 +0100, Alexander Kanavin wrote:
> > On Fri, 20 Jan 2023 at 20:29, Bruce Ashfield <bruce.ashfield@gmail.com> wrote:
> > > Because I'm simply not going to insist on it in all the patches. I
> > > need all the contributions I can get, and I'm not going to
> > > pedantically insist on that.
> > >
> > > meta-virt is not oe-core, I do the lifting. Therefore, if bitbake
> > > errors, I have to fix it.
> >
> > But you do not need to insist on the needed metadata or fix it after
> > the fact. Bitbake will do the insisting for you, when contributors
> > test the change locally *before* they send it to you. If bitbake
> > errors on your side, this means they never built their contribution,
> > and you should raise a concern for that reason, and not for the
> > missing metadata.
> 
> It isn't that simple since this is a configurable QA warning, all it
> takes is one layer/distro to disable it and it is disabled for all
> layers that user works on.
> 
> This is why "core" is a separate config to "noncore" but we can't have
> a config for every layer and even if we did, people would still turn it
> off.

Rather than having separate QA tests for "patch-status-core" and 
"patch-status-noncore", couldn't we have a single "patch-status" and then 
configure it using a separate variable that specifies the layers that 
require the Upstream-Status trailer? Then each layer with this requirement 
can add itself in its layer.conf file and thus it is up to the maintainer 
to decide whether they want it or not.

> If it is turned off, it means people send patches and Bruce has to fix
> them, or ask them to resubmit which is extra overhead to the
> maintainer.
> 
> I've been thinking about this and if I do make it the default, it will
> mean warnings show up on other CI systems and layer maintainers will
> get patches or complaints about the warnings. I'm not sure I really
> want to get into this.
> 
> I do think it is something the project should be doing but I don't want
> to burn out our existing maintainers. Since there isn't wide community
> buy in, I suspect I should just drop the idea.
> 
> Cheers,
> 
> Richard

//Peter

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Richard Purdie]

On Sun, 2023-01-22 at 12:46 +0000, Peter Kjellerstedt wrote:
> > -----Original Message-----
> > From: openembedded-core@lists.openembedded.org <openembedded-
> > core@lists.openembedded.org> On Behalf Of Richard Purdie
> > Sent: den 21 januari 2023 00:01
> > To: Alexander Kanavin <alex.kanavin@gmail.com>; Bruce Ashfield
> > <bruce.ashfield@gmail.com>
> > Cc: Ross Burton <Ross.Burton@arm.com>; OE-core <openembedded-
> > core@lists.openembedded.org>
> > Subject: Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error
> > handling
> > 
> > On Fri, 2023-01-20 at 20:38 +0100, Alexander Kanavin wrote:
> > > On Fri, 20 Jan 2023 at 20:29, Bruce Ashfield <bruce.ashfield@gmail.com> wrote:
> > > > Because I'm simply not going to insist on it in all the patches. I
> > > > need all the contributions I can get, and I'm not going to
> > > > pedantically insist on that.
> > > > 
> > > > meta-virt is not oe-core, I do the lifting. Therefore, if bitbake
> > > > errors, I have to fix it.
> > > 
> > > But you do not need to insist on the needed metadata or fix it after
> > > the fact. Bitbake will do the insisting for you, when contributors
> > > test the change locally *before* they send it to you. If bitbake
> > > errors on your side, this means they never built their contribution,
> > > and you should raise a concern for that reason, and not for the
> > > missing metadata.
> > 
> > It isn't that simple since this is a configurable QA warning, all it
> > takes is one layer/distro to disable it and it is disabled for all
> > layers that user works on.
> > 
> > This is why "core" is a separate config to "noncore" but we can't have
> > a config for every layer and even if we did, people would still turn it
> > off.
> 
> Rather than having separate QA tests for "patch-status-core" and 
> "patch-status-noncore", couldn't we have a single "patch-status" and then 
> configure it using a separate variable that specifies the layers that
> require the Upstream-Status trailer? Then each layer with this requirement 
> can add itself in its layer.conf file and thus it is up to the maintainer 
> to decide whether they want it or not.

Even now, the QA warning/error code isn't entirely straight forward and
having the two categories keeps things simple and means we don't need
some new mechanism.

What you describe is possible, but there is a lot more runtime
computation overhead, which will further impact parsing time since
knowing what data to put into the task hashes and what not to put in
becomes more complicated.

I was hoping something simpler would suffice. I don't think I have a
lot of interest in going beyond this, particularly given the likely
impacts, both code wise and socially. If people don't want to do this I
am really running low on the energy to try and push it forward. Making
changes is hard, the socialising acceptance of a patch is the piece
many people overlook and it isn't here in this case.

Cheers,

Richard

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Mikko Rapeli]

Hi,

On Sun, Jan 22, 2023 at 10:19:29PM +0000, Richard Purdie wrote:
> I was hoping something simpler would suffice. I don't think I have a
> lot of interest in going beyond this, particularly given the likely
> impacts, both code wise and socially. If people don't want to do this I
> am really running low on the energy to try and push it forward. Making
> changes is hard, the socialising acceptance of a patch is the piece
> many people overlook and it isn't here in this case.

I'm requesting Upstream-Status to be filled to every patch file in
reviews. There is a lot of value in this information when maintaining
layers for a longer time. I want this. Everyone I speak with understands
how important this information is and that it's better to figure things
out or at least trigger discussion with upstream projects before
accepting patches.

-Mikko

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Alexander Kanavin]

On Mon, 23 Jan 2023 at 09:08, Mikko Rapeli <mikko.rapeli@linaro.org> wrote:
> > I was hoping something simpler would suffice. I don't think I have a
> > lot of interest in going beyond this, particularly given the likely
> > impacts, both code wise and socially. If people don't want to do this I
> > am really running low on the energy to try and push it forward. Making
> > changes is hard, the socialising acceptance of a patch is the piece
> > many people overlook and it isn't here in this case.
>
> I'm requesting Upstream-Status to be filled to every patch file in
> reviews. There is a lot of value in this information when maintaining
> layers for a longer time. I want this. Everyone I speak with understands
> how important this information is and that it's better to figure things
> out or at least trigger discussion with upstream projects before
> accepting patches.

Perhaps you and Peter could prototype the per-layer option then?

Alex

Re: [OE-core] [PATCH 1/3] insane: Improve patch warning/error handling

[Luca Ceresoli]

Hello,

On Thu, 19 Jan 2023 10:24:23 +0100
"Luca Ceresoli via lists.openembedded.org"
<luca.ceresoli=bootlin.com@lists.openembedded.org> wrote:

> Hello Richard,
> 
> On Wed, 18 Jan 2023 14:22:19 +0000
> "Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:
> 
> > 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
> >  * Enables warnings for missing Upstream-Status in non-core layer patches by default
> > 
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> >  meta/classes-global/insane.bbclass | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/meta/classes-global/insane.bbclass b/meta/classes-global/insane.bbclass
> > index ada8a7ef4e4..e1295f85392 100644
> > --- a/meta/classes-global/insane.bbclass
> > +++ b/meta/classes-global/insane.bbclass
> > @@ -29,11 +29,12 @@
> >  WARN_QA ?= " libdir xorg-driver-abi buildpaths \
> >              textrel incompatible-license files-invalid \
> >              infodir build-deps src-uri-bad symlink-to-sysroot multilib \
> > -            invalid-packageconfig host-user-contaminated uppercase-pn patch-fuzz \
> > +            invalid-packageconfig host-user-contaminated uppercase-pn \
> >              mime mime-xdg unlisted-pkg-lics unhandled-features-check \
> >              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 \  
> 
> AB testing with this patch applied revealed a few Upstream-Status
> warnings:
> 
> - 5 in meta-agl-core:
>   https://autobuilder.yoctoproject.org/typhoon/#/builders/120/builds/2272/steps/13/logs/stdio
> 
> - 3 in meta-intel:
>   https://autobuilder.yoctoproject.org/typhoon/#/builders/100/builds/3968/steps/13/logs/stdio

And I missed:

 - 9 in meta-virtualization:
   https://autobuilder.yoctoproject.org/typhoon/#/builders/128/builds/1168/steps/13/logs/stdio

> Jan-Simon, Anuj, can you look at those patches and maybe fix the Upstream-Status?

Bruce is in the list too now :)

Apologies for the noise.

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com