Repository: bazelbuild/bazel
Issue: Some environment variables not passed to persistent workers

I'm working on creating a persistent worker to support incremental compilation [for Swift](https://github.com/bazelbuild/rules_swift/issues/48). In doing so, the first issue I hit, was the lack of required environment variables being passed to the newly created persistent workers.

When running the builds normally, this is the environment passed to the build:

```
DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer
TMPDIR=/var/folders/5b/bs2_gmk512147rfb9c6278f80000gp/T/
SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator12.0.sdk
XCODE_VERSION_OVERRIDE=10.0.0
__CF_USER_TEXT_ENCODING=0x1F6:0x0:0x0
PWD=/private/var/tmp/_bazel_...
SHLVL=1
APPLE_SDK_PLATFORM=iPhoneSimulator
APPLE_SDK_VERSION_OVERRIDE=12.0
```

But the persistent workers are launched with this environment:

```
XCODE_VERSION_OVERRIDE=10.0.0
PWD=/private/var/tmp/_bazel_...
SHLVL=1
APPLE_SDK_PLATFORM=iPhoneSimulator
APPLE_SDK_VERSION_OVERRIDE=12.0
```

This results in the underlying [script](https://github.com/bazelbuild/rules_swift/blob/c8138ccf8cd798aefc9533840fcab72546e4c46c/tools/wrappers/xcrunwrapper.sh#L38) to fail since `SDKROOT` is required but not defined.

Is this variable intentionally excluded from persistent workers? Or is there a place we need to whitelist this variable to be passed along? I am testing with bazel 0.17.1

Issue comments:
`DEVELOPER_DIR` is also missing from the persistent worker env here
Ok I think I discovered why this is the case. When building locally we hit this codepath: https://github.com/bazelbuild/bazel/blob/18bd7dec9609ff8924d9ebed5f14be523df4eec0/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java#L224-L225 which adds the variables we want from here https://github.com/bazelbuild/bazel/blob/1f684e1b87cd8881a0a4b33e86ba66743e32d674/src/main/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProvider.java#L66-L108 but persistent workers don't call this local env rewrite logic https://github.com/bazelbuild/bazel/blob/5a1ed1f24e74f8a608ee145133f399c1f34a8390/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java#L131 meaning they don't add the variables we were hoping for.

I'm not really sure what an acceptable solution for this is since I don't think normally we'd want to pass persistent workers the local environment logic, but in this case it's required for quite a few arguments.
@allevato assuming you have some context on these, any thoughts?
I've run into the same problem in my worker prototype, and my solution was to hack it by having my wrapper set DEVELOPER_DIR and SDKROOT by querying xcode-select/xcrun manually if they're not already set. Not ideal.
Sounds like a problem with persistent workers. @keith wanna send us a patch? :-) cc: @philwo 
@buchgr I'm happy to submit that fix, but is that a reasonable one? Just based on the naming tying the local environment objects to the persistent workers sounds like they're intentionally attached
I've submitted a PR that fixes it in this way, please let me know what you think! https://github.com/bazelbuild/bazel/pull/6287

Pull request review comments:
I've seen some different things passed for `/tmp` here, I assume there's something other than this to be more platform agnostic that I should pass?

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
index 46bb0dc554547c..df23d6d5d8ad0f 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
@@ -140,13 +140,15 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
     Preconditions.checkNotNull(workerPool);
     ImmutableMultimap<String, String> extraFlags =
         ImmutableMultimap.copyOf(env.getOptions().getOptions(WorkerOptions.class).workerExtraFlags);
+    LocalEnvProvider localEnvProvider = createLocalEnvProvider(env);
     WorkerSpawnRunner spawnRunner =
         new WorkerSpawnRunner(
             env.getExecRoot(),
             workerPool,
             extraFlags,
             env.getReporter(),
-            createFallbackRunner(env),
+            createFallbackRunner(env, localEnvProvider),
+            localEnvProvider,
             env.getOptions()
                 .getOptions(SandboxOptions.class)
                 .symlinkedSandboxExpandsTreeArtifactsInRunfilesTree);
@@ -156,15 +158,9 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
     builder.addStrategyByContext(SpawnActionContext.class, "worker");
   }
 
-  private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
+  private static SpawnRunner createFallbackRunner(CommandEnvironment env, LocalEnvProvider localEnvProvider) {
     LocalExecutionOptions localExecutionOptions =
         env.getOptions().getOptions(LocalExecutionOptions.class);
-    LocalEnvProvider localEnvProvider =
-        OS.getCurrent() == OS.DARWIN
-            ? new XcodeLocalEnvProvider(env.getClientEnv())
-            : (OS.getCurrent() == OS.WINDOWS
-                ? new WindowsLocalEnvProvider(env.getClientEnv())
-                : new PosixLocalEnvProvider(env.getClientEnv()));
     return new LocalSpawnRunner(
         env.getExecRoot(),
         localExecutionOptions,
@@ -172,6 +168,14 @@ private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
         localEnvProvider);
   }
 
+  private static LocalEnvProvider createLocalEnvProvider(CommandEnvironment env) {
+    return OS.getCurrent() == OS.DARWIN
+        ? new XcodeLocalEnvProvider(env.getClientEnv())
+        : (OS.getCurrent() == OS.WINDOWS
+            ? new WindowsLocalEnvProvider(env.getClientEnv())
+            : new PosixLocalEnvProvider(env.getClientEnv()));
+  }
+
   @Subscribe
   public void buildComplete(BuildCompleteEvent event) {
     if (options != null && options.workerQuitAfterBuild) {
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
index 9b1c8652e38562..50473f2967a0c8 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
@@ -19,7 +19,6 @@
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Multimap;
 import com.google.common.hash.HashCode;
@@ -37,6 +36,7 @@
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.exec.SpawnRunner;
+import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
 import com.google.devtools.build.lib.sandbox.SandboxHelpers;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.Path;
@@ -76,6 +76,7 @@ final class WorkerSpawnRunner implements SpawnRunner {
   private final Multimap<String, String> extraFlags;
   private final EventHandler reporter;
   private final SpawnRunner fallbackRunner;
+  private final LocalEnvProvider localEnvProvider;
   private final boolean sandboxUsesExpandedTreeArtifactsInRunfiles;
 
   public WorkerSpawnRunner(
@@ -84,12 +85,14 @@ public WorkerSpawnRunner(
       Multimap<String, String> extraFlags,
       EventHandler reporter,
       SpawnRunner fallbackRunner,
+      LocalEnvProvider localEnvProvider,
       boolean sandboxUsesExpandedTreeArtifactsInRunfiles) {
     this.execRoot = execRoot;
     this.workers = Preconditions.checkNotNull(workers);
     this.extraFlags = extraFlags;
     this.reporter = reporter;
     this.fallbackRunner = fallbackRunner;
+    this.localEnvProvider = localEnvProvider;
     this.sandboxUsesExpandedTreeArtifactsInRunfiles = sandboxUsesExpandedTreeArtifactsInRunfiles;
   }
 
@@ -128,7 +131,8 @@ private SpawnResult actuallyExec(Spawn spawn, SpawnExecutionContext context)
     // its args and put them into the WorkRequest instead.
     List<String> flagFiles = new ArrayList<>();
     ImmutableList<String> workerArgs = splitSpawnArgsIntoWorkerArgsAndFlagFiles(spawn, flagFiles);
-    ImmutableMap<String, String> env = spawn.getEnvironment();
+    Map<String, String> env =
+        localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, "/tmp");
 
     MetadataProvider inputFileCache = context.getMetadataProvider();
 


Test patch: