| Message ID | 20241209164608.3595821-1-m.felsch@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | icecc: fix PN 'no-pn' handling | expand |
On Mon, 2024-12-09 at 17:46 +0100, Marco Felsch via lists.openembedded.org wrote: > Since bitbake commit f24bbaaddb36 ("data: Add support for new > BB_HASH_CODEPARSER_VALS for cache optimisation") the > BB_HASH_CODEPARSER_VALS are passed during the bb.build_dependencies() > step. > > With PN set to 'no-pn' and the icecc_version() running during > bb.build_dependencies() (due to the 'vardepsexclude') the bb.fatal() is > triggered while parsing target-sdk-provides-dummy.bb albeit it was > already disabled via ICECC_RECIPE_DISABLE. > > To fix this use_icecc() need to verify if PN is set to 'no-pn' which > indicates the early bb.build_dependencies() task and return 'no' in that > case. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > Hi, > > this patch should fix the reoprted ICECC bug: > > https://lists.yoctoproject.org/g/yocto/topic/icecc_support_broken/103429714 > > Regards, > Marco > > meta/classes/icecc.bbclass | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/meta/classes/icecc.bbclass b/meta/classes/icecc.bbclass > index 159cae20f8ca..df73e31e9514 100644 > --- a/meta/classes/icecc.bbclass > +++ b/meta/classes/icecc.bbclass > @@ -159,6 +159,12 @@ def use_icecc(bb,d): > bb.debug(1, "%s: bbclass %s found in disable, disable icecc" % (pn, bbclass)) > return "no" > > + # PN set to 'no-pn' indicates that bitbake is at the early > + # bb.build_dependencies() stage and it's not possible to use the value to > + # decide if icecc can be used. > + if pn == "no-pn": > + return "no" > + > disabled_recipes = (d.getVar('ICECC_RECIPE_DISABLE') or "").split() > enabled_recipes = (d.getVar('ICECC_RECIPE_ENABLE') or "").split() > I'm not sure the patch description is quite right and I'm also not sure this is a safe way to solve the issue here. Bitbake is trying to work out the dependencies of the various functions. The reasoning was that setting some dummy values for that code would stop it spiralling off and analysing multiple pieces of code that were effectively functionally identical where for example a version string just changes. This gets messy with shell function, for example with the shell function: set_icecc_env() { if [ "${@use_icecc(bb, d)}" = "no" ] then return fi } it has to run use_icecc() to expand that in case it turned into a block of code. If we take a patch like this, I would like the comment above and the commit message to be correct. "bb.build_dependencies" doesn't exist and the early stage you refer to is misleading as it doesn't work like that. If we're hitting the "NULL prefix" code in icecc_version(), wouldn't it be better to special case pn == "no-pn" there, note that it is at dependency parse time and return a dummy "parse-time-dummy- icetarball.tgz" value? Cheers, Richard
On Mon, 2024-12-09 at 17:22 +0000, Richard Purdie via lists.openembedded.org wrote: > On Mon, 2024-12-09 at 17:46 +0100, Marco Felsch via lists.openembedded.org wrote: > > Since bitbake commit f24bbaaddb36 ("data: Add support for new > > BB_HASH_CODEPARSER_VALS for cache optimisation") the > > BB_HASH_CODEPARSER_VALS are passed during the bb.build_dependencies() > > step. > > > > With PN set to 'no-pn' and the icecc_version() running during > > bb.build_dependencies() (due to the 'vardepsexclude') the bb.fatal() is > > triggered while parsing target-sdk-provides-dummy.bb albeit it was > > already disabled via ICECC_RECIPE_DISABLE. > > > > To fix this use_icecc() need to verify if PN is set to 'no-pn' which > > indicates the early bb.build_dependencies() task and return 'no' in that > > case. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > Hi, > > > > this patch should fix the reoprted ICECC bug: > > > > https://lists.yoctoproject.org/g/yocto/topic/icecc_support_broken/103429714 > > > > Regards, > > Marco > > > > meta/classes/icecc.bbclass | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/meta/classes/icecc.bbclass b/meta/classes/icecc.bbclass > > index 159cae20f8ca..df73e31e9514 100644 > > --- a/meta/classes/icecc.bbclass > > +++ b/meta/classes/icecc.bbclass > > @@ -159,6 +159,12 @@ def use_icecc(bb,d): > > bb.debug(1, "%s: bbclass %s found in disable, disable icecc" % (pn, bbclass)) > > return "no" > > > > + # PN set to 'no-pn' indicates that bitbake is at the early > > + # bb.build_dependencies() stage and it's not possible to use the value to > > + # decide if icecc can be used. > > + if pn == "no-pn": > > + return "no" > > + > > disabled_recipes = (d.getVar('ICECC_RECIPE_DISABLE') or "").split() > > enabled_recipes = (d.getVar('ICECC_RECIPE_ENABLE') or "").split() > > > > I'm not sure the patch description is quite right and I'm also not sure > this is a safe way to solve the issue here. > > Bitbake is trying to work out the dependencies of the various > functions. The reasoning was that setting some dummy values for that > code would stop it spiralling off and analysing multiple pieces of code > that were effectively functionally identical where for example a > version string just changes. > > This gets messy with shell function, for example with the shell > function: > > set_icecc_env() { > if [ "${@use_icecc(bb, d)}" = "no" ] > then > return > fi > } > > it has to run use_icecc() to expand that in case it turned into a block > of code. > > If we take a patch like this, I would like the comment above and the > commit message to be correct. "bb.build_dependencies" doesn't exist and > the early stage you refer to is misleading as it doesn't work like > that. > > If we're hitting the "NULL prefix" code in icecc_version(), wouldn't it > be better to special case pn == "no-pn" there, note that it is at > dependency parse time and return a dummy "parse-time-dummy- > icetarball.tgz" value? I meant to add, if we rewrite some of these functions in python, these issues would go away as we don't expand data in python functions. The modern way of doing much of this would be a python prefunc on the appropriate tasks. I have to wonder how many people are using icecc :/. Cheers, Richard
On 24-12-09, Richard Purdie wrote: > On Mon, 2024-12-09 at 17:22 +0000, Richard Purdie via lists.openembedded.org wrote: > > On Mon, 2024-12-09 at 17:46 +0100, Marco Felsch via lists.openembedded.org wrote: > > > Since bitbake commit f24bbaaddb36 ("data: Add support for new > > > BB_HASH_CODEPARSER_VALS for cache optimisation") the > > > BB_HASH_CODEPARSER_VALS are passed during the bb.build_dependencies() > > > step. > > > > > > With PN set to 'no-pn' and the icecc_version() running during > > > bb.build_dependencies() (due to the 'vardepsexclude') the bb.fatal() is > > > triggered while parsing target-sdk-provides-dummy.bb albeit it was > > > already disabled via ICECC_RECIPE_DISABLE. > > > > > > To fix this use_icecc() need to verify if PN is set to 'no-pn' which > > > indicates the early bb.build_dependencies() task and return 'no' in that > > > case. > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > --- > > > Hi, > > > > > > this patch should fix the reoprted ICECC bug: > > > > > > https://lists.yoctoproject.org/g/yocto/topic/icecc_support_broken/103429714 > > > > > > Regards, > > > � Marco > > > > > > �meta/classes/icecc.bbclass | 6 ++++++ > > > �1 file changed, 6 insertions(+) > > > > > > diff --git a/meta/classes/icecc.bbclass b/meta/classes/icecc.bbclass > > > index 159cae20f8ca..df73e31e9514 100644 > > > --- a/meta/classes/icecc.bbclass > > > +++ b/meta/classes/icecc.bbclass > > > @@ -159,6 +159,12 @@ def use_icecc(bb,d): > > > ������������ bb.debug(1, "%s: bbclass %s found in disable, disable icecc" % (pn, bbclass)) > > > ������������ return "no" > > > � > > > +��� # PN set to 'no-pn' indicates that bitbake is at the early > > > +��� # bb.build_dependencies() stage and it's not possible to use the value to > > > +��� # decide if icecc can be used. > > > +��� if pn == "no-pn": > > > +������� return "no" > > > + > > > ���� disabled_recipes = (d.getVar('ICECC_RECIPE_DISABLE') or "").split() > > > ���� enabled_recipes = (d.getVar('ICECC_RECIPE_ENABLE') or "").split() > > > � > > > > I'm not sure the patch description is quite right and I'm also not sure > > this is a safe way to solve the issue here. > > > > Bitbake is trying to work out the dependencies of the various > > functions. The reasoning was that setting some dummy values for that > > code would stop it spiralling off and analysing multiple pieces of code > > that were effectively functionally identical where for example a > > version string just changes. > > > > This gets messy with shell function, for example with the shell > > function: > > > > set_icecc_env() { > > ��� if [ "${@use_icecc(bb, d)}" = "no" ] > > ��� then > > ������� return > > ��� fi > > } > > > > it has to run use_icecc() to expand that in case it turned into a block > > of code. > > > > If we take a patch like this, I would like the comment above and the > > commit message to be correct. "bb.build_dependencies" doesn't exist and > > the early stage you refer to is misleading as it doesn't work like > > that. Okay, thanks for the clarification. I'm not very deep into the bitbake core development. > > If we're hitting the "NULL prefix" code in icecc_version(), wouldn't it > > be better to special case pn == "no-pn" there, note that it is at > > dependency parse time and return a dummy "parse-time-dummy- > > icetarball.tgz" value? I used the current location (use_icecc) since it makes no sense to me to check for recipe names (disabled_recipes, enabled_recipes) if we can't use the PN value. > I meant to add, if we rewrite some of these functions in python, these > issues would go away as we don't expand data in python functions. Now with your explanation it makes more sense to me why only set_icecc_env() failed... Thanks a lot! > The modern way of doing much of this would be a python prefunc on the > appropriate tasks. I have to wonder how many people are using icecc :/. Okay, I will give it a try with a python based prefunc. Regards, Marco
On Mon, 2024-12-09 at 18:38 +0100, Marco Felsch wrote: > On 24-12-09, Richard Purdie wrote: > > On Mon, 2024-12-09 at 17:22 +0000, Richard Purdie via > > lists.openembedded.org wrote: > > > Okay, thanks for the clarification. I'm not very deep into the > bitbake > core development. > > > > If we're hitting the "NULL prefix" code in icecc_version(), > > > wouldn't it > > > be better to special case pn == "no-pn" there, note that it is at > > > dependency parse time and return a dummy "parse-time-dummy- > > > icetarball.tgz" value? > > I used the current location (use_icecc) since it makes no sense to me > to > check for recipe names (disabled_recipes, enabled_recipes) if we > can't > use the PN value. > > > I meant to add, if we rewrite some of these functions in python, > > these > > issues would go away as we don't expand data in python functions. > > Now with your explanation it makes more sense to me why only > set_icecc_env() failed... Thanks a lot! > > > The modern way of doing much of this would be a python prefunc on > > the > > appropriate tasks. I have to wonder how many people are using icecc > > :/. > > Okay, I will give it a try with a python based prefunc. Just wanted to say thanks for that patch, it looks like a neater way to fix this and likely other issues too. Cheers, Richard
Hi, On 24-12-16, Richard Purdie wrote: > On Mon, 2024-12-09 at 18:38 +0100, Marco Felsch wrote: > > On 24-12-09, Richard Purdie wrote: > > > On Mon, 2024-12-09 at 17:22 +0000, Richard Purdie via > > > lists.openembedded.org wrote: ... > > > The modern way of doing much of this would be a python prefunc on > > > the > > > appropriate tasks. I have to wonder how many people are using icecc > > > :/. > > > > Okay, I will give it a try with a python based prefunc. > > Just wanted to say thanks for that patch, it looks like a neater way to > fix this and likely other issues too. I've seen that you applied both v2 patches onto the master branch. Is it possible to apply both to scarthgap too? Regards, Marco
On Mon, 2024-12-16 at 23:22 +0100, Marco Felsch wrote: > Hi, > > On 24-12-16, Richard Purdie wrote: > > On Mon, 2024-12-09 at 18:38 +0100, Marco Felsch wrote: > > > On 24-12-09, Richard Purdie wrote: > > > > On Mon, 2024-12-09 at 17:22 +0000, Richard Purdie via > > > > lists.openembedded.org wrote: > > ... > > > > > The modern way of doing much of this would be a python prefunc > > > > on > > > > the > > > > appropriate tasks. I have to wonder how many people are using > > > > icecc > > > > :/. > > > > > > Okay, I will give it a try with a python based prefunc. > > > > Just wanted to say thanks for that patch, it looks like a neater > > way to > > fix this and likely other issues too. > > I've seen that you applied both v2 patches onto the master branch. Is > it possible to apply both to scarthgap too? From my perspective I think they would be reasonable fixes to backport given the relative isolation of the class and the benefit as they are broken at the moment. I've copied Steve Sakoman on the email, you need to point him at the patches. To save a few electrons, the patches in question are: https://git.yoctoproject.org/poky/commit/?id=f391bf2c0804aca812ad24e52c14b30347726111 https://git.yoctoproject.org/poky/commit/?id=b8e99c75a1b0c7c5bb84adcb907061005f504764 Cheers, Richard
diff --git a/meta/classes/icecc.bbclass b/meta/classes/icecc.bbclass index 159cae20f8ca..df73e31e9514 100644 --- a/meta/classes/icecc.bbclass +++ b/meta/classes/icecc.bbclass @@ -159,6 +159,12 @@ def use_icecc(bb,d): bb.debug(1, "%s: bbclass %s found in disable, disable icecc" % (pn, bbclass)) return "no" + # PN set to 'no-pn' indicates that bitbake is at the early + # bb.build_dependencies() stage and it's not possible to use the value to + # decide if icecc can be used. + if pn == "no-pn": + return "no" + disabled_recipes = (d.getVar('ICECC_RECIPE_DISABLE') or "").split() enabled_recipes = (d.getVar('ICECC_RECIPE_ENABLE') or "").split()
Since bitbake commit f24bbaaddb36 ("data: Add support for new BB_HASH_CODEPARSER_VALS for cache optimisation") the BB_HASH_CODEPARSER_VALS are passed during the bb.build_dependencies() step. With PN set to 'no-pn' and the icecc_version() running during bb.build_dependencies() (due to the 'vardepsexclude') the bb.fatal() is triggered while parsing target-sdk-provides-dummy.bb albeit it was already disabled via ICECC_RECIPE_DISABLE. To fix this use_icecc() need to verify if PN is set to 'no-pn' which indicates the early bb.build_dependencies() task and return 'no' in that case. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- Hi, this patch should fix the reoprted ICECC bug: https://lists.yoctoproject.org/g/yocto/topic/icecc_support_broken/103429714 Regards, Marco meta/classes/icecc.bbclass | 6 ++++++ 1 file changed, 6 insertions(+)