Ugh, this makes the entire class not thread safe. IMHO we should instead create a copy of os.environ, alter that one and pass it down to the subprocess calls we end up invoking.

In that case we need to add a subprocess helper and require people to always use it, which is very limiting and requires people to change their code. Most people are using it in single threaded code, so it would be very disruptive IMO.

What about adding a keep_env/skip_env argument, defaulting to False, to disable the environment variable modification, and add both a subprocess helper and a env attribute with the env that should be used in subprocess invocations? The bad side is that people running this in multi-threaded/parallel situations would have to opt-in, but this way it would not disrupt existing code and would keep the API simple for single-threaded code, which is most of it. I feel this compromise is reasonable, what do you think?

Sounds reasonable if that's the case 👍 I thought this might be easier considering we already overwrite the pep517 packages subprocess invocation with our own as far as I remember 🤔 (so we should now that part).

Hum, actually, even if a bit disruptive, I am now leading towards update_env argument, with the opposite function. The API I proposed above motivates? (I don't remember the word I was looking for, something along those lines but that made sense in this sentence) non thread safe APIs, which is something we should probably avoid. Worse case scenario, people will get the exact same behavior as currently, they just might run into #373. And this would technically be a breaking change in thread safety, even though the line there is a bit blurry.

We 'overwrite' it in the project builder for all builds. If we ovewrite it in the isolated env class then users will have to pass the isolated env's runner to the project builder and we'd need to provide some sort of function to wrap user-provided subprocess runners if they are to be used in conjunction with an isolated env. We should definitely not add a flag AND a subprocess runner wrapper AND an env attribute AND optionally mutate os.environ.

It shouldn't need to interface with the builder, only with the isolated env, right? So you could slot in any isolated env you like:

with IsolatedEnvBuilder(...) as isolated_env:
    ProjectBuilder.from_isolated_env(isolated_env)

# Or...

with MyCustomEnvBuilderWhichReturnsAnIsolatedEnvSubclass() as isolated_env:
    ProjectBuilder.from_isolated_env(isolated_env)

It shouldn't need to interface with the builder, only with the isolated env, right? So you could slot in any isolated env you like:

Hum, sure. That looks good to me.

I am not sure if it would make sense to make it public API, probably not.

Actually, as long as we keep the API simple, I think it would be alright.

Actually, as long as we keep the API simple, I think it would be alright.

Then we cannot go down by adopting my pep-517 implementation path. The API is purposefully not simple because encourages maximum flexibility. The entire frontend is public and non-trivial https://github.com/tox-dev/tox/blob/rewrite/src/tox/util/pep517/frontend.py#L1

Could you create a new issue explaining how this differs from pep517 and how to proceed with adopting it?

Sorry, don't have time for that. At its core it differs by:

• allow keeping alive the backend to reuse it in between commands
• provides stdout/stderr for commands executed
• frontend python 3 only and type hinted 👍