[OE-Core][PATCH v3 0/4] add failed tests artifacts retriever

[OE-Core][PATCH v3 0/4] add failed tests artifacts retriever

[alexis.lothore]

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

This series is a proposal to bring in an "artifact retriever" to ease
debugging when some runtime tests fails. This is a follow-up to
the v2 ([1]) and its corresponding RFC ([2]), which in turn is
a proposal to address general debugging issues like [3]

The main change is based on Alexander's suggestion to defer ptests
artifacts list in core-image-ptest.bb, which as a consequence will
automatically filter out succeeding ptests artifacts.
For this new revision which now relies on multiconfig, I stumbled upon
issues because of PTEST_EXPECT_FAILURE = "1" in core-image-ptest.bb. From
my understanding, this is used with unittest.expectFailure to prevent
tests suffering from intermittent failures from making general test session
fail systematically. To keep this behaviour, a new helper is needed to know
about those failing tests which are not accounted.

Changes since v2:
- add helper to know about any tolerated ptest failure
- allow to retrieve ptests artifacts only for failing ptests, by benefiting
  from the multiconfig feature in ptest images

Changes since v1:
- remove legacy scp option
- put back target stop in 'finally' clause
- retrieve artifacts only in nominal target run (ie no exception from
  ssh/qemu target run)
- list artifacts directly in variable instead of using intermediate file
- use standard variables in artifacts paths
- allow glob patterns in artifacts paths
- expand artifacts path on target before tryiong to retrieve them

[1] https://lore.kernel.org/openembedded-core/20230607083015.20760-1-alexis.lothore@bootlin.com/
[2] https://lore.kernel.org/openembedded-core/20230523161619a8c871d9@mail.local/T/#t
[3] https://bugzilla.yoctoproject.org/show_bug.cgi?id=14901

Alexis Lothoré (4):
  oeqa/core/runner: add helper to know about expected failures
  oeqa/target/ssh: update options for SCP
  testimage: implement test artifacts retriever for failing tests
  core-image-ptest: append ptest directory to artifacts list

 meta/classes-recipe/testimage.bbclass        | 48 ++++++++++++++++++++
 meta/lib/oeqa/core/runner.py                 |  4 ++
 meta/lib/oeqa/core/target/ssh.py             |  5 +-
 meta/recipes-core/images/core-image-ptest.bb |  1 +
 4 files changed, 57 insertions(+), 1 deletion(-)

-- 
2.40.1

[OE-Core][PATCH v3 1/4] oeqa/core/runner: add helper to know about expected failures

[alexis.lothore]

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

Testing framework currently uses the unittest.expectedFailure decorator for
tests that can have intermittent failures (see PTEST_EXPECT_FAILURE = "1")
in core-image-ptest.bb. While it allows upper layers to run tests without
failing on "fragile" tests, it prevents those from knowing more about those
failing tests since they are not accounting as failures (for example we
could want to retrieve some logs about failed tests to improve them, and
eventually to drop expectFailure decorator)

Add a helper to allow upper layers to know about those failures which won't
make global testing session

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 meta/lib/oeqa/core/runner.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/meta/lib/oeqa/core/runner.py b/meta/lib/oeqa/core/runner.py
index d50690ab37f8..5077eb8e3e32 100644
--- a/meta/lib/oeqa/core/runner.py
+++ b/meta/lib/oeqa/core/runner.py
@@ -229,6 +229,10 @@ class OETestResult(_TestResult):
         # Override as we unexpected successes aren't failures for us
         return (len(self.failures) == len(self.errors) == 0)
 
+    def hasAnyFailingTest(self):
+        # Account for expected failures
+        return not self.wasSuccessful() or len(self.expectedFailures)
+
 class OEListTestsResult(object):
     def wasSuccessful(self):
         return True
-- 
2.40.1

[OE-Core][PATCH v3 2/4] oeqa/target/ssh: update options for SCP

[alexis.lothore]

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

By default scp expects files. Passing -r option allows to copy directories
too

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
Changes since v1:
- drop legacy scp protocol option
---
 meta/lib/oeqa/core/target/ssh.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/meta/lib/oeqa/core/target/ssh.py b/meta/lib/oeqa/core/target/ssh.py
