Re: [OE-core] [PATCH] buildhistory: Fix do_package race issues

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

On Wed, 2024-05-29 at 14:17 +0200, Alexander Kanavin via lists.openembedded.org wrote:
> On Wed, 29 May 2024 at 13:57, pmi183 via lists.openembedded.org
> <pmi183=gmail.com@lists.openembedded.org> wrote:
> 
> > Patch 1 - fixes race condition detected while executing functions registered on `SSTATEPOSTINSTFUNCS`
> 
> The patches needs to be sent as separate messages in a series, like
> every other patch submitted here ('git send-email' does it).
> 
> > ---
> >  meta/classes-global/sstate.bbclass | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/meta/classes-global/sstate.bbclass b/meta/classes-global/sstate.bbclass
> > index 76a7b59636..9887169e4f 100644
> > --- a/meta/classes-global/sstate.bbclass
> > +++ b/meta/classes-global/sstate.bbclass
> > @@ -403,7 +403,6 @@ def sstate_installpkgdir(ss, d):
> >      for state in ss['dirs']:
> >          prepdir(state[1])
> >          bb.utils.rename(sstateinst + state[0], state[1])
> > -    sstate_install(ss, d)
> > 
> >      for plain in ss['plaindirs']:
> >          workdir = d.getVar('WORKDIR')
> > @@ -416,6 +415,8 @@ def sstate_installpkgdir(ss, d):
> >          prepdir(dest)
> >          bb.utils.rename(src, dest)
> > 
> > +    sstate_install(ss, d)
> > +
> >      return True
> > 
> >  python sstate_hardcode_path_unpack () {
> 
> sstate.bbclass is a highly sensitive piece in yocto, and any changes
> need to be strictly justified. Specifically, you need to explain what
> is racing against each other, demonstrate how to reproduce and observe
> the problem that the patch is fixing, and (ideally) provide a test
> case (in oe-selftest) that fails without the change and passes with
> it.

I did look at the sstate code and this part of the problem
specifically. I think there is an issue here and this probably is the
nearly the right fix.

This is the only call site for sstate_install and the "for plain in
ss['plaindirs']:" block probably needs to move into sstate_install
before the SSTATEPOSTINSTFUNCS function calls. I say that as the code
is then within the lock operations. It probably doesn't technically
need to be but it would be what most users would expect.

I'm not sure we've ever guaranteed that SSTATEPOSTINSTFUNCS can see
plaindirs but the buildhistory code clearly depends on that. Quite
often that would happen to work but I can see "from sstate" codepaths
where it wouldn't.

I haven't looked into the other patches in much detail but I did notice
that patch 2 has shell whitespace problems that one of the later
patches fixes.

Cheers,

Richard