Fix #2881 Propagate environment from ctx to subprocesses
#2882
Conversation
main/util/src/mill/util/Jvm.scala
Outdated
| stderr = backgroundOutputs.map(_._2).getOrElse(os.Pipe), | ||
| propagateEnv = false |
There was a problem hiding this comment.
I'm not convinced this is correct. Yes, in order to avoid using stale env vars from the server, we need to set propagaEnv to false. But I think we should then always use the env vars send to us from the client as a base line. And if some env vars are given by the parameters, we should add them too.
This logic is of course not easily applyable here, as we don't have the access to the target context here.
There was a problem hiding this comment.
The test case should use a env var typically used by the OS which is now unset.
There was a problem hiding this comment.
I did not know that envArgs are meant to be additional environment variables and not all the environment to be passed to the process.
I'm not sure how to achieve this to be honest..
We could add a propagateEnv: Boolean = true parameter here and set it to false when we know that we don't want to propagate the environment.
We could also add a private global AtomicReference holding the current env and set it when we read the environment from client and read it here.
Do you have other ideas?
There was a problem hiding this comment.
TBH, we never specified whether a run with an empty forkEnv should use the OS env or the empty env. But the status quo seems to exactly do this. But in run we have access to a task context, so we could also accept an implicit one in the Jvm API.
Without passing it explicitly/implicitly, holding a static (global) map might be the alternative.
There was a problem hiding this comment.
TBH, we never specified whether a
runwith an emptyforkEnvshould use the OS env or the empty env. But the status quo seems to exactly do this. But inrunwe have access to a task context, so we could also accept an implicit one in theJvmAPI.
We could change this but, in case, I would prefer to do it in Mill 0.12 breaking window.
Without passing it explicitly/implicitly, holding a static (global) map might be the alternative.
This seems the easiest solution as it doesn't need to introduce new APIs / deprecate APIs. Also, the environment is stable enough since you can't change it at runtime on the JVM. So the variable would change not that often (at most once per CLI execution).
There was a problem hiding this comment.
I ended up adding some private methods that receive also the propagateEnv argument so we can pass the env we have already in an implicit ctx: Ctx in Jvm. I don't want to make these overloads public since in 0.12 it's better to add (implicit ctx: Ctx) to the methods instead.
the current semantics of runSubprocess methods are that the env arguments are not the whole env but extra arguments passed added to the system environment. To allow passing the client env we need to use ctx. We can't add overloads with an implicit parameter list so I'm adding private overloads that recieve a `propagateEnv: Boolean` parameter to be passed to os-lib. This API is not what we want and will be revisited in Mill 0.12, that's why I made the new methods private.
|
Now I don't have idea how to test this. Do we have integration tests where the client is used to call |
lolgab
changed the title
ctx to subprocesses
lolgab
added
the
compat-breaker
|
Superseded by #3437 |
Fixes #2881
Dependencies
propagateEnvFix propagateEnv = false to not propagate env os-lib#238