index 51079075b5bd..e650302052db 100644
--- a/meta/lib/oeqa/core/target/ssh.py
+++ b/meta/lib/oeqa/core/target/ssh.py
@@ -40,8 +40,11 @@ class OESSHTarget(OETarget):
                 '-o', 'StrictHostKeyChecking=no',
                 '-o', 'LogLevel=ERROR'
                 ]
+        scp_options = [
+                '-r'
+        ]
         self.ssh = ['ssh', '-l', self.user ] + ssh_options
-        self.scp = ['scp'] + ssh_options
+        self.scp = ['scp'] + ssh_options + scp_options
         if port:
             self.ssh = self.ssh + [ '-p', port ]
             self.scp = self.scp + [ '-P', port ]
-- 
2.40.1

[OE-Core][PATCH v3 3/4] testimage: implement test artifacts retriever for failing tests

[alexis.lothore]

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

Add a basic artifacts retrievers in testimage class which:
- triggers when at least one runtime test fails but tests execution
  encountered no major issue
- reads a list of paths to retrieve from TESTIMAGE_FAILED_QA_ARTIFACTS
- checks for artifacts presence on target
- retrieve those files over scp thanks to existing ssh class
- store those files in an "artifacts" directory in "tmp/log/oeqa/<image>"

This implementation assumes that the SSH or Qemu target has run and
finished gracefully. If tests do not finish because of an exception,
artifacts will not be retrieved

