[OE-Core][PATCH v3 0/2] testimage: enable artifacts retrieval for failed tests

[OE-Core][PATCH v3 0/2] testimage: enable artifacts retrieval for failed tests

[Alexis Lothoré]

Hello,
this small series is a new version of failed ptests artifacts retrieval
series.
The previous version can be found in [1]. This new version just fixes
commit message as well as a patchtest error (commit subject length).
With this series:
- "standard" images running tests leads to default artifact list being
  stored whenever a test fails
- if the image has ptest feature enabled, failed ptests directories will be
  retrieved
- artifacts lists can be customized: TESTIMAGE_FAILED_QA_ARTIFACTS:append =
  " <file1> <file2>..."

The feature has been tested with core-image-sato and core-image-ptest-fast
(running only a subtests of configured tests)

[1] https://lore.kernel.org/openembedded-core/20240203092538.5491-1-alexis.lothore@bootlin.com/

Alexis Lothoré (2):
  testimage: move TESTIMAGE_FAILED_QA_ARTIFACTS default to core-image
  core-image-ptest: retrieve ptests directory when ptests fail

 meta/classes-recipe/core-image.bbclass       | 8 ++++++++
 meta/classes-recipe/testimage.bbclass        | 9 ---------
 meta/recipes-core/images/core-image-ptest.bb | 2 ++
 3 files changed, 10 insertions(+), 9 deletions(-)

-- 
2.43.0

[OE-Core][PATCH v3 1/2] testimage: move TESTIMAGE_FAILED_QA_ARTIFACTS default to core-image

[Alexis Lothoré]

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.
-- 
2.43.0

Re: [OE-Core][PATCH v3 1/2] testimage: move TESTIMAGE_FAILED_QA_ARTIFACTS default to core-image

[Richard Purdie]

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

Re: [OE-Core][PATCH v3 1/2] testimage: move TESTIMAGE_FAILED_QA_ARTIFACTS default to core-image

[Alexis Lothoré]

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
> 
> 

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [OE-Core][PATCH v3 1/2] testimage: move TESTIMAGE_FAILED_QA_ARTIFACTS default to core-image

[Richard Purdie]

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

Re: [OE-Core][PATCH v3 1/2] testimage: move TESTIMAGE_FAILED_QA_ARTIFACTS default to core-image

[Alexis Lothoré]

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.

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [OE-Core][PATCH v3 1/2] testimage: move TESTIMAGE_FAILED_QA_ARTIFACTS default to core-image

[Richard Purdie]

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

Re: [OE-Core][PATCH v3 1/2] testimage: move TESTIMAGE_FAILED_QA_ARTIFACTS default to core-image

[Alexis Lothoré]

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

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[OE-Core][PATCH v3 2/2] core-image-ptest: retrieve ptests directory when ptests fail

[Alexis Lothoré]

From: Alexis Lothoré <alexis.lothore@bootlin.com>

TESTIMAGE_FAILED_QA_ARTIFACTS is set with a default, minimal list of files
to retrieve whenever a runtime test fails. By appending the ptest directory
only in core-image-ptest.bb, thanks to multiconfig feature used in the
recipe, only failing ptests will lead to corresponding ptest artifacts
retrieval, instead of all ptests artifacts retrieval.

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
This patch was present in the original artifacts series ([1])

Changes in v3:
- slightly reword commit message body

Changes in v1 (since being picked in new series)
- use += instead of append syntax

[1] https://lore.kernel.org/openembedded-core/20230609064802.11777-4-alexis.lothore@bootlin.com/
---
 meta/recipes-core/images/core-image-ptest.bb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/meta/recipes-core/images/core-image-ptest.bb b/meta/recipes-core/images/core-image-ptest.bb
index b6f5c2fd6049..fb96c542c0a3 100644
--- a/meta/recipes-core/images/core-image-ptest.bb
+++ b/meta/recipes-core/images/core-image-ptest.bb
@@ -43,3 +43,5 @@ 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"
-- 
2.43.0