| Message ID | 20230118142221.1926666-1-richard.purdie@linuxfoundation.org |
|---|---|
| State | Accepted, archived |
| Commit | 76a685bfcf927593eac67157762a53259089ea8a |
| Headers | show |
| Series | [1/3] insane: Improve patch warning/error handling | expand |
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.
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.
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
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 > > >
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
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
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
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
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
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
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
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
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
> -----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
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
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
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
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() {
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(-)