Bring partial solution to [YOCTO #14901]

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
Changes since v2:
- remove ptests directories from default artifact list, now managed in
  core-image-ptest.bb
- use new function to check for any failed ptest

Changes since v1:
- only gather artifacts in nominal case (ie qemu runs without any raised
  exception)
- list artifacts directly in variable instead of using external file
- use standard variables in artifacts paths
- allow glob patterns usage in artifacts paths
- expand/filter artifacts list on target before retrieving them
- tune default artifacts list
---
 meta/classes-recipe/testimage.bbclass | 48 +++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/meta/classes-recipe/testimage.bbclass b/meta/classes-recipe/testimage.bbclass
index b48cd96575d2..765184c18008 100644
--- a/meta/classes-recipe/testimage.bbclass
+++ b/meta/classes-recipe/testimage.bbclass
@@ -18,6 +18,15 @@ 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.
@@ -192,6 +201,39 @@ def get_testimage_boot_patterns(d):
                 boot_patterns[flag] = flagval.encode().decode('unicode-escape')
     return boot_patterns
 
+def get_artifacts_list(target, raw_list):
+    result = []
+    # Passed list may contains patterns in paths, expand them directly on target
+    for raw_path in raw_list.split():
+        cmd = f"for p in {raw_path}; do if [ -e $p ]; then echo $p; fi; done"
+        try:
+            status, output = target.run(cmd)
+            if status != 0 or not output:
+                raise Exception()
+            result += output.split()
+        except:
+            bb.warn(f"No file/directory matching path {raw_path}")
+
+    return result
+
+def retrieve_test_artifacts(target, artifacts_list, target_dir):
+    import shutil
+
+    local_artifacts_dir = os.path.join(target_dir, "artifacts")
+    if os.path.isdir(local_artifacts_dir):
+        shutil.rmtree(local_artifacts_dir)
+
+    os.makedirs(local_artifacts_dir)
+    for artifact_path in artifacts_list:
+        if not os.path.isabs(artifact_path):
+            bb.warn(f"{artifact_path} is not an absolute path")
+            continue
+        try:
+            dest_dir = os.path.join(local_artifacts_dir, os.path.dirname(artifact_path[1:]))
+            os.makedirs(dest_dir, exist_ok=True)
+            target.copyFrom(artifact_path, dest_dir)
+        except:
+            bb.warn(f"Can not retrieve {artifact_path} from test target")
 
 def testimage_main(d):
     import os
@@ -383,6 +425,12 @@ def testimage_main(d):
             pass
         results = tc.runTests()
         complete = True
+        if results.hasAnyFailingTest():
+            artifacts_list = get_artifacts_list(tc.target, d.getVar("TESTIMAGE_FAILED_QA_ARTIFACTS"))
+            if not artifacts_list:
+                bb.warn("Could not load artifacts list, skip artifacts retrieval")
+            else:
+                retrieve_test_artifacts(tc.target, artifacts_list, get_testimage_json_result_dir(d))
     except (KeyboardInterrupt, BlockingIOError) as err:
         if isinstance(err, KeyboardInterrupt):
             bb.error('testimage interrupted, shutting down...')
-- 
2.40.1

[OE-Core][PATCH v3 4/4] core-image-ptest: append ptest directory to artifacts list

[alexis.lothore]

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

TESTIMAGE_FAILED_QA_ARTIFACTS is defined in testimage.bbclass with a
minimal list of files to retrieve when a test fail. 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>
---
 meta/recipes-core/images/core-image-ptest.bb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/meta/recipes-core/images/core-image-ptest.bb b/meta/recipes-core/images/core-image-ptest.bb
index 90c26641ba3a..e1be81bb2666 100644
--- a/meta/recipes-core/images/core-image-ptest.bb
+++ b/meta/recipes-core/images/core-image-ptest.bb
@@ -28,6 +28,7 @@ QB_MEM:virtclass-mcextend-lttng-tools = "-m 4096"
 QB_MEM:virtclass-mcextend-python3-cryptography = "-m 4096"
 
 TEST_SUITES = "ping ssh parselogs ptest"
+TESTIMAGE_FAILED_QA_ARTIFACTS:append=" ${libdir}/${MCNAME}/ptest"
 
 # Sadly at the moment the full set of ptests is not robust enough and sporadically fails in random places
 PTEST_EXPECT_FAILURE = "1"
-- 
2.40.1

Re: [OE-Core][PATCH v3 4/4] core-image-ptest: append ptest directory to artifacts list

[Mikko Rapeli]

Hi,

On Fri, Jun 09, 2023 at 08:48:02AM +0200, Alexis Lothor� via lists.openembedded.org wrote:
> From: Alexis Lothor� <alexis.lothore@bootlin.com>
> 
> TESTIMAGE_FAILED_QA_ARTIFACTS is defined in testimage.bbclass with a
> minimal list of files to retrieve when a test fail. 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>
> ---
>  meta/recipes-core/images/core-image-ptest.bb | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/meta/recipes-core/images/core-image-ptest.bb b/meta/recipes-core/images/core-image-ptest.bb
> index 90c26641ba3a..e1be81bb2666 100644
> --- a/meta/recipes-core/images/core-image-ptest.bb
> +++ b/meta/recipes-core/images/core-image-ptest.bb
> @@ -28,6 +28,7 @@ QB_MEM:virtclass-mcextend-lttng-tools = "-m 4096"
>  QB_MEM:virtclass-mcextend-python3-cryptography = "-m 4096"
>  
>  TEST_SUITES = "ping ssh parselogs ptest"
> +TESTIMAGE_FAILED_QA_ARTIFACTS:append=" ${libdir}/${MCNAME}/ptest"

Why not += ? Also, spaces around =.

If :append is used, bbappend in other layers can not easily override
this variable.

Cheers,

-Mikko

Re: [OE-Core][PATCH v3 4/4] core-image-ptest: append ptest directory to artifacts list

[Alexis Lothoré]

Hi Mikko,

On 6/9/23 08:52, Mikko Rapeli wrote:
> Hi,
> 
> On Fri, Jun 09, 2023 at 08:48:02AM +0200, Alexis Lothoré via lists.openembedded.org wrote:
>> From: Alexis Lothoré <alexis.lothore@bootlin.com>
>>
>> TESTIMAGE_FAILED_QA_ARTIFACTS is defined in testimage.bbclass with a
>> minimal list of files to retrieve when a test fail. 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>
>> ---
>>  meta/recipes-core/images/core-image-ptest.bb | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/meta/recipes-core/images/core-image-ptest.bb b/meta/recipes-core/images/core-image-ptest.bb
>> index 90c26641ba3a..e1be81bb2666 100644
>> --- a/meta/recipes-core/images/core-image-ptest.bb
>> +++ b/meta/recipes-core/images/core-image-ptest.bb
>> @@ -28,6 +28,7 @@ QB_MEM:virtclass-mcextend-lttng-tools = "-m 4096"
>>  QB_MEM:virtclass-mcextend-python3-cryptography = "-m 4096"
>>  
>>  TEST_SUITES = "ping ssh parselogs ptest"
>> +TESTIMAGE_FAILED_QA_ARTIFACTS:append=" ${libdir}/${MCNAME}/ptest"
> 
> Why not += ? Also, spaces around =.
> 
> If :append is used, bbappend in other layers can not easily override
> this variable.

Good catch, thanks, I'll wait a bit for any more reviews and send a new version
with this point fixed.

Thanks,
Alexis

> 
> Cheers,
> 
> -Mikko
> 
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#182538): https://lists.openembedded.org/g/openembedded-core/message/182538
> Mute This Topic: https://lists.openembedded.org/mt/99423382/7394887
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [alexis.lothore@bootlin.com]
> -=-=-=-=-=-=-=-=-=-=-=-
> 

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

Re: [OE-Core][PATCH v3 4/4] core-image-ptest: append ptest directory to artifacts list

[Alexander Kanavin]

Doesn't += override the earlier ?= (or ??=), so you lose what was in
the weak assignment and end up only with the value in += ? Yes, I'm
still uncertain about this after all the years :)


Alex

On Fri, 9 Jun 2023 at 09:23, Alexis Lothoré via lists.openembedded.org
<alexis.lothore=bootlin.com@lists.openembedded.org> wrote:
>
> Hi Mikko,
>
> On 6/9/23 08:52, Mikko Rapeli wrote:
> > Hi,
> >
> > On Fri, Jun 09, 2023 at 08:48:02AM +0200, Alexis Lothoré via lists.openembedded.org wrote:
> >> From: Alexis Lothoré <alexis.lothore@bootlin.com>
> >>
> >> TESTIMAGE_FAILED_QA_ARTIFACTS is defined in testimage.bbclass with a
> >> minimal list of files to retrieve when a test fail. 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>
> >> ---
> >>  meta/recipes-core/images/core-image-ptest.bb | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/meta/recipes-core/images/core-image-ptest.bb b/meta/recipes-core/images/core-image-ptest.bb
> >> index 90c26641ba3a..e1be81bb2666 100644
> >> --- a/meta/recipes-core/images/core-image-ptest.bb
> >> +++ b/meta/recipes-core/images/core-image-ptest.bb
> >> @@ -28,6 +28,7 @@ QB_MEM:virtclass-mcextend-lttng-tools = "-m 4096"
> >>  QB_MEM:virtclass-mcextend-python3-cryptography = "-m 4096"
> >>
> >>  TEST_SUITES = "ping ssh parselogs ptest"
> >> +TESTIMAGE_FAILED_QA_ARTIFACTS:append=" ${libdir}/${MCNAME}/ptest"
> >
> > Why not += ? Also, spaces around =.
> >
> > If :append is used, bbappend in other layers can not easily override
> > this variable.
>
> Good catch, thanks, I'll wait a bit for any more reviews and send a new version
> with this point fixed.
>
> Thanks,
> Alexis
>
> >
> > Cheers,
> >
> > -Mikko
> >
> >
> >
> >
> >
>
> --
> Alexis Lothoré, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#182541): https://lists.openembedded.org/g/openembedded-core/message/182541
> Mute This Topic: https://lists.openembedded.org/mt/99423382/1686489
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [alex.kanavin@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>

Re: [OE-Core][PATCH v3 4/4] core-image-ptest: append ptest directory to artifacts list

[Alex Kiernan]

[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]

I'm glad it's not just me!

On Sun, 11 Jun 2023, 16:16 Alexander Kanavin, <alex.kanavin@gmail.com>
wrote:

> Doesn't += override the earlier ?= (or ??=), so you lose what was in
> the weak assignment and end up only with the value in += ? Yes, I'm
> still uncertain about this after all the years :)
>
>
> Alex
>
> On Fri, 9 Jun 2023 at 09:23, Alexis Lothoré via lists.openembedded.org
> <alexis.lothore=bootlin.com@lists.openembedded.org> wrote:
> >
> > Hi Mikko,
> >
> > On 6/9/23 08:52, Mikko Rapeli wrote:
> > > Hi,
> > >
> > > On Fri, Jun 09, 2023 at 08:48:02AM +0200, Alexis Lothoré via
> lists.openembedded.org wrote:
> > >> From: Alexis Lothoré <alexis.lothore@bootlin.com>
> > >>
> > >> TESTIMAGE_FAILED_QA_ARTIFACTS is defined in testimage.bbclass with a
> > >> minimal list of files to retrieve when a test fail. 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>
> > >> ---
> > >>  meta/recipes-core/images/core-image-ptest.bb | 1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/meta/recipes-core/images/core-image-ptest.bb
> b/meta/recipes-core/images/core-image-ptest.bb
> > >> index 90c26641ba3a..e1be81bb2666 100644
> > >> --- a/meta/recipes-core/images/core-image-ptest.bb
> > >> +++ b/meta/recipes-core/images/core-image-ptest.bb
> > >> @@ -28,6 +28,7 @@ QB_MEM:virtclass-mcextend-lttng-tools = "-m 4096"
> > >>  QB_MEM:virtclass-mcextend-python3-cryptography = "-m 4096"
> > >>
> > >>  TEST_SUITES = "ping ssh parselogs ptest"
> > >> +TESTIMAGE_FAILED_QA_ARTIFACTS:append=" ${libdir}/${MCNAME}/ptest"
> > >
> > > Why not += ? Also, spaces around =.
> > >
> > > If :append is used, bbappend in other layers can not easily override
> > > this variable.
> >
> > Good catch, thanks, I'll wait a bit for any more reviews and send a new
> version
> > with this point fixed.
> >
> > Thanks,
> > Alexis
> >
> > >
> > > Cheers,
> > >
> > > -Mikko
> > >
> > >
> > >
> > >
> > >
> >
> > --
> > Alexis Lothoré, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> >
> >
> >
> >
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#182620):
> https://lists.openembedded.org/g/openembedded-core/message/182620
> Mute This Topic: https://lists.openembedded.org/mt/99423382/3618097
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [
> alex.kiernan@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
>

[-- Attachment #2: Type: text/html, Size: 5236 bytes --]

Re: [OE-Core][PATCH v3 4/4] core-image-ptest: append ptest directory to artifacts list

[Richard Purdie]

On Sun, 2023-06-11 at 17:16 +0200, Alexander Kanavin wrote:
> Doesn't += override the earlier ?=

It depends. If ?= is parsed first, += will append. If the ?= is parsed
after +=, the ?= will not happen as the variable has a value.

>  (or ??=), 

+= would override ??=

Cheers,

Richard

Re: [OE-Core][PATCH v3 4/4] core-image-ptest: append ptest directory to artifacts list

[Mikko Rapeli]

Hi Richard,

On Thu, Jun 15, 2023 at 08:03:44AM +0100, Richard Purdie wrote:
> On Sun, 2023-06-11 at 17:16 +0200, Alexander Kanavin wrote:
> > Doesn't += override the earlier ?=
> 
> It depends. If ?= is parsed first, += will append. If the ?= is parsed
> after +=, the ?= will not happen as the variable has a value.
> 
> >  (or ??=),�
> 
> += would override ??=

In which kind of cases would ??= be preferred?

For basic recipes in oe-core and other layers, setting variables with
basic assignment and then appending it with += should be enough unless machine
or other override specific qualifiers are needed :append:machine.

Using plain :append without any qualifiers is annoying in downstream layers which
try to fine tune upstream open source meta layers and recipes and still remain compatible
to apply security and other updates, including full version/branch upgrades. 

I see some, for me, bad examples of ??= to set initial PACKAGECONFIG, for example.

Downstream layers would need to use PACKAGECONFIG:append to add some feature
to upstream defaults since += would require to fully control the PACKAGECONFIG
in a bbappend. I would prefer ?= for that so that a += could be used to add an
extra non-default feature but keep all the rest in upstream defaults.

This gets even more tricky with intermediate layers which might want to enable
features across recipes and layers, e.g. "selinux". Now they'd have to use use :append
to make sure "selinux" is added to PACKAGECONFIG everywhere and no other upstream defaults
are changed. And then product layers who 'know better' to for example drop some features
would need to :remove them explicitly and can't fully overwrite the PACKAGECONFIG with a simple
assignment in a bbappend. Messy. Staring a lot of "bitbake -e" ouput needed.

Cheers,

-Mikko

Re: [OE-Core][PATCH v3 4/4] core-image-ptest: append ptest directory to artifacts list

[Richard Purdie]

On Thu, 2023-06-15 at 11:34 +0300, Mikko Rapeli wrote:
> Hi Richard,
> 
> On Thu, Jun 15, 2023 at 08:03:44AM +0100, Richard Purdie wrote:
> > On Sun, 2023-06-11 at 17:16 +0200, Alexander Kanavin wrote:
> > > Doesn't += override the earlier ?=
> > 
> > It depends. If ?= is parsed first, += will append. If the ?= is parsed
> > after +=, the ?= will not happen as the variable has a value.
> > 
> > >  (or ??=), 
> > 
> > += would override ??=
> 
> In which kind of cases would ??= be preferred?
> 
> For basic recipes in oe-core and other layers, setting variables with
> basic assignment and then appending it with += should be enough unless machine
> or other override specific qualifiers are needed :append:machine.
> 
> Using plain :append without any qualifiers is annoying in downstream layers which
> try to fine tune upstream open source meta layers and recipes and still remain compatible
> to apply security and other updates, including full version/branch upgrades. 
> 
> I see some, for me, bad examples of ??= to set initial PACKAGECONFIG, for example.

This is all a big problem area and I've talked about it before. We
can't make one recommendation which will work for every person in every
scenario.

The original intent was that ??= would be widely used and solve some of
the problems. That plan was flawed but we (I?) didn't realise until too
late.

I'd not call ??= with PACKAGECONFIG as bad, it just doesn't do what you
want in some scenarios.

> Downstream layers would need to use PACKAGECONFIG:append to add some feature
> to upstream defaults since += would require to fully control the PACKAGECONFIG
> in a bbappend. I would prefer ?= for that so that a += could be used to add an
> extra non-default feature but keep all the rest in upstream defaults.
> 
> This gets even more tricky with intermediate layers which might want to enable
> features across recipes and layers, e.g. "selinux". Now they'd have to use use :append
> to make sure "selinux" is added to PACKAGECONFIG everywhere and no other upstream defaults
> are changed. And then product layers who 'know better' to for example drop some features
> would need to :remove them explicitly and can't fully overwrite the PACKAGECONFIG with a simple
> assignment in a bbappend. Messy. Staring a lot of "bitbake -e" ouput needed.

I wish I had a magic answer. ?= has it's own set of issues too last
time I sat and thought about all this :( I think the issues come if
someone uses distro overrides from a config file instead of a bbappend.

It needs time spending on it by someone willing to look at all the
different use cases, including the ones they don't use themselves and
come up with some kind of proposal. Personally, I'm willing but I can't
even get through the patch review backlog or autobuilder intermittent
bug queue at the moment :(.

Cheers,

Richard

RE: [OE-Core][PATCH v3 4/4] core-image-ptest: append ptest directory to artifacts list

[Peter Kjellerstedt]

> -----Original Message-----
> From: openembedded-core@lists.openembedded.org <openembedded-
> core@lists.openembedded.org> On Behalf Of Richard Purdie
> Sent: den 15 juni 2023 11:05
> To: Mikko Rapeli <mikko.rapeli@linaro.org>
> Cc: Alexander Kanavin <alex.kanavin@gmail.com>;
> alexis.lothore@bootlin.com; Openembedded-core@lists.openembedded.org;
> Thomas Petazzoni <thomas.petazzoni@bootlin.com>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>
> Subject: Re: [OE-Core][PATCH v3 4/4] core-image-ptest: append ptest
> directory to artifacts list
> 
> On Thu, 2023-06-15 at 11:34 +0300, Mikko Rapeli wrote:
> > Hi Richard,
> >
> > On Thu, Jun 15, 2023 at 08:03:44AM +0100, Richard Purdie wrote:
> > > On Sun, 2023-06-11 at 17:16 +0200, Alexander Kanavin wrote:
> > > > Doesn't += override the earlier ?=
> > >
> > > It depends. If ?= is parsed first, += will append. If the ?= is parsed
> > > after +=, the ?= will not happen as the variable has a value.
> > >
> > > >  (or ??=),
> > >
> > > += would override ??=
> >
> > In which kind of cases would ??= be preferred?
> >
> > For basic recipes in oe-core and other layers, setting variables with
> > basic assignment and then appending it with += should be enough unless machine
> > or other override specific qualifiers are needed :append:machine.
> >
> > Using plain :append without any qualifiers is annoying in downstream layers which
> > try to fine tune upstream open source meta layers and recipes and still remain compatible
> > to apply security and other updates, including full version/branch upgrades.
> >
> > I see some, for me, bad examples of ??= to set initial PACKAGECONFIG, for example.
> 
> This is all a big problem area and I've talked about it before. We
> can't make one recommendation which will work for every person in every
> scenario.
> 
> The original intent was that ??= would be widely used and solve some of
> the problems. That plan was flawed but we (I?) didn't realise until too
> late.
> 
> I'd not call ??= with PACKAGECONFIG as bad, it just doesn't do what you
> want in some scenarios.
> 
> > Downstream layers would need to use PACKAGECONFIG:append to add some feature
> > to upstream defaults since += would require to fully control the PACKAGECONFIG
> > in a bbappend. I would prefer ?= for that so that a += could be used to add an
> > extra non-default feature but keep all the rest in upstream defaults.
> >
> > This gets even more tricky with intermediate layers which might want to enable
> > features across recipes and layers, e.g. "selinux". Now they'd have to use use :append
> > to make sure "selinux" is added to PACKAGECONFIG everywhere and no other upstream defaults
> > are changed. And then product layers who 'know better' to for example drop some features
> > would need to :remove them explicitly and can't fully overwrite the PACKAGECONFIG with a simple
> > assignment in a bbappend. Messy. Staring a lot of "bitbake -e" ouput needed.
> 
> I wish I had a magic answer. ?= has it's own set of issues too last
> time I sat and thought about all this :( I think the issues come if
> someone uses distro overrides from a config file instead of a bbappend.
> 
> It needs time spending on it by someone willing to look at all the
> different use cases, including the ones they don't use themselves and
> come up with some kind of proposal. Personally, I'm willing but I can't
> even get through the patch review backlog or autobuilder intermittent
> bug queue at the moment :(.
> 
> Cheers,
> 
> Richard

I will not claim to know all use cases, but when it comes to 
PACKAGECONFIG my belief is that using = is the right thing to do 
in a recipe. The rationale is that the recipe is expected to be 
the first instance to set it. Then any further modifications are 
done via bbappends (or overrides, but the latter do not really 
care whether =, ?= or ??= was used as they are applied after all 
of them). 

You can of course use ?= instead, but there really isn't a need 
for it since any bbappend can just use = to override what the 
recipe set using =, or use += to add to it. 

Using ??= to set PACKAGECONFIG in the recipe on the other hand 
is problematic as it means any bbappends must use :append to add 
to it.

The most problematic aspect in this is when a bbappend wants to 
remove a feature from the list, due to how :remove works. In a 
perfect world, all bbappends would do something like this when 
they need to use the :remove operator:

FOO = "foo"
PACKAGECONFIG:remove = "${FOO}"

That way there is at least a way to for a higher layer to restore 
the feature.

Now things always get messy when you involve multiple layers 
from different sources that all try to adapt the configuration 
to their liking, so it is more than likely that my view is too 
simplified. But I am convinced that both = and ?= are better 
choices than ??= when setting PACKAGECONFIG in a recipe due to 
how the latter one forces the use of :append in bbappends, 
which the former two don't.

//Peter