Re: [OE-core] [PATCH v4] sstatesig: Fix pn and taskname derivation in find_siginfo

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

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