Homogenize LocalEnvProvider instantiation in a single place.

This adds a new forCurrentOS factory method to LocalEnvProvider to construct
the correct local env provider for the current OS, and removes duplicate (and
inconsistent) logic from a bunch of places.

To make this change simpler, this also moves XcodeLocalEnvProvider from an
"apple" package (which is useless because it only contains one class and we
depend on in anyway in a Bazel build) to the "local" subpackage.

And as a side-effect, this gets rid of XcodeLocalEnvProviderTest, which I
don't understand how it could possibly have ever worked: the test was trying
to validate a condition on non-Darwin platforms but was later restricting
itself to running on Darwin platforms only.  We didn't see this because
this test was not being executed in the Bazel exported tree (that is, was
not run on BazelCI) and is now run after the code move.  I tried to fix the
test, but it's so out of sync with the current code that even understanding
what condition it wanted to test was hard (but it was not that useful).

RELNOTES: None.
PiperOrigin-RevId: 247663150

src/main/java/com/google/devtools/build/lib/BUILD

@@ -35,7 +35,6 @@ filegroup(
         "//src/main/java/com/google/devtools/build/lib/collect:srcs",
         "//src/main/java/com/google/devtools/build/lib/concurrent:srcs",
         "//src/main/java/com/google/devtools/build/lib/dynamic:srcs",
-        "//src/main/java/com/google/devtools/build/lib/exec/apple:srcs",
         "//src/main/java/com/google/devtools/build/lib/exec/local:srcs",
         "//src/main/java/com/google/devtools/build/lib/graph:srcs",
         "//src/main/java/com/google/devtools/build/lib/metrics:srcs",

src/main/java/com/google/devtools/build/lib/exec/local/BUILD

@@ -13,6 +13,7 @@ java_library(
         "LocalSpawnRunner.java",
         "PosixLocalEnvProvider.java",
         "WindowsLocalEnvProvider.java",
+        "XcodeLocalEnvProvider.java",
     ],
     data = [
         "//src/main/tools:process-wrapper",
@@ -26,6 +27,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/profiler",
+        "//src/main/java/com/google/devtools/build/lib/rules/apple",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//third_party:error_prone_annotations",

src/main/java/com/google/devtools/build/lib/exec/local/LocalEnvProvider.java

@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.exec.local;
 
 import com.google.devtools.build.lib.exec.BinTools;
+import com.google.devtools.build.lib.util.OS;
 import java.io.IOException;
 import java.util.Map;
 
@@ -23,14 +24,22 @@
  */
 public interface LocalEnvProvider {
 
-  public static final LocalEnvProvider UNMODIFIED =
-      new LocalEnvProvider() {
-        @Override
-        public Map<String, String> rewriteLocalEnv(
-            Map<String, String> env, BinTools binTools, String fallbackTmpDir) {
-          return env;
-        }
-      };
+  /**
+   * Creates a local environment provider for the current OS.
+   *
+   * @param clientEnv the environment variables as supplied by the Bazel client
+   * @return the local environment provider
+   */
+  static LocalEnvProvider forCurrentOs(Map<String, String> clientEnv) {
+    switch (OS.getCurrent()) {
+      case DARWIN:
+        return new XcodeLocalEnvProvider(clientEnv);
+      case WINDOWS:
+        return new WindowsLocalEnvProvider(clientEnv);
+      default:
+        return new PosixLocalEnvProvider(clientEnv);
+    }
+  }
 
   /**
    * Rewrites a {@code Spawn}'s the environment if necessary.

src/main/java/com/google/devtools/build/lib/exec/local/PosixLocalEnvProvider.java

@@ -26,6 +26,9 @@ public final class PosixLocalEnvProvider implements LocalEnvProvider {
   /**
    * Create a new {@link PosixLocalEnvProvider}.
    *
+   * <p>Use {@link LocalEnvProvider#forCurrentOs(Map)} to instantiate this unless the calling code
+   * is platform-specific.
+   *
    * @param clientEnv a map of the current Bazel command's environment
    */
   public PosixLocalEnvProvider(Map<String, String> clientEnv) {

src/main/java/com/google/devtools/build/lib/exec/local/WindowsLocalEnvProvider.java

@@ -26,9 +26,11 @@ public final class WindowsLocalEnvProvider implements LocalEnvProvider {
   /**
    * Create a new {@link WindowsLocalEnvProvider}.
    *
+   * <p>Use {@link LocalEnvProvider#forCurrentOs(Map)} to instantiate this.
+   *
    * @param clientEnv a map of the current Bazel command's environment
    */
-  public WindowsLocalEnvProvider(Map<String, String> clientEnv) {
+  WindowsLocalEnvProvider(Map<String, String> clientEnv) {
     this.clientEnv = clientEnv;
   }
 

src/main/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProvider.java → src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java

@@ -11,13 +11,12 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-package com.google.devtools.build.lib.exec.apple;
+package com.google.devtools.build.lib.exec.local;
 
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.exec.BinTools;
-import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
 import com.google.devtools.build.lib.rules.apple.AppleConfiguration;
 import com.google.devtools.build.lib.rules.apple.DottedVersion;
 import com.google.devtools.build.lib.shell.AbnormalTerminationException;
@@ -57,9 +56,11 @@ public final class XcodeLocalEnvProvider implements LocalEnvProvider {
   /**
    * Creates a new {@link XcodeLocalEnvProvider}.
    *
+   * <p>Use {@link LocalEnvProvider#forCurrentOs(Map)} to instantiate this.
+   *
    * @param clientEnv a map of the current Bazel command's environment
    */
-  public XcodeLocalEnvProvider(Map<String, String> clientEnv) {
+  XcodeLocalEnvProvider(Map<String, String> clientEnv) {
     this.clientEnv = clientEnv;
   }
 

src/main/java/com/google/devtools/build/lib/sandbox/BUILD

@@ -27,7 +27,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity",
-        "//src/main/java/com/google/devtools/build/lib/exec/apple",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
         "//src/main/java/com/google/devtools/build/lib/exec/local:options",
         "//src/main/java/com/google/devtools/build/lib/profiler",

src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java

@@ -27,7 +27,6 @@
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.exec.TreeDeleter;
-import com.google.devtools.build.lib.exec.apple.XcodeLocalEnvProvider;
 import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.runtime.ProcessWrapperUtil;
@@ -148,7 +147,7 @@ private static boolean computeIsSupported() {
     this.allowNetwork = SandboxHelpers.shouldAllowNetwork(cmdEnv.getOptions());
     this.alwaysWritableDirs = getAlwaysWritableDirs(cmdEnv.getRuntime().getFileSystem());
     this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv);
-    this.localEnvProvider = new XcodeLocalEnvProvider(cmdEnv.getClientEnv());
+    this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv());
     this.sandboxBase = sandboxBase;
     this.timeoutKillDelay = timeoutKillDelay;
     this.sandboxfsProcess = sandboxfsProcess;

src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java

@@ -30,7 +30,6 @@
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.exec.TreeDeleter;
 import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
-import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider;
 import com.google.devtools.build.lib.runtime.CommandCompleteEvent;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.runtime.ProcessWrapperUtil;
@@ -179,7 +178,7 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient)
     this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv);
     this.sandboxBase = sandboxBase;
     this.defaultImage = defaultImage;
-    this.localEnvProvider = new PosixLocalEnvProvider(cmdEnv.getClientEnv());
+    this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv());
     this.timeoutKillDelay = timeoutKillDelay;
     this.commandId = cmdEnv.getCommandId().toString();
     this.reporter = cmdEnv.getReporter();

src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java

@@ -18,9 +18,7 @@
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.exec.TreeDeleter;
-import com.google.devtools.build.lib.exec.apple.XcodeLocalEnvProvider;
 import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
-import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.runtime.ProcessWrapperUtil;
 import com.google.devtools.build.lib.util.OS;
@@ -60,10 +58,7 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
     super(cmdEnv);
     this.processWrapper = ProcessWrapperUtil.getProcessWrapper(cmdEnv);
     this.execRoot = cmdEnv.getExecRoot();
-    this.localEnvProvider =
-        OS.getCurrent() == OS.DARWIN
-            ? new XcodeLocalEnvProvider(cmdEnv.getClientEnv())
-            : new PosixLocalEnvProvider(cmdEnv.getClientEnv());
+    this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv());
     this.sandboxBase = sandboxBase;
     this.timeoutKillDelay = timeoutKillDelay;
     this.treeDeleter = treeDeleter;

src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java

@@ -33,11 +33,9 @@
 import com.google.devtools.build.lib.exec.ExecutorBuilder;
 import com.google.devtools.build.lib.exec.SpawnRunner;
 import com.google.devtools.build.lib.exec.TreeDeleter;
-import com.google.devtools.build.lib.exec.apple.XcodeLocalEnvProvider;
 import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
 import com.google.devtools.build.lib.exec.local.LocalExecutionOptions;
 import com.google.devtools.build.lib.exec.local.LocalSpawnRunner;
-import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.SilentCloseable;
 import com.google.devtools.build.lib.runtime.BlazeModule;
@@ -374,17 +372,12 @@ private static SpawnRunner withFallback(CommandEnvironment env, SpawnRunner sand
   private static SpawnRunner createFallbackRunner(CommandEnvironment env) {
     LocalExecutionOptions localExecutionOptions =
         env.getOptions().getOptions(LocalExecutionOptions.class);
-    LocalEnvProvider localEnvProvider =
-        OS.getCurrent() == OS.DARWIN
-            ? new XcodeLocalEnvProvider(env.getClientEnv())
-            : new PosixLocalEnvProvider(env.getClientEnv());
-    return
-        new LocalSpawnRunner(
-            env.getExecRoot(),
-            localExecutionOptions,
-            ResourceManager.instance(),
-            localEnvProvider,
-            env.getBlazeWorkspace().getBinTools());
+    return new LocalSpawnRunner(
+        env.getExecRoot(),
+        localExecutionOptions,
+        ResourceManager.instance(),
+        LocalEnvProvider.forCurrentOs(env.getClientEnv()),
+        env.getBlazeWorkspace().getBinTools());
   }
 
   private static final class SandboxFallbackSpawnRunner implements SpawnRunner {

src/main/java/com/google/devtools/build/lib/standalone/BUILD

@@ -17,11 +17,9 @@ java_library(
     ],
     deps = [
         "//src/main/java/com/google/devtools/build/lib:build-base",
-        "//src/main/java/com/google/devtools/build/lib:os_util",
         "//src/main/java/com/google/devtools/build/lib:runtime",
         "//src/main/java/com/google/devtools/build/lib:testing-support-rules",
         "//src/main/java/com/google/devtools/build/lib/actions",
-        "//src/main/java/com/google/devtools/build/lib/exec/apple",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
         "//src/main/java/com/google/devtools/build/lib/exec/local:options",
         "//src/main/java/com/google/devtools/build/lib/rules/cpp",

src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java

@@ -25,16 +25,12 @@
 import com.google.devtools.build.lib.exec.SpawnRunner;
 import com.google.devtools.build.lib.exec.StandaloneTestStrategy;
 import com.google.devtools.build.lib.exec.TestStrategy;
-import com.google.devtools.build.lib.exec.apple.XcodeLocalEnvProvider;
 import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
 import com.google.devtools.build.lib.exec.local.LocalExecutionOptions;
 import com.google.devtools.build.lib.exec.local.LocalSpawnRunner;
-import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider;
-import com.google.devtools.build.lib.exec.local.WindowsLocalEnvProvider;
 import com.google.devtools.build.lib.rules.test.ExclusiveTestStrategy;
 import com.google.devtools.build.lib.runtime.BlazeModule;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.Path;
 
 /**
@@ -56,11 +52,18 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
             env.getBlazeWorkspace().getBinTools(),
             testTmpRoot);
 
+    SpawnRunner localSpawnRunner =
+        new LocalSpawnRunner(
+            env.getExecRoot(),
+            env.getOptions().getOptions(LocalExecutionOptions.class),
+            ResourceManager.instance(),
+            LocalEnvProvider.forCurrentOs(env.getClientEnv()),
+            env.getBlazeWorkspace().getBinTools());
+
     // Order of strategies passed to builder is significant - when there are many strategies that
     // could potentially be used and a spawnActionContext doesn't specify which one it wants, the
     // last one from strategies list will be used
-    builder.addActionContext(
-        new StandaloneSpawnStrategy(env.getExecRoot(), createLocalRunner(env)));
+    builder.addActionContext(new StandaloneSpawnStrategy(env.getExecRoot(), localSpawnRunner));
     builder.addActionContext(testStrategy);
     builder.addActionContext(new ExclusiveTestStrategy(testStrategy));
     builder.addActionContext(new FileWriteStrategy());
@@ -74,22 +77,4 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
     // necessarily the default.
     builder.addStrategyByContext(SpawnActionContext.class, "standalone");
   }
-
-  private static SpawnRunner createLocalRunner(CommandEnvironment env) {
-    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,
-            ResourceManager.instance(),
-            localEnvProvider,
-            env.getBlazeWorkspace().getBinTools());
-  }
 }

src/main/java/com/google/devtools/build/lib/worker/BUILD

@@ -16,12 +16,10 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:build-base",
         "//src/main/java/com/google/devtools/build/lib:events",
         "//src/main/java/com/google/devtools/build/lib:io",
-        "//src/main/java/com/google/devtools/build/lib:os_util",
         "//src/main/java/com/google/devtools/build/lib:resource-converter",
         "//src/main/java/com/google/devtools/build/lib:runtime",
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib/actions",
-        "//src/main/java/com/google/devtools/build/lib/exec/apple",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
         "//src/main/java/com/google/devtools/build/lib/exec/local:options",
         "//src/main/java/com/google/devtools/build/lib/sandbox",

src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java

@@ -27,18 +27,14 @@
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.exec.ExecutorBuilder;
 import com.google.devtools.build.lib.exec.SpawnRunner;
-import com.google.devtools.build.lib.exec.apple.XcodeLocalEnvProvider;
 import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
 import com.google.devtools.build.lib.exec.local.LocalExecutionOptions;
 import com.google.devtools.build.lib.exec.local.LocalSpawnRunner;
-import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider;
-import com.google.devtools.build.lib.exec.local.WindowsLocalEnvProvider;
 import com.google.devtools.build.lib.runtime.BlazeModule;
 import com.google.devtools.build.lib.runtime.Command;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.runtime.commands.CleanCommand.CleanStartingEvent;
 import com.google.devtools.build.lib.sandbox.SandboxOptions;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.worker.WorkerOptions.MultiResourceConverter;
 import com.google.devtools.common.options.OptionsBase;
@@ -145,7 +141,7 @@ 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);
+    LocalEnvProvider localEnvProvider = LocalEnvProvider.forCurrentOs(env.getClientEnv());
     WorkerSpawnRunner spawnRunner =
         new WorkerSpawnRunner(
             env.getExecRoot(),
@@ -176,14 +172,6 @@ private static SpawnRunner createFallbackRunner(
         env.getBlazeWorkspace().getBinTools());
   }
 
-  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) {