| Message ID | 20240205143757.81826-2-alexis.lothore@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | testimage: enable artifacts retrieval for failed tests | expand |
On Mon, 2024-02-05 at 15:37 +0100, Alexis Lothoré via lists.openembedded.org wrote: > From: Alexis Lothoré <alexis.lothore@bootlin.com> > > TESTIMAGE_FAILED_QA_ARTIFACTS currently sets a default list of files to be > saved whenever some tests fail. Unfortunately, this default value is very > easily discarded, because TESTIMAGE_FAILED_QA_ARTIFACTS is expected to be > enriched by some core recipes (example: ptest images) which are often > parsed before testimage.bbclass. Those core recipes could still manage to > get the default value AND add some files by using override syntax, but then > it makes it difficult for downstream recipes or bbappend files to tune this > variable. > > Allow to set this default value and make sure it is not overwritten by > recipes wanting to tune it, by setting the default value in core-image, > which is parsed early enough. While doing so, set it as a default value > instead of a weak default value. > > Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com> > --- > Changes in v3: > - fix commit subject length and content > > Changes in v2: > - set the default value in core-image instead of core-image-minimal > --- > meta/classes-recipe/core-image.bbclass | 8 ++++++++ > meta/classes-recipe/testimage.bbclass | 9 --------- > 2 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/meta/classes-recipe/core-image.bbclass b/meta/classes-recipe/core-image.bbclass > index 40fc15cb04f2..8ca64a95a318 100644 > --- a/meta/classes-recipe/core-image.bbclass > +++ b/meta/classes-recipe/core-image.bbclass > @@ -83,4 +83,12 @@ CORE_IMAGE_EXTRA_INSTALL ?= "" > > IMAGE_INSTALL ?= "${CORE_IMAGE_BASE_INSTALL}" > > +# When any test fails, TESTIMAGE_FAILED_QA_ARTIFACTS will be parsed and for > +# each entry in it, if artifact pointed by path description exists on target, > +# it will be retrieved onto host > +TESTIMAGE_FAILED_QA_ARTIFACTS ?= "\ > + ${localstatedir}/log \ > + ${sysconfdir}/version \ > + ${sysconfdir}/os-release" > + > inherit image > diff --git a/meta/classes-recipe/testimage.bbclass b/meta/classes-recipe/testimage.bbclass > index f36d9418914f..cfda5b631ba8 100644 > --- a/meta/classes-recipe/testimage.bbclass > +++ b/meta/classes-recipe/testimage.bbclass > @@ -18,15 +18,6 @@ inherit image-artifact-names > > TESTIMAGE_AUTO ??= "0" > > -# When any test fails, TESTIMAGE_FAILED_QA ARTIFACTS will be parsed and for > -# each entry in it, if artifact pointed by path description exists on target, > -# it will be retrieved onto host > - > -TESTIMAGE_FAILED_QA_ARTIFACTS ??= "\ > - ${localstatedir}/log \ > - ${sysconfdir}/version \ > - ${sysconfdir}/os-release" > - > # You can set (or append to) TEST_SUITES in local.conf to select the tests > # which you want to run for your target. > # The test names are the module names in meta/lib/oeqa/runtime/cases. I can imagine why ??= breaks if you use +=. Does this work if you change it from ??= to just = ? I'd really prefer to keep this in testimage.bbclass. Recipes should be able to use += if it is set with = and the addition is after the class inherit. Cheers, Richard
Hello Richard, thanks for the fast review On 2/5/24 16:08, Richard Purdie wrote: > On Mon, 2024-02-05 at 15:37 +0100, Alexis Lothoré via > lists.openembedded.org wrote: [...] >> -TESTIMAGE_FAILED_QA_ARTIFACTS ??= "\ >> - ${localstatedir}/log \ >> - ${sysconfdir}/version \ >> - ${sysconfdir}/os-release" >> - >> # You can set (or append to) TEST_SUITES in local.conf to select the tests >> # which you want to run for your target. >> # The test names are the module names in meta/lib/oeqa/runtime/cases. > > I can imagine why ??= breaks if you use +=. > > Does this work if you change it from ??= to just = ? I've indeed given a try to this before moving TESTIMAGE_FAILED_QA_ARTIFACTS to core-image, but when using "=" instead of "?=" or "??=", I observe that the part affecting TESTIMAGE_FAILED_QA_ARTIFACTS in core-image-ptest is not taken, so we are missing the ptests artifacts (but we still get the generic artifacts defined in testimage.bbclass). So to summarize: - "??=" or "?=" : we only get ptests artifacts, not the "generic" one (/etc/version, /var/log, etc) - "=": we only get the generic artifacts, not the ptests one. It is not clear for me why the second one fails, and TESTIMAGE_FAILED_QA_ARTIFACTS does not appear at all in 'bitbake -e" output, which makes it even more cryptic to me. > I'd really prefer to keep this in testimage.bbclass.Recipes should be > able to use += if it is set with = and the addition is after the class > inherit. Based on my understanding of bitbake manual, I theoretically agree with you, yet, as mentioned above this is not the behavior I observe, so I may be missing something important here. Maybe the parsing order is not the one I think I am observing, maybe its multiconfig, maybe something else. I can try to dig this further and clarify why it does not work as expected with "=" Alexis > > Cheers, > > Richard > >
On Mon, 2024-02-05 at 20:22 +0100, Alexis Lothoré wrote: > Hello Richard, thanks for the fast review > > On 2/5/24 16:08, Richard Purdie wrote: > > On Mon, 2024-02-05 at 15:37 +0100, Alexis Lothoré via > > lists.openembedded.org wrote: > > [...] > > > > -TESTIMAGE_FAILED_QA_ARTIFACTS ??= "\ > > > - ${localstatedir}/log \ > > > - ${sysconfdir}/version \ > > > - ${sysconfdir}/os-release" > > > - > > > # You can set (or append to) TEST_SUITES in local.conf to select the tests > > > # which you want to run for your target. > > > # The test names are the module names in meta/lib/oeqa/runtime/cases. > > > > I can imagine why ??= breaks if you use +=. > > > > Does this work if you change it from ??= to just = ? > > I've indeed given a try to this before moving TESTIMAGE_FAILED_QA_ARTIFACTS to > core-image, but when using "=" instead of "?=" or "??=", I observe that the part > affecting TESTIMAGE_FAILED_QA_ARTIFACTS in core-image-ptest is not taken, so we > are missing the ptests artifacts (but we still get the generic artifacts defined > in testimage.bbclass). So to summarize: > - "??=" or "?=" : we only get ptests artifacts, not the "generic" one > (/etc/version, /var/log, etc) > - "=": we only get the generic artifacts, not the ptests one. > It is not clear for me why the second one fails, and > TESTIMAGE_FAILED_QA_ARTIFACTS does not appear at all in 'bitbake -e" output, > which makes it even more cryptic to me. > > > I'd really prefer to keep this in testimage.bbclass.Recipes should be > > able to use += if it is set with = and the addition is after the class > > inherit. > > Based on my understanding of bitbake manual, I theoretically agree with you, > yet, as mentioned above this is not the behavior I observe, so I may be missing > something important here. Maybe the parsing order is not the one I think I am > observing, maybe its multiconfig, maybe something else. > I can try to dig this further and clarify why it does not work as expected with "=" I would like to better understand why this isn't doing what we expect... Cheers, Richard
On 2/5/24 20:22, Alexis Lothoré wrote: > Hello Richard, thanks for the fast review > > On 2/5/24 16:08, Richard Purdie wrote: >> On Mon, 2024-02-05 at 15:37 +0100, Alexis Lothoré via >> lists.openembedded.org wrote: > > [...] > >>> -TESTIMAGE_FAILED_QA_ARTIFACTS ??= "\ >>> - ${localstatedir}/log \ >>> - ${sysconfdir}/version \ >>> - ${sysconfdir}/os-release" >>> - >>> # You can set (or append to) TEST_SUITES in local.conf to select the tests >>> # which you want to run for your target. >>> # The test names are the module names in meta/lib/oeqa/runtime/cases. >> >> I can imagine why ??= breaks if you use +=. >> >> Does this work if you change it from ??= to just = ? > > I've indeed given a try to this before moving TESTIMAGE_FAILED_QA_ARTIFACTS to > core-image, but when using "=" instead of "?=" or "??=", I observe that the part > affecting TESTIMAGE_FAILED_QA_ARTIFACTS in core-image-ptest is not taken, so we > are missing the ptests artifacts (but we still get the generic artifacts defined > in testimage.bbclass). So to summarize: > - "??=" or "?=" : we only get ptests artifacts, not the "generic" one > (/etc/version, /var/log, etc) > - "=": we only get the generic artifacts, not the ptests one. > It is not clear for me why the second one fails, and > TESTIMAGE_FAILED_QA_ARTIFACTS does not appear at all in 'bitbake -e" output, > which makes it even more cryptic to me. > >> I'd really prefer to keep this in testimage.bbclass.Recipes should be >> able to use += if it is set with = and the addition is after the class >> inherit. > > Based on my understanding of bitbake manual, I theoretically agree with you, > yet, as mentioned above this is not the behavior I observe, so I may be missing > something important here. Maybe the parsing order is not the one I think I am > observing, maybe its multiconfig, maybe something else. > I can try to dig this further and clarify why it does not work as expected with "=" So I've did multiple attempts to make this work, and I see no way to define the default value in testimage.bbclass with "=", and add content to it in core-image-ptest.bbwith "+=". My understanding is that testimage is always parsed after core-image-ptest.bb, so there is no nice way to set its default value in there: - "=" will always overwrite ptests artifacts set with "+=" - "?=" or "??=" will always be overwritten by ptests artifacts set with "+=" My configuration relies on IMAGE_CLASSES += 'testimage', which is also done in CI. Still, I understand your concern about keeping default value in testimage.bbclass. The only compromise I see here to both keep the default value in testimage and be able to override it at user level is not to enrich it in core-image-ptest but to redefine it: diff --git a/meta/recipes-core/images/core-image-ptest.bb b/meta/recipes-core/images/core-image-ptest.bb index fb96c542c0a3..4a90181afd83 100644 --- a/meta/recipes-core/images/core-image-ptest.bb +++ b/meta/recipes-core/images/core-image-ptest.bb @@ -43,5 +43,8 @@ python () { raise bb.parse.SkipRecipe("No class extension set") } -# Include ptest directory in artifacts to retrieve if there is a failed test -TESTIMAGE_FAILED_QA_ARTIFACTS += "${libdir}/${MCNAME}/ptest" +TESTIMAGE_FAILED_QA_ARTIFACTS = "\ + ${localstatedir}/log \ + ${sysconfdir}/version \ + ${sysconfdir}/os-release \ + ${libdir}/${MCNAME}/ptest" That's for example what is done with TEST_SUITES. In this setup, we can keep a default value in testimage, and override it as needed with append/prepend/remove syntax at user level. I'll wait a bit in case there's any comment or concern about this approach, and then send a new version.
On Tue, 2024-02-06 at 12:09 +0100, Alexis Lothoré wrote: > So I've did multiple attempts to make this work, and I see no way to define the > default value in testimage.bbclass with "=", and add content to it in > core-image-ptest.bbwith "+=". My understanding is that testimage is always > parsed after core-image-ptest.bb, so there is no nice way to set its default > value in there: > - "=" will always overwrite ptests artifacts set with "+=" > - "?=" or "??=" will always be overwritten by ptests artifacts set with "+=" > My configuration relies on IMAGE_CLASSES += 'testimage', which is also done in CI. The issue is this: classes-recipe/image.bbclass:inherit_defer ${IMGCLASSES} since this is deferred to after the recipe is parsed. > Still, I understand your concern about keeping default value in > testimage.bbclass. The only compromise I see here to both keep the default value > in testimage and be able to override it at user level is not to enrich it in > core-image-ptest but to redefine it: > > diff --git a/meta/recipes-core/images/core-image-ptest.bb > b/meta/recipes-core/images/core-image-ptest.bb > index fb96c542c0a3..4a90181afd83 100644 > --- a/meta/recipes-core/images/core-image-ptest.bb > +++ b/meta/recipes-core/images/core-image-ptest.bb > @@ -43,5 +43,8 @@ python () { > raise bb.parse.SkipRecipe("No class extension set") > } > > -# Include ptest directory in artifacts to retrieve if there is a failed test > -TESTIMAGE_FAILED_QA_ARTIFACTS += "${libdir}/${MCNAME}/ptest" > +TESTIMAGE_FAILED_QA_ARTIFACTS = "\ > + ${localstatedir}/log \ > + ${sysconfdir}/version \ > + ${sysconfdir}/os-release \ > + ${libdir}/${MCNAME}/ptest" > > That's for example what is done with TEST_SUITES. In this setup, we can keep a > default value in testimage, and override it as needed with append/prepend/remove > syntax at user level. > > I'll wait a bit in case there's any comment or concern about this approach, and > then send a new version. I see two options: a) Make testimage.bbclass a += or b) Put the new entry into testimage.bbclass even if it is ptest specific. I'm leaning to doing both, make it a += and just put it there by default in case someone includes ptests in a non ptest image. Cheers, Richard
On 2/6/24 13:00, Richard Purdie wrote: > On Tue, 2024-02-06 at 12:09 +0100, Alexis Lothoré wrote: >> So I've did multiple attempts to make this work, and I see no way to define the >> default value in testimage.bbclass with "=", and add content to it in >> core-image-ptest.bbwith "+=". My understanding is that testimage is always >> parsed after core-image-ptest.bb, so there is no nice way to set its default >> value in there: >> - "=" will always overwrite ptests artifacts set with "+=" >> - "?=" or "??=" will always be overwritten by ptests artifacts set with "+=" >> My configuration relies on IMAGE_CLASSES += 'testimage', which is also done in CI. > > The issue is this: > > classes-recipe/image.bbclass:inherit_defer ${IMGCLASSES} > > since this is deferred to after the recipe is parsed. Ah, thank you for the explanation, I was not aware of this defer mechanism. >> Still, I understand your concern about keeping default value in >> testimage.bbclass. The only compromise I see here to both keep the default value >> in testimage and be able to override it at user level is not to enrich it in >> core-image-ptest but to redefine it: >> >> diff --git a/meta/recipes-core/images/core-image-ptest.bb >> b/meta/recipes-core/images/core-image-ptest.bb >> index fb96c542c0a3..4a90181afd83 100644 >> --- a/meta/recipes-core/images/core-image-ptest.bb >> +++ b/meta/recipes-core/images/core-image-ptest.bb >> @@ -43,5 +43,8 @@ python () { >> raise bb.parse.SkipRecipe("No class extension set") >> } >> >> -# Include ptest directory in artifacts to retrieve if there is a failed test >> -TESTIMAGE_FAILED_QA_ARTIFACTS += "${libdir}/${MCNAME}/ptest" >> +TESTIMAGE_FAILED_QA_ARTIFACTS = "\ >> + ${localstatedir}/log \ >> + ${sysconfdir}/version \ >> + ${sysconfdir}/os-release \ >> + ${libdir}/${MCNAME}/ptest" >> >> That's for example what is done with TEST_SUITES. In this setup, we can keep a >> default value in testimage, and override it as needed with append/prepend/remove >> syntax at user level. >> >> I'll wait a bit in case there's any comment or concern about this approach, and >> then send a new version. > > I see two options: > > a) Make testimage.bbclass a += > > or > > b) Put the new entry into testimage.bbclass even if it is ptest > specific. > > > I'm leaning to doing both, make it a += and just put it there by > default in case someone includes ptests in a non ptest image. Ok, sounds good to me too and ptests artifact retrieval seems to behave properly even when defined in testimage. I'll submit a new version which keeps the default artifact list in testimage, and adds immediately ptests artifacts to it. Thanks for the guidance :) Alexis
diff --git a/meta/classes-recipe/core-image.bbclass b/meta/classes-recipe/core-image.bbclass index 40fc15cb04f2..8ca64a95a318 100644 --- a/meta/classes-recipe/core-image.bbclass +++ b/meta/classes-recipe/core-image.bbclass @@ -83,4 +83,12 @@ CORE_IMAGE_EXTRA_INSTALL ?= "" IMAGE_INSTALL ?= "${CORE_IMAGE_BASE_INSTALL}" +# When any test fails, TESTIMAGE_FAILED_QA_ARTIFACTS will be parsed and for +# each entry in it, if artifact pointed by path description exists on target, +# it will be retrieved onto host +TESTIMAGE_FAILED_QA_ARTIFACTS ?= "\ + ${localstatedir}/log \ + ${sysconfdir}/version \ + ${sysconfdir}/os-release" + inherit image diff --git a/meta/classes-recipe/testimage.bbclass b/meta/classes-recipe/testimage.bbclass index f36d9418914f..cfda5b631ba8 100644 --- a/meta/classes-recipe/testimage.bbclass +++ b/meta/classes-recipe/testimage.bbclass @@ -18,15 +18,6 @@ inherit image-artifact-names TESTIMAGE_AUTO ??= "0" -# When any test fails, TESTIMAGE_FAILED_QA ARTIFACTS will be parsed and for -# each entry in it, if artifact pointed by path description exists on target, -# it will be retrieved onto host - -TESTIMAGE_FAILED_QA_ARTIFACTS ??= "\ - ${localstatedir}/log \ - ${sysconfdir}/version \ - ${sysconfdir}/os-release" - # You can set (or append to) TEST_SUITES in local.conf to select the tests # which you want to run for your target. # The test names are the module names in meta/lib/oeqa/runtime/cases.