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

[Richard Purdie <richard.purdie@linuxfoundation.org>]

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