| Message ID | 20230609064802.11777-5-alexis.lothore@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | add failed tests artifacts retriever | expand |
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
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] > -=-=-=-=-=-=-=-=-=-=-=- >
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] > -=-=-=-=-=-=-=-=-=-=-=- >
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] > -=-=-=-=-=-=-=-=-=-=-=- > >
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
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
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
> -----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
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"