Hi Quentin, On Tue, 27 Jan 2026 at 00:43, Quentin Schulz <[email protected]> wrote: > > Hi Simon, > > On 1/22/26 11:46 PM, Simon Glass wrote: > > Hi, > > > > On Thu, 22 Jan 2026 at 02:06, Quentin Schulz <[email protected]> > > wrote: > >> > >> Hi Wojciech, > >> > >> On 1/21/26 1:43 PM, Wojciech Dubowik wrote: > >>> On Tue, Jan 20, 2026 at 04:53:04PM +0100, Quentin Schulz wrote: > >>> Hello Quentin, > >>>> Hi Wojciech, > >>>> > >>>> On 1/20/26 9:12 AM, Wojciech Dubowik wrote: > >> [...] > >>>>> + os.environ['SOFTHSM2_CONF'] = softhsm2_conf > >>>> > >>>> This is wrong, you'll be messing up with the environment of all tests > >>>> being > >>>> run in the same thread. You must use the "with > >>>> unittest.mock.patch.dict('os.environ'," implementation I used in > >>>> testFitSignPKCS11Simple. > >>> > >>> Well, I have done so in my V2 but has been commented as wrong by the > >>> first reviewer. I will restore it back. > >>> > >> > >> Indeed, I see Simon asked you to do this in v2 and I missed it. It isn't > >> how we should be doing it. > >> > >> This is likely fine on its own because there's only one test that is now > >> modifying os.environ's SOFTHSM2_CONF but this will be a problem next > >> time a test wants to modify it. I actually hit this issue when > >> developing the PKCS11 fit signing tests as I had two tests modifying the > >> environment. > >> > >> The only trace of it left is the changelog in > >> https://lore.kernel.org/u-boot/[email protected]/ > >> > >> """ > >> - fixed issues due to modification of the environment in tests failing > >> other tests, by using unittest.mock.patch.dict() on os.environ as > >> suggested by the unittest.mock doc, > >> """ > >> > >> and you can check the diff between the v2 and v3 to check I used to > >> modify the env directly but now mock it instead. > >> > >> Sorry for not catching this, should have answered to Simon in the v2. > > > > In practice we try to set values for various which are important, so > > future tests should explicitly delete the var if needed. But I am OK > > This is not working. See this very simple example (too lazy to use > threading.Lock so synchronization done via time.sleep instead): > > """ > #!/usr/bin/env python3 > > import os > import time > import threading > > > def thread_func(n): > if n == 1: > time.sleep(1) > print(f'Thread {n} read environ var FOO={os.environ["FOO"]}') > if n == 1: > time.sleep(1) > print(f'Thread {n} set environ var FOO to foo{n}') > os.environ['FOO'] = f'foo{n}' > if n == 0: > time.sleep(5) > print(f'Thread {n} read environ var FOO={os.environ["FOO"]}') > if n == 0: > print(f'Thread {n} removes environ var FOO') > del os.environ["FOO"] > else: > time.sleep(10) > print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}') > > > threads = [] > > os.environ["FOO"] = "foo" > > for i in range(0, 2): > t = threading.Thread(target=thread_func, args=(i,)) > threads.append(t) > > for t in threads: > t.start() > > for t in threads: > t.join() > > """ > > This results in: > > """ > Thread 0 read environ var FOO=foo > Thread 0 set environ var FOO to foo0 > Thread 1 read environ var FOO=foo0 > Thread 1 set environ var FOO to foo1 > Thread 1 read environ var FOO=foo1 > Thread 0 read environ var FOO=foo1 > Thread 0 removes environ var FOO > Thread 1 read environ var FOO=None > """ > > You see that modification made to os.environ in a different thread > impacts the other threads. A test should definitely NOT modify anything > for another test, especially not when it's already running. > > So now, I implemented mocking instead (like in my tests for PKCS11 in > tools/binman/ftest.py) because I know it works. > > See: > > """ > #!/usr/bin/env python3 > > import os > import time > import threading > import unittest.mock > > > def thread_func(n): > if n == 1: > time.sleep(1) > print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}') > if n == 1: > time.sleep(1) > with unittest.mock.patch.dict('os.environ', > {'FOO': f'foo{n}'}): > print(f'Thread {n} set environ var FOO to foo{n}') > if n == 0: > time.sleep(5) > print(f'Thread {n} read mocked environ var > FOO={os.environ.get("FOO")}') > if n == 1: > time.sleep(6) > print(f'Thread {n} read environ var FOO={os.environ.get("FOO")}') > > > threads = [] > > for i in range(0, 2): > t = threading.Thread(target=thread_func, args=(i,)) > threads.append(t) > > for t in threads: > t.start() > > for t in threads: > t.join() > """ > > Lo and behold, it.... does NOT work??????? > > I get: > > """ > Thread 0 read environ var FOO=None > Thread 0 set environ var FOO to foo0 > Thread 1 read environ var FOO=foo0 > Thread 1 set environ var FOO to foo1 > Thread 1 read mocked environ var FOO=foo1 > Thread 0 read mocked environ var FOO=foo1 > Thread 0 read environ var FOO=None > Thread 1 read environ var FOO=foo0 > """ > > I've read that os.environ isn't thread-safe, due to setenv() in glibc > not being thread-safe: > https://sourceware.org/glibc/manual/latest/html_node/Environment-Access.html#Environment-Access-1 > > """ > Modifications of environment variables are not allowed in multi-threaded > programs. > """ > > I'm not sure if this applies to any other Python implementation than > CPython? But that is likely the one that most people are using. > > So... In short, I'm at a loss, no clue how to fix this (if it is even > fixable). The obvious answer is "spawn multiple processes instead of > multiple threads" but I can guarantee you, you don't want to be going > this route as multiprocessing is a lot of headaches in Python. We could > have the Python thread spawn a subprocess which has a different > environment if we wanted to (via the `env` command for example), but > that means not using binman Python API, rather its CLI. We could have > bintools accept an environment dict that needs to be passed via the > `env` command or the `env` kwargs of subprocess.Popen(). > > Headaches, headaches.
I would argue that requiring a particular environment var for a test is not a great idea. Better to pass an argument through the call stack and have the external program run with a new environment containing what is needed. Regards, Simon
