Not propagate system environment in run methods

[lolgab]

Use only forkEnv

Pull Request: #3392

[lihaoyi]

I'm not sure we do not want to propagate env vars in run. Unlike targets, commands are generally not expected to be hermetic. And if we expect run to be the same as launcher and then running it, we should expect environment variables to be propagated.

TBH I'm not sure what the right answer here is for how these things (forkEnv, forkArgs) all interact with run and launcher and assembly. What's our overall policy? Once we know that, then we can adjust the various implementations to match

[lolgab]

If we propagate it doesn't make sense that the default for forkEnv is T.env, it should be Map.empty and should be named forkExtraEnv or something.
I don't know which is the right answer, but we need to find one before 0.12.0 is released and be consistent with it.
The current behavior doesn't make much sense to me, naming and defaults are wrong and it doesn't work well in client-server mode.

[lihaoyi]

I think to make a decision here on what Mill should do, it's worth surveying what other tools do. How does SBT/Bazel/Gradle/Docker handle this sort of thing, where something (env vars, args, etc.) can be passed both in the build config as well as at the execution site?

From there we would be able to see if there's some common standard behavior or common set of tradeoffs that other similar tools are making, or whether there is no standard and everyone does it differently. And from there we can figure out what Mill should do

[lolgab]

I checked some of the other tools behavior as you suggested.

• Sbt has a task called envVars which is equivalent to Mill's forkEnv. It defaults to an empty map, as Sbt does propagate the system environment.
• Docker doesn't propagate the system environment, it does make sense though, since you are running a different operative system
• gradle sets the environment value to the gradle's process environment, and then users can add/remove environment variables.

After the discussions we had and given the other tools behaviors, I'd say the best tradeoff is to follow Sbt and set forkEnv to Map.empty[String, String].
This way we don't need to use a non-standard setting everywhere (propagateEnv = false), but at the same time we don't pass the same exact environment in forkEnv which is confusing (for example as removing an environment variable from the map doesn't actually remove it).

[lolgab]

Dropped in favor of #3442