Use nailgun environment in forked processes #413
Conversation
| @@ -9,8 +9,8 @@ object Integrations { | |||
| val MiniBetterFiles = RootProject(uri( | |||
| "git://github.com/scalacenter/mini-better-files.git#0ed848993a2fd5a36e4366b5efb9c68dce958fc2")) | |||
| val WithResources = RootProject( | |||
| uri("git://github.com/scalacenter/with-resources.git#7529b2c3ac455cbb1889d4791c4e0d4957e29306")) | |||
| uri("git://github.com/scalacenter/with-resources.git#f0a46830cae7ef6282d9bba64b6da34bae18f339")) | |||
There was a problem hiding this comment.
These new hashes contain modifications to the dummy code to fail if the env variable BLOOP_OWNER is not accessible via sys.env.
|
What this means is that now Bloop will use the environment variable of the places where the bloop client is involved (e.g. your terminal) to run and test code. |
jvican
force-pushed
the
topic/expose-env-variables
branch
4 times, most recently
from
5d35914
to
e161985
Compare
|
This is ready for review 🎉. |
| @@ -47,6 +47,7 @@ final case class ForkProcess(javaEnv: JavaEnv, classpath: Array[AbsolutePath]) { | |||
| className: String, | |||
| args: Array[String], | |||
| logger: Logger, | |||
| env: Properties, | |||
| @@ -68,6 +69,9 @@ final case class ForkProcess(javaEnv: JavaEnv, classpath: Array[AbsolutePath]) { | |||
| } else { | |||
| val processBuilder = new ProcessBuilder(cmd: _*) | |||
| processBuilder.directory(cwd.toFile) | |||
| val processEnv = processBuilder.environment() | |||
| processEnv.clear() | |||
| env.forEach { case (k: String, v: String) => processEnv.put(k, v); () } | |||
There was a problem hiding this comment.
I think that you can use putAll here
| @@ -68,6 +69,9 @@ final case class ForkProcess(javaEnv: JavaEnv, classpath: Array[AbsolutePath]) { | |||
| } else { | |||
| val processBuilder = new ProcessBuilder(cmd: _*) | |||
| processBuilder.directory(cwd.toFile) | |||
| val processEnv = processBuilder.environment() | |||
| processEnv.clear() | |||
There was a problem hiding this comment.
Are we sure that we want to discard everything?
There was a problem hiding this comment.
Yeah, I elaborated on my other comment.
| @@ -79,7 +73,8 @@ object TestInternals { | |||
| fork: ForkProcess, | |||
| discoveredTests: DiscoveredTests, | |||
| eventHandler: EventHandler, | |||
| logger: Logger): Unit = { | |||
| logger: Logger, | |||
| env: Properties): Unit = { | |||
| import scala.collection.JavaConverters._ | ||
| System.getenv().asScala.foldLeft(new Properties()) { | ||
| case (props, (key, value)) => props.setProperty(key, value); props | ||
| } |
This PR will not expose any of the server's environment variables to the test and run tasks, so yeah, if users expect those to be in they won't. I think that doing this is the right behaviour, and it's very unlikely someone would want env variables from the server for these particular tasks. However, the rest of the tasks will still use the environment variables of the server (as expected). |
jvican
force-pushed
the
topic/expose-env-variables
branch
from
0bfacfe
to
704169e
Compare
This commit propagates the `remoteEnv` properties in the nailgun context to the place where we run and test projects in forked projects. Thanks to the fact that we always fork, we're able to use the environment variables from the user without weird hacks to mutate `System.getenv` and scope it per client. Fixes #335.
The problem is that we were expecting 5 sources instead of 6. This commit fixes that and adds more cases to make the whole test more reliable.
Something is acting up.
jvican
force-pushed
the
topic/expose-env-variables
branch
from
704169e
to
87181dc
Compare
This commit propagates the
remoteEnvproperties in the nailgun contextto the place where we run and test projects in forked projects. Thanks
to the fact that we always fork, we're able to use the environment
variables from the user without weird hacks to mutate
System.getenvand scope it per client.
Fixes #335.