[PATCH v3 1/2] binman: bintool: add extra_env parameter to run_cmd

[Simon Glass]

Hi Quentin,

On Thu, 7 May 2026 at 07:19, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> Hi Simon,
>
> On 5/1/26 4:10 AM, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Mon, 27 Apr 2026 at 03:48, Quentin Schulz <quentin.schulz at cherry.de> wrote:
> >>
> >> Hi Sergio,
> >>
> >> On 4/23/26 5:45 PM, Sergio Prado wrote:
> >>> [You don't often get email from sergio.prado at e-labworks.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >>>
> >>> Add an optional extra_env dict parameter to run_cmd() and
> >>> run_cmd_result(). When provided, the key/value pairs are merged into
> >>> the subprocess environment for that invocation only, leaving the parent
> >>> process environment unchanged.
> >>>
> >>> This might be needed by bintools that must inject environment variables
> >>> into specific tool calls without polluting the global environment.
> >>>
> >>
> >> Yeah, I'm not convinced this is useful.
> >>
> >> We would need to propagate this new variable to whatever calls those
> >> methods as well. And it quickly balloons.
> >>
> >> Just mock the environment with
> >>
> >> with unittest.mock.patch.dict('os.environ', {'MYVAR': 'myval'}): like in
> >> testFitSignPKCS11Object for example?
> >
> > While we can mock the environment, it becomes confusing as the mocks
> > pile up. The code path in the test effectively differs from the actual
> > code path, since the mock is overriding something that it assumes the
> > underlying code does, so any changes can cause strange errors.
> >
>
> Sorry, I don't understand what you meant here, can you rephrase?

Basically mocks are fragile since they assume certain behaviour of the
implementation. Change that and the mocks break in confusing ways.

My understanding of the justification for NOT passing the env argument
is that it is annoying to do that plumbing. But I would rather do that
plumbing. Perhaps not stated, but one of Binman's design goals is to
try to have callers pass in their requirements rather than relying on
state or sideways data (although there is plenty of that even so :-)

>
> > The normal approach with binman tests is to pass through the settings
> > they want. See for example _DoTestFile()
> >
> > If you look through the tests, there is almost no mocking despite the
> > large amount of functionality provided by Binman.
> >
>
> How many of those features are actually dependent on environment variables?
>
> Here, configuring OpenSSL, SOFTHSM2, and OpenSSL providers, we clearly
> are in the same scenario as when doing FIT signing with OpenSSL engines.
>
> >>
> >> I'm also very skeptical of the need for binman to *set* environment
> >> variables to setup openssl.
> >>
> >> Look at what's been done for testFitSignPKCS11Object, we externally
> >> configure openssl and softhsm so that binman doesn't have to know about
> >> it. This avoids having to play smart and adding a bunch of new
> >> variables. Is it possible to implement something like that for the x509
> >> certificates? For FIT, we do use fit,engine,
> >> fit,engine-keydir/key-name-hint with mkimage to (possibly) use pkcs11,
> >> custom openssl engines, etc..
> >>
> >> Also, I don't think we should decide for the user whether they are
> >> forced to use a provider or an engine based on the presence of the
> >> provider? just let them configure openssl to use the one they want
> >> externally?
> >
> > Fair enough. I wonder how the tests deal with something which is
> > externally configured, though?
> >
>
> You set the OPENSSL_CONF, OPENSSL_ENGINES and SOFTHSM2_CONF environment
> variables to point at files that are generated for the tests. See the
> various unittest.mock.patch.dict('os.environ',) in tools/binman/ftest.py
> and tools/binman/test/fit/openssl.conf as well as
> tools/binman/test/fit/softhsm2.conf.

Right, so the tests determine / set up the environment and then test
accordingly. That's fine. I'd just like to avoid mocks here.

Regards,
Simon