[PATCH] oeqa/selftest: Fix single threaded race issue

[PATCH] oeqa/selftest: Fix single threaded race issue

[Richard Purdie]

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]
         if newbuilddir and ret.wasSuccessful() and self.removefunc:
             self.removefunc(newbuilddir)
 

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

[Quentin Schulz]

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 :)

Cheers,
Quentin

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

[Richard Purdie]

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