[v4] sstatesig: Fix pn and taskname derivation in find_siginfo

From: Yang Xu <yang.xu@mediatek.com>

The `bb.siggen.compare_sigfiles` method transforms the key format from
`[mc:<mc_name>:][virtual:][native:]<recipe path>:<taskname>` to
`<recipe dir>/<recipe name>:<taskname>[:virtual][:native][:mc:<mc_name>]`
by `clean_basepaths`. However, `find_siginfo` uses the original format
to get the package name (pn) and task name.

This commit corrects the method for deriving the pn and task name in
`find_siginfo` and adds handling for multilib name.
And add test for compare_sigfiles and find_siginfo working together.

Signed-off-by: Yang Xu <yang.xu@mediatek.com>
---

Notes:
    v1: correct handling for pn and taskname for native target in find_siginfo
    v2: add handling for multilib target in find_siginfo
    v3: add testcase for compare_sigfiles and find_siginfo work together.
    v4: optimize testcase to avoid non-deterministic fail

 .../recipes-test/binutils/binutils_%.bbappend |  2 +
 meta/lib/oe/sstatesig.py                      | 17 ++--
 meta/lib/oeqa/selftest/cases/sstatetests.py   | 83 +++++++++++++++++++
 3 files changed, 97 insertions(+), 5 deletions(-)
 create mode 100644 meta-selftest/recipes-test/binutils/binutils_%.bbappend

Dear Sirs,

Sorry, could you please help to review patch v4?
In patch v4, I optimize the testcase to avoid non-deterministic fail.

Thank you

On Thu, 2023-07-13 at 02:16 +0000, yang.xu@mediatek.com wrote:
> From: Yang Xu <yang.xu@mediatek.com>
> 
> The `bb.siggen.compare_sigfiles` method transforms the key format
> from
> `[mc:<mc_name>:][virtual:][native:]<recipe path>:<taskname>` to
> `<recipe dir>/<recipe
> name>:<taskname>[:virtual][:native][:mc:<mc_name>]`
> by `clean_basepaths`. However, `find_siginfo` uses the original
> format
> to get the package name (pn) and task name.
> 
> This commit corrects the method for deriving the pn and task name in
> `find_siginfo` and adds handling for multilib name.
> And add test for compare_sigfiles and find_siginfo working together.
> 
> Signed-off-by: Yang Xu <yang.xu@mediatek.com>
> ---
> 
> Notes:
>     v1: correct handling for pn and taskname for native target in
> find_siginfo
>     v2: add handling for multilib target in find_siginfo
>     v3: add testcase for compare_sigfiles and find_siginfo work
> together.
>     v4: optimize testcase to avoid non-deterministic fail
> 
>  .../recipes-test/binutils/binutils_%.bbappend |  2 +
>  meta/lib/oe/sstatesig.py                      | 17 ++--
>  meta/lib/oeqa/selftest/cases/sstatetests.py   | 83
> +++++++++++++++++++
>  3 files changed, 97 insertions(+), 5 deletions(-)
>  create mode 100644 meta-selftest/recipes-
> test/binutils/binutils_%.bbappend
> 
> diff --git a/meta-selftest/recipes-test/binutils/binutils_%.bbappend
> b/meta-selftest/recipes-test/binutils/binutils_%.bbappend
> new file mode 100644
> index 0000000000..205720982c
> --- /dev/null
> +++ b/meta-selftest/recipes-test/binutils/binutils_%.bbappend
> @@ -0,0 +1,2 @@
> +# This bbappend is used to alter the recipe using the
> test_recipe.inc file created by tests.
> +include test_recipe.inc
> diff --git a/meta/lib/oe/sstatesig.py b/meta/lib/oe/sstatesig.py
> index f943df181e..f041a0c430 100644
> --- a/meta/lib/oe/sstatesig.py
> +++ b/meta/lib/oe/sstatesig.py
> @@ -321,11 +321,18 @@ def find_siginfo(pn, taskname, taskhashlist,
> d):
>      if not taskname:
>          # We have to derive pn and taskname
>          key = pn
> -        splitit = key.split('.bb:')
> -        taskname = splitit[1]
> -        pn = os.path.basename(splitit[0]).split('_')[0]
> -        if key.startswith('virtual:native:'):
> -            pn = pn + '-native'
> +        if key.count(':') >= 2:
> +            splitit, taskname, affix = key.split(':', 2)
> +        else:
> +            splitit, taskname = key.split(':', 1)
> +            affix = ''
> +        pn =
> os.path.splitext(os.path.basename(splitit))[0].split('_')[0]
> +        affixitems = affix.split(':')
> +        if affixitems[0] == 'virtual':
> +            if affixitems[1] == 'native':
> +                pn = pn + '-native'
> +            if affixitems[1] == 'multilib':
> +                pn = affixitems[2] + '-' + pn
>  
>      hashfiles = {}
>      filedates = {}
> diff --git a/meta/lib/oeqa/selftest/cases/sstatetests.py
> b/meta/lib/oeqa/selftest/cases/sstatetests.py
> index febafdb2f7..3fa3038218 100644
> --- a/meta/lib/oeqa/selftest/cases/sstatetests.py
> +++ b/meta/lib/oeqa/selftest/cases/sstatetests.py
> @@ -691,3 +691,86 @@ TMPDIR = "${TOPDIR}/tmp-sstatesamehash2"
>          self.maxDiff = None
>          self.assertCountEqual(files1, files2)
>  
> +class SStateFindSiginfo(SStateBase):
> +    def test_sstate_compare_sigfiles_and_find_siginfo(self):
> +        """
> +        Test the functionality of the find_siginfo: basic function
> and callback in compare_sigfiles
> +        """
> +        self.write_config("""
> +TMPDIR = \"${TOPDIR}/tmp-sstates-findsiginfo\"
> +TCLIBCAPPEND = \"\"
> +MACHINE = \"qemux86-64\"
> +require conf/multilib.conf
> +MULTILIBS = "multilib:lib32"
> +DEFAULTTUNE:virtclass-multilib-lib32 = "x86"
> +BB_SIGNATURE_HANDLER = "OEBasicHash"
> +""")
> +        self.track_for_cleanup(self.topdir + "/tmp-sstates-
> findsiginfo")
> +
> +        pns = ["binutils", "binutils-native", "lib32-binutils"]
> +        target_configs = [
> +"""
> +TMPVAL1 = "tmpval1"
> +TMPVAL2 = "tmpval2"
> +do_tmptask1() {
> +    echo ${TMPVAL1}
> +}
> +do_tmptask2() {
> +    echo ${TMPVAL2}
> +}
> +addtask do_tmptask1
> +addtask tmptask2 before do_tmptask1
> +""",
> +"""
> +TMPVAL3 = "tmpval3"
> +TMPVAL4 = "tmpval4"
> +do_tmptask1() {
> +    echo ${TMPVAL3}
> +}
> +do_tmptask2() {
> +    echo ${TMPVAL4}
> +}
> +addtask do_tmptask1
> +addtask tmptask2 before do_tmptask1
> +"""
> +        ]
> +
> +        for target_config in target_configs:
> +            self.write_recipeinc("binutils", target_config)
> +            for pn in pns:
> +                bitbake("%s -c do_tmptask1 -S none" % pn)
> +            self.delete_recipeinc("binutils")
> +
> +        with bb.tinfoil.Tinfoil() as tinfoil:
> +            tinfoil.prepare(config_only=True)
> +
> +            def find_siginfo(pn, taskname, sigs=None):
> +                result = None
> +                tinfoil.set_event_mask(["bb.event.FindSigInfoResult"
> ,
> +                                "bb.command.CommandCompleted"])
> +                ret = tinfoil.run_command("findSigInfo", pn,
> taskname, sigs)
> +                if ret:
> +                    while True:
> +                        event = tinfoil.wait_event(1)
> +                        if event:
> +                            if isinstance(event,
> bb.command.CommandCompleted):
> +                                break
> +                            elif isinstance(event,
> bb.event.FindSigInfoResult):
> +                                result = event.result
> +                return result
> +
> +            def recursecb(key, hash1, hash2):
> +                nonlocal recursecb_count
> +                recursecb_count += 1
> +                hashes = [hash1, hash2]
> +                hashfiles = find_siginfo(key, None, hashes)
> +                self.assertCountEqual(hashes, hashfiles)
> +                bb.siggen.compare_sigfiles(hashfiles[hash1],
> hashfiles[hash2], recursecb)
> +
> +            for pn in pns:
> +                recursecb_count = 0
> +                filedates = find_siginfo(pn, "do_tmptask1")
> +                self.assertGreaterEqual(len(filedates), 2)
> +                latestfiles = sorted(filedates.keys(), key=lambda f:
> filedates[f])[-2:]
> +                bb.siggen.compare_sigfiles(latestfiles[-2],
> latestfiles[-1], recursecb)
> +                self.assertEqual(recursecb_count,1)

On Thu, 2023-07-13 at 02:16 +0000, Yang Xu via lists.openembedded.org
wrote:
> From: Yang Xu <yang.xu@mediatek.com>
> 
> The `bb.siggen.compare_sigfiles` method transforms the key format from
> `[mc:<mc_name>:][virtual:][native:]<recipe path>:<taskname>` to
> `<recipe dir>/<recipe name>:<taskname>[:virtual][:native][:mc:<mc_name>]`
> by `clean_basepaths`. However, `find_siginfo` uses the original format
> to get the package name (pn) and task name.
> 
> This commit corrects the method for deriving the pn and task name in
> `find_siginfo` and adds handling for multilib name.
> And add test for compare_sigfiles and find_siginfo working together.
> 
> Signed-off-by: Yang Xu <yang.xu@mediatek.com>
> ---
> 
> Notes:
>     v1: correct handling for pn and taskname for native target in find_siginfo
>     v2: add handling for multilib target in find_siginfo
>     v3: add testcase for compare_sigfiles and find_siginfo work together.
>     v4: optimize testcase to avoid non-deterministic fail
> 
>  .../recipes-test/binutils/binutils_%.bbappend |  2 +
>  meta/lib/oe/sstatesig.py                      | 17 ++--
>  meta/lib/oeqa/selftest/cases/sstatetests.py   | 83 +++++++++++++++++++
>  3 files changed, 97 insertions(+), 5 deletions(-)
>  create mode 100644 meta-selftest/recipes-test/binutils/binutils_%.bbappend
> 
> diff --git a/meta-selftest/recipes-test/binutils/binutils_%.bbappend b/meta-selftest/recipes-test/binutils/binutils_%.bbappend
> new file mode 100644
> index 0000000000..205720982c
> --- /dev/null
> +++ b/meta-selftest/recipes-test/binutils/binutils_%.bbappend
> @@ -0,0 +1,2 @@
> +# This bbappend is used to alter the recipe using the test_recipe.inc file created by tests.
> +include test_recipe.inc
> diff --git a/meta/lib/oe/sstatesig.py b/meta/lib/oe/sstatesig.py
> index f943df181e..f041a0c430 100644
> --- a/meta/lib/oe/sstatesig.py
> +++ b/meta/lib/oe/sstatesig.py
> @@ -321,11 +321,18 @@ def find_siginfo(pn, taskname, taskhashlist, d):
>      if not taskname:
>          # We have to derive pn and taskname
>          key = pn
> -        splitit = key.split('.bb:')
> -        taskname = splitit[1]
> -        pn = os.path.basename(splitit[0]).split('_')[0]
> -        if key.startswith('virtual:native:'):
> -            pn = pn + '-native'
> +        if key.count(':') >= 2:
> +            splitit, taskname, affix = key.split(':', 2)
> +        else:
> +            splitit, taskname = key.split(':', 1)
> +            affix = ''
> +        pn = os.path.splitext(os.path.basename(splitit))[0].split('_')[0]
> +        affixitems = affix.split(':')
> +        if affixitems[0] == 'virtual':
> +            if affixitems[1] == 'native':
> +                pn = pn + '-native'
> +            if affixitems[1] == 'multilib':
> +                pn = affixitems[2] + '-' + pn
>  
>      hashfiles = {}
>      filedates = {}


I've stared at this patch long and hard and I'm coming to the
conclusion that whilst what you're doing improves things, there are
more corner cases remaining and we're just moving the problem to new
ones down the road. Having to hardcode in each of the class names and
special case them is a big warning sign.

I started wondering why we encode pathnames into the siginfo files. The
reason is that is how bitbake handles them within runqueue. In earlier
times when this was being built, that was fine but things have been
extended many times over since that decision was made.

The issue is that those "internal" representations don't map onto other
systems, so sstate writes files in a different format. There is no easy
way to map the original representations to the format used in the
sstate file names.

My conclusion is that we should change the siginfo files to have the
data already in "sstate" format. That does potentially break a few
things but is probably worth doing for the simplicity gains in code
like this.

Cheers,

Richard

On Fri, 2023-08-04 at 17:13 +0100, Richard Purdie via
lists.openembedded.org wrote:
> On Thu, 2023-07-13 at 02:16 +0000, Yang Xu via lists.openembedded.org
> wrote:
> > From: Yang Xu <yang.xu@mediatek.com>
> > 
> > The `bb.siggen.compare_sigfiles` method transforms the key format from
> > `[mc:<mc_name>:][virtual:][native:]<recipe path>:<taskname>` to
> > `<recipe dir>/<recipe name>:<taskname>[:virtual][:native][:mc:<mc_name>]`
> > by `clean_basepaths`. However, `find_siginfo` uses the original format
> > to get the package name (pn) and task name.
> > 
> > This commit corrects the method for deriving the pn and task name in
> > `find_siginfo` and adds handling for multilib name.
> > And add test for compare_sigfiles and find_siginfo working together.
> > 
> > Signed-off-by: Yang Xu <yang.xu@mediatek.com>
> > ---
> > 
> > Notes:
> >     v1: correct handling for pn and taskname for native target in find_siginfo
> >     v2: add handling for multilib target in find_siginfo
> >     v3: add testcase for compare_sigfiles and find_siginfo work together.
> >     v4: optimize testcase to avoid non-deterministic fail
> > 
> >  .../recipes-test/binutils/binutils_%.bbappend |  2 +
> >  meta/lib/oe/sstatesig.py                      | 17 ++--
> >  meta/lib/oeqa/selftest/cases/sstatetests.py   | 83 +++++++++++++++++++
> >  3 files changed, 97 insertions(+), 5 deletions(-)
> >  create mode 100644 meta-selftest/recipes-test/binutils/binutils_%.bbappend
> > 
> > diff --git a/meta-selftest/recipes-test/binutils/binutils_%.bbappend b/meta-selftest/recipes-test/binutils/binutils_%.bbappend
> > new file mode 100644
> > index 0000000000..205720982c
> > --- /dev/null
> > +++ b/meta-selftest/recipes-test/binutils/binutils_%.bbappend
> > @@ -0,0 +1,2 @@
> > +# This bbappend is used to alter the recipe using the test_recipe.inc file created by tests.
> > +include test_recipe.inc
> > diff --git a/meta/lib/oe/sstatesig.py b/meta/lib/oe/sstatesig.py
> > index f943df181e..f041a0c430 100644
> > --- a/meta/lib/oe/sstatesig.py
> > +++ b/meta/lib/oe/sstatesig.py
> > @@ -321,11 +321,18 @@ def find_siginfo(pn, taskname, taskhashlist, d):
> >      if not taskname:
> >          # We have to derive pn and taskname
> >          key = pn
> > -        splitit = key.split('.bb:')
> > -        taskname = splitit[1]
> > -        pn = os.path.basename(splitit[0]).split('_')[0]
> > -        if key.startswith('virtual:native:'):
> > -            pn = pn + '-native'
> > +        if key.count(':') >= 2:
> > +            splitit, taskname, affix = key.split(':', 2)
> > +        else:
> > +            splitit, taskname = key.split(':', 1)
> > +            affix = ''
> > +        pn = os.path.splitext(os.path.basename(splitit))[0].split('_')[0]
> > +        affixitems = affix.split(':')
> > +        if affixitems[0] == 'virtual':
> > +            if affixitems[1] == 'native':
> > +                pn = pn + '-native'
> > +            if affixitems[1] == 'multilib':
> > +                pn = affixitems[2] + '-' + pn
> >  
> >      hashfiles = {}
> >      filedates = {}
> 
> 
> I've stared at this patch long and hard and I'm coming to the
> conclusion that whilst what you're doing improves things, there are
> more corner cases remaining and we're just moving the problem to new
> ones down the road. Having to hardcode in each of the class names and
> special case them is a big warning sign.
> 
> I started wondering why we encode pathnames into the siginfo files. The
> reason is that is how bitbake handles them within runqueue. In earlier
> times when this was being built, that was fine but things have been
> extended many times over since that decision was made.
> 
> The issue is that those "internal" representations don't map onto other
> systems, so sstate writes files in a different format. There is no easy
> way to map the original representations to the format used in the
> sstate file names.
> 
> My conclusion is that we should change the siginfo files to have the
> data already in "sstate" format. That does potentially break a few
> things but is probably worth doing for the simplicity gains in code
> like this.

I've sent a couple of patches, one to bitbake and one to OE-Core which
change the format of the data in runtaskdeps to be PN based rather than
recipe filename based which then makes the above change unneeded, we no
longer need to hardcode classes into it.

With those changes, I did confirm that the testcase added in this
pattch does pass.

We need to review those other changes but assuming testing shows they
work out ok could you update this patch to simply add the test case?
I'd also appreciate it if you could check those patches do resolve the
issue you were seeing.

Cheers,

Richard

It is a elegant solution for current issue. I will test and update my
patch to only include testcase part soon.

Thank you

Yang

On Sat, 2023-08-05 at 11:36 +0100, Richard Purdie wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  
> On Fri, 2023-08-04 at 17:13 +0100, Richard Purdie via
> lists.openembedded.org wrote:
> > On Thu, 2023-07-13 at 02:16 +0000, Yang Xu via
> lists.openembedded.org
> > wrote:
> > > From: Yang Xu <yang.xu@mediatek.com>
> > > 
> > > The `bb.siggen.compare_sigfiles` method transforms the key format
> from
> > > `[mc:<mc_name>:][virtual:][native:]<recipe path>:<taskname>` to
> > > `<recipe dir>/<recipe
> name>:<taskname>[:virtual][:native][:mc:<mc_name>]`
> > > by `clean_basepaths`. However, `find_siginfo` uses the original
> format
> > > to get the package name (pn) and task name.
> > > 
> > > This commit corrects the method for deriving the pn and task name
> in
> > > `find_siginfo` and adds handling for multilib name.
> > > And add test for compare_sigfiles and find_siginfo working
> together.
> > > 
> > > Signed-off-by: Yang Xu <yang.xu@mediatek.com>
> > > ---
> > > 
> > > Notes:
> > >     v1: correct handling for pn and taskname for native target in
> find_siginfo
> > >     v2: add handling for multilib target in find_siginfo
> > >     v3: add testcase for compare_sigfiles and find_siginfo work
> together.
> > >     v4: optimize testcase to avoid non-deterministic fail
> > > 
> > >  .../recipes-test/binutils/binutils_%.bbappend |  2 +
> > >  meta/lib/oe/sstatesig.py                      | 17 ++--
> > >  meta/lib/oeqa/selftest/cases/sstatetests.py   | 83
> +++++++++++++++++++
> > >  3 files changed, 97 insertions(+), 5 deletions(-)
> > >  create mode 100644 meta-selftest/recipes-
> test/binutils/binutils_%.bbappend
> > > 
> > > diff --git a/meta-selftest/recipes-
> test/binutils/binutils_%.bbappend b/meta-selftest/recipes-
> test/binutils/binutils_%.bbappend
> > > new file mode 100644
> > > index 0000000000..205720982c
> > > --- /dev/null
> > > +++ b/meta-selftest/recipes-test/binutils/binutils_%.bbappend
> > > @@ -0,0 +1,2 @@
> > > +# This bbappend is used to alter the recipe using the
> test_recipe.inc file created by tests.
> > > +include test_recipe.inc
> > > diff --git a/meta/lib/oe/sstatesig.py b/meta/lib/oe/sstatesig.py
> > > index f943df181e..f041a0c430 100644
> > > --- a/meta/lib/oe/sstatesig.py
> > > +++ b/meta/lib/oe/sstatesig.py
> > > @@ -321,11 +321,18 @@ def find_siginfo(pn, taskname,
> taskhashlist, d):
> > >      if not taskname:
> > >          # We have to derive pn and taskname
> > >          key = pn
> > > -        splitit = key.split('.bb:')
> > > -        taskname = splitit[1]
> > > -        pn = os.path.basename(splitit[0]).split('_')[0]
> > > -        if key.startswith('virtual:native:'):
> > > -            pn = pn + '-native'
> > > +        if key.count(':') >= 2:
> > > +            splitit, taskname, affix = key.split(':', 2)
> > > +        else:
> > > +            splitit, taskname = key.split(':', 1)
> > > +            affix = ''
> > > +        pn =
> os.path.splitext(os.path.basename(splitit))[0].split('_')[0]
> > > +        affixitems = affix.split(':')
> > > +        if affixitems[0] == 'virtual':
> > > +            if affixitems[1] == 'native':
> > > +                pn = pn + '-native'
> > > +            if affixitems[1] == 'multilib':
> > > +                pn = affixitems[2] + '-' + pn
> > >  
> > >      hashfiles = {}
> > >      filedates = {}
> > 
> > 
> > I've stared at this patch long and hard and I'm coming to the
> > conclusion that whilst what you're doing improves things, there are
> > more corner cases remaining and we're just moving the problem to
> new
> > ones down the road. Having to hardcode in each of the class names
> and
> > special case them is a big warning sign.
> > 
> > I started wondering why we encode pathnames into the siginfo files.
> The
> > reason is that is how bitbake handles them within runqueue. In
> earlier
> > times when this was being built, that was fine but things have been
> > extended many times over since that decision was made.
> > 
> > The issue is that those "internal" representations don't map onto
> other
> > systems, so sstate writes files in a different format. There is no
> easy
> > way to map the original representations to the format used in the
> > sstate file names.
> > 
> > My conclusion is that we should change the siginfo files to have
> the
> > data already in "sstate" format. That does potentially break a few
> > things but is probably worth doing for the simplicity gains in code
> > like this.
> 
> I've sent a couple of patches, one to bitbake and one to OE-Core
> which
> change the format of the data in runtaskdeps to be PN based rather
> than
> recipe filename based which then makes the above change unneeded, we
> no
> longer need to hardcode classes into it.
> 
> With those changes, I did confirm that the testcase added in this
> pattch does pass.
> 
> We need to review those other changes but assuming testing shows they
> work out ok could you update this patch to simply add the test case?
> I'd also appreciate it if you could check those patches do resolve
> the
> issue you were seeing.
> 
> Cheers,
> 
> Richard
>

On Tue, 2023-08-08 at 10:15 +0000, Yang Xu (徐扬) wrote:
> It is a elegant solution for current issue. I will test and update my
> patch to only include testcase part soon.
> 
> Thank you

I did end up tweaking your patch to this:

https://git.yoctoproject.org/poky/commit/?h=master-next&id=3a79452507846756f6d78cab4dc4adafa0cb2be7

Cheers,

Richard

I got it, thank you :)

Yang

On Tue, 2023-08-08 at 11:39 +0100, Richard Purdie wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  
> On Tue, 2023-08-08 at 10:15 +0000, Yang Xu (徐扬) wrote:
> > It is a elegant solution for current issue. I will test and update
> my
> > patch to only include testcase part soon.
> > 
> > Thank you
> 
> I did end up tweaking your patch to this:
> 
> 
https://git.yoctoproject.org/poky/commit/?h=master-next&id=3a79452507846756f6d78cab4dc4adafa0cb2be7
> 
> Cheers,
> 
> Richard