Re: [OE-core] [PATCH] oeqa/selftest: Fix single threaded race issue

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

On Mon, 2025-10-06 at 18:56 +0200, Quentin Schulz wrote:
> Hi Richard,
> 
> On 10/6/25 3:09 PM, Richard Purdie via lists.openembedded.org wrote:
> > oe-selftest sets up separate build directories to run the tests in.
> > To to this, environment paths pointing at the previous build directory
> > are updated. In the multi-threaded case this is fine as the thread is
> > destroyed and the parent remains unchanged but in the single threaded
> > case, the environment is broken afterwards. This can mean we try and access
> > a directory which is in the process of being deleted (e.g. by clobberdir).
> > 
> > Restore the environment afterwards regardless to ensure the single threaded
> > case doesn't try and access the build directory which is now being deleted.
> > 
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> >   meta/lib/oeqa/selftest/context.py | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/meta/lib/oeqa/selftest/context.py b/meta/lib/oeqa/selftest/context.py
> > index 16f82c6737d..c9eb4817253 100644
> > --- a/meta/lib/oeqa/selftest/context.py
> > +++ b/meta/lib/oeqa/selftest/context.py
> > @@ -44,9 +44,13 @@ class NonConcurrentTestSuite(unittest.TestSuite):
> >           self.bb_vars = bb_vars
> >   
> >       def run(self, result):
> > +        origenv = os.environ.copy()
> >           (builddir, newbuilddir) = self.setupfunc("-st", None, self.suite)
> >           ret = super().run(result)
> > +        # In forks we don't have to restore but in a single process, restore cwd and the env
> >           os.chdir(builddir)
> > +        for e in origenv:
> > +            os.environ[e] = origenv[e]
> 
> Wondering if we cannot replace the loop with
> 
> os.environ.update(origenv)
> 
> which should have the same behavior?
> 
> Note that keys that didn't exist in origenv but exists in os.environ 
> after the copy() will stay there (in the update() and suggested implem 
> in the patch). If you want to really have the same one, I'm wondering if 
> os.environ = origenv[:] would work, but I'm assuming there's a reason
> you didn't go for that that I am missing. Another option could be to 
> delete/pop all keys that do not exist in both, e.g. with (not tested)
> 
> del_keys = set(os.environ.keys()) - set(origenv.keys())
> for del_key in del_keys:
>      del os.environ[del_key]
> 
> Though I'm wondering if we cannot simply call os.environ.clear() before? 
> Not sure what the side effects could be, I have a feeling this 
> os.environ is a bit more than a dict :)

os.environ is magic and I've been burnt by trying to be too clever with
it in the past, it is definitely not a dict. It has been massively
improved since I was burnt by it too but the memories make me want to
just treat it carefully and clearly.

Cheers,

Richard