| Message ID | 20250728095936.1495441-4-philip.lorenz@bmw.de |
|---|---|
| State | New |
| Headers | show |
| Series | Improve handling of build qa issues | expand |
On Mon, 2025-07-28 at 11:59 +0200, Philip Lorenz via lists.openembedded.org wrote: > So far, failing the build when a QA error was raised happened implicitly > when another (unrelated) caller called `exit_if_errors` (e.g. > insane.bbclass's anonymous Python function). This is error prone and may > also lead to inconsistent abortion behaviour when parsing QA errors are > only raised later. > > Fix this by introducing an explicit call to `exit_if_errors` once recipe > parsing has finished. > > Currently code in the following .bbclasses relies on insane.bbclass to > call `exit_if_errors`: > * license.bbclass > * yocto-check-layer.bbclass > > Signed-off-by: Philip Lorenz <philip.lorenz@bmw.de> > --- > meta/classes-global/base.bbclass | 4 ++++ > meta/classes-global/insane.bbclass | 2 -- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/meta/classes-global/base.bbclass b/meta/classes-global/base.bbclass > index 6be1f5c2df2..6fe37755ffa 100644 > --- a/meta/classes-global/base.bbclass > +++ b/meta/classes-global/base.bbclass > @@ -348,6 +348,10 @@ python base_eventhandler() { > profprov = d.getVar("PREFERRED_PROVIDER_" + p) > if profprov and pn != profprov: > raise bb.parse.SkipRecipe("PREFERRED_PROVIDER_%s set to %s, not %s" % (p, profprov, pn)) > + > + # Bail out early if parsing raised any QA errors > + oe.qa.exit_if_errors(d) > + > } > > CONFIGURESTAMPFILE = "${WORKDIR}/configure.sstate" > diff --git a/meta/classes-global/insane.bbclass b/meta/classes-global/insane.bbclass > index fed8163c3e0..f0b7f83e51d 100644 > --- a/meta/classes-global/insane.bbclass > +++ b/meta/classes-global/insane.bbclass > @@ -1627,6 +1627,4 @@ python () { > if broken_order: > oe.qa.handle_error("native-last", "%s: native/nativesdk class is not inherited last, this can result in unexpected behaviour. " > "Classes inherited after native/nativesdk: %s" % (pn, " ".join(broken_order)), d) > - > - oe.qa.exit_if_errors(d) > } I can understand why you've sent this patch but I'm not sure it fits with the overall design we were trying to aim for. The aim has been for the "QA tests" to be together in insane.bbclass and moving this call to base.bbclass just means we're accepting we'll just splatter these QA checks everywhere. I'd also note that parse time checks are expensive and we really should be trying to avoid them it at all possible. This is just going to encourage them :/. I'd like to understand a bit more about the issues you saw which lead to this patch... Cheers, Richard
Hi Richard, On 31.07.25 10:45, Richard Purdie wrote: > On Mon, 2025-07-28 at 11:59 +0200, Philip Lorenz via lists.openembedded.org wrote: >> So far, failing the build when a QA error was raised happened implicitly >> when another (unrelated) caller called `exit_if_errors` (e.g. >> insane.bbclass's anonymous Python function). This is error prone and may >> also lead to inconsistent abortion behaviour when parsing QA errors are >> only raised later. >> >> Fix this by introducing an explicit call to `exit_if_errors` once recipe >> parsing has finished. >> >> Currently code in the following .bbclasses relies on insane.bbclass to >> call `exit_if_errors`: >> * license.bbclass >> * yocto-check-layer.bbclass >> >> Signed-off-by: Philip Lorenz <philip.lorenz@bmw.de> >> --- >> meta/classes-global/base.bbclass | 4 ++++ >> meta/classes-global/insane.bbclass | 2 -- >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/meta/classes-global/base.bbclass b/meta/classes-global/base.bbclass >> index 6be1f5c2df2..6fe37755ffa 100644 >> --- a/meta/classes-global/base.bbclass >> +++ b/meta/classes-global/base.bbclass >> @@ -348,6 +348,10 @@ python base_eventhandler() { >> profprov = d.getVar("PREFERRED_PROVIDER_" + p) >> if profprov and pn != profprov: >> raise bb.parse.SkipRecipe("PREFERRED_PROVIDER_%s set to %s, not %s" % (p, profprov, pn)) >> + >> + # Bail out early if parsing raised any QA errors >> + oe.qa.exit_if_errors(d) >> + >> } >> >> CONFIGURESTAMPFILE = "${WORKDIR}/configure.sstate" >> diff --git a/meta/classes-global/insane.bbclass b/meta/classes-global/insane.bbclass >> index fed8163c3e0..f0b7f83e51d 100644 >> --- a/meta/classes-global/insane.bbclass >> +++ b/meta/classes-global/insane.bbclass >> @@ -1627,6 +1627,4 @@ python () { >> if broken_order: >> oe.qa.handle_error("native-last", "%s: native/nativesdk class is not inherited last, this can result in unexpected behaviour. " >> "Classes inherited after native/nativesdk: %s" % (pn, " ".join(broken_order)), d) >> - >> - oe.qa.exit_if_errors(d) >> } > I can understand why you've sent this patch but I'm not sure it fits > with the overall design we were trying to aim for. > > The aim has been for the "QA tests" to be together in insane.bbclass > and moving this call to base.bbclass just means we're accepting we'll > just splatter these QA checks everywhere. > > I'd also note that parse time checks are expensive and we really should > be trying to avoid them it at all possible. This is just going to > encourage them :/. > > I'd like to understand a bit more about the issues you saw which lead > to this patch... The origin of this patch series is issues related to missing calls to `exit_if_errors` that we saw on kirkstone which lead to a complete analysis of all uses `oe.qa.handle_error` in poky that are missing a corresponding call to `exit_if_errors`. In this analysis it turned out that both `license.py:check_license_format` (via `base.bbclass`) and `yocto-check-layer.bbclass` currently rely on some other part in the system to do this for them (i.e. insane.bbclass). I guess that the call to check_license_format could actually be moved to do_recipe_qa and `yocto-check-layer.bbclass` could be extended to explicitly call `exit_if_errors` to avoid reliance on some other anonymous function to call it at a later stage. If that makes sense I can send out a revised patch. What would be the advise for custom recipe QA checks to hook themselves into the build? Injecting a custom function into the task's prefuncs? Philip
diff --git a/meta/classes-global/base.bbclass b/meta/classes-global/base.bbclass index 6be1f5c2df2..6fe37755ffa 100644 --- a/meta/classes-global/base.bbclass +++ b/meta/classes-global/base.bbclass @@ -348,6 +348,10 @@ python base_eventhandler() { profprov = d.getVar("PREFERRED_PROVIDER_" + p) if profprov and pn != profprov: raise bb.parse.SkipRecipe("PREFERRED_PROVIDER_%s set to %s, not %s" % (p, profprov, pn)) + + # Bail out early if parsing raised any QA errors + oe.qa.exit_if_errors(d) + } CONFIGURESTAMPFILE = "${WORKDIR}/configure.sstate" diff --git a/meta/classes-global/insane.bbclass b/meta/classes-global/insane.bbclass index fed8163c3e0..f0b7f83e51d 100644 --- a/meta/classes-global/insane.bbclass +++ b/meta/classes-global/insane.bbclass @@ -1627,6 +1627,4 @@ python () { if broken_order: oe.qa.handle_error("native-last", "%s: native/nativesdk class is not inherited last, this can result in unexpected behaviour. " "Classes inherited after native/nativesdk: %s" % (pn, " ".join(broken_order)), d) - - oe.qa.exit_if_errors(d) }
So far, failing the build when a QA error was raised happened implicitly when another (unrelated) caller called `exit_if_errors` (e.g. insane.bbclass's anonymous Python function). This is error prone and may also lead to inconsistent abortion behaviour when parsing QA errors are only raised later. Fix this by introducing an explicit call to `exit_if_errors` once recipe parsing has finished. Currently code in the following .bbclasses relies on insane.bbclass to call `exit_if_errors`: * license.bbclass * yocto-check-layer.bbclass Signed-off-by: Philip Lorenz <philip.lorenz@bmw.de> --- meta/classes-global/base.bbclass | 4 ++++ meta/classes-global/insane.bbclass | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-)