[PATCH] icecc: fix PN 'no-pn' handling

[PATCH] icecc: fix PN 'no-pn' handling

[Marco Felsch]

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()
 
-- 
2.39.5

Re: [OE-core] [PATCH] icecc: fix PN 'no-pn' handling

[Richard Purdie]

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

Re: [OE-core] [PATCH] icecc: fix PN 'no-pn' handling

[Richard Purdie]

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

Re: [OE-core] [PATCH] icecc: fix PN 'no-pn' handling

[Marco Felsch]

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

Re: [OE-core] [PATCH] icecc: fix PN 'no-pn' handling

[Richard Purdie]

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

Re: [OE-core] [PATCH] icecc: fix PN 'no-pn' handling

[Marco Felsch]

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

Re: [OE-core] [PATCH] icecc: fix PN 'no-pn' handling

[Richard Purdie]

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