Extract the MacOS/XCode env rewrite logic into lib.exec.apple

Also add an interface to allow injecting that logic into LocalSpawnRunner; this
is in preparation for rewriting StandaloneSpawnStrategy to use
LocalSpawnRunner.

At the same time, this reduces the dependencies from exec / standalone to
rules.apple, which is a prerequisite for micro-Bazel.

There's a small semantic change hidden here - we now only set the new
XCodeLocalEnvProvider if we're actually running on Darwin, so we no longer
fail execution on non-Darwin platforms if XCODE_VERSION_OVERRIDE or
APPLE_SDK_VERSION_OVERRIDE is set. As a result, I moved the corresponding test
from StandaloneSpawnStrategyTest to the new XCodeLocalEnvProviderTest.

While I'm at it, also open source DottedVersionTest and CacheManagerTest.

PiperOrigin-RevId: 158829077

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

@@ -24,6 +24,7 @@ filegroup(
         "//src/main/java/com/google/devtools/build/lib/buildeventservice/client:srcs",
         "//src/main/java/com/google/devtools/build/lib/causes:srcs",
         "//src/main/java/com/google/devtools/build/lib/cmdline: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/query2:srcs",
         "//src/main/java/com/google/devtools/build/lib/remote:srcs",

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

@@ -0,0 +1,24 @@
+package(
+    default_visibility = ["//src:__subpackages__"],
+)
+
+java_library(
+    name = "apple",
+    srcs = glob(["*.java"]),
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib:build-base",
+        "//src/main/java/com/google/devtools/build/lib:packages-internal",
+        "//src/main/java/com/google/devtools/build/lib:shell",
+        "//src/main/java/com/google/devtools/build/lib:util",
+        "//src/main/java/com/google/devtools/build/lib:vfs",
+        "//src/main/java/com/google/devtools/build/lib/exec/local",
+        "//src/main/java/com/google/devtools/build/lib/rules/apple",
+        "//third_party:guava",
+    ],
+)
+
+filegroup(
+    name = "srcs",
+    testonly = 0,  # All srcs should be not test only, overwrite package default.
+    srcs = glob(["**"]),
+)

src/main/java/com/google/devtools/build/lib/rules/apple/CacheManager.java → src/main/java/com/google/devtools/build/lib/exec/apple/CacheManager.java

@@ -12,19 +12,17 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.devtools.build.lib.rules.apple;
+package com.google.devtools.build.lib.exec.apple;
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
-
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.util.List;
-
 import javax.annotation.Nullable;
 
 /**
@@ -42,7 +40,7 @@
  * threads may write the same entry to cache. This is fine, as retrieval from the cache will simply
  * return the first found entry.
  */
-class CacheManager {
+final class CacheManager {
 
   private final Path cacheFilePath;
   private boolean cacheFileTouched;

src/main/java/com/google/devtools/build/lib/rules/apple/AppleHostInfo.java → src/main/java/com/google/devtools/build/lib/exec/apple/XCodeLocalEnvProvider.java

@@ -1,4 +1,4 @@
-// Copyright 2015 The Bazel Authors. All rights reserved.
+// Copyright 2017 The Bazel Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -11,34 +11,76 @@
 // 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.rules.apple;
+package com.google.devtools.build.lib.exec.apple;
 
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
+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;
 import com.google.devtools.build.lib.shell.Command;
 import com.google.devtools.build.lib.shell.CommandException;
 import com.google.devtools.build.lib.shell.CommandResult;
 import com.google.devtools.build.lib.shell.TerminationStatus;
+import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.Path;
-
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.util.Map;
 
 /**
- * Obtains information pertaining to Apple host machines required for using Apple toolkits in
- * local action execution.
+ * Adds to the given environment all variables that are dependent on system state of the host
+ * machine.
+ *
+ * <p>Admittedly, hermeticity is "best effort" in such cases; these environment values should be
+ * as tied to configuration parameters as possible.
+ *
+ * <p>For example, underlying iOS toolchains require that SDKROOT resolve to an absolute system
+ * path, but, when selecting which SDK to resolve, the version number comes from build
+ * configuration.
  */
-public class AppleHostInfo {
-
+public final class XCodeLocalEnvProvider implements LocalEnvProvider {
   private static final String XCRUN_CACHE_FILENAME = "__xcruncache";
   private static final String XCODE_LOCATOR_CACHE_FILENAME = "__xcodelocatorcache";
 
+  @Override
+  public Map<String, String> rewriteLocalEnv(
+      Map<String, String> env, Path execRoot, String productName) throws IOException {
+    boolean containsXcodeVersion = env.containsKey(AppleConfiguration.XCODE_VERSION_ENV_NAME);
+    boolean containsAppleSdkVersion =
+        env.containsKey(AppleConfiguration.APPLE_SDK_VERSION_ENV_NAME);
+    if (!containsXcodeVersion && !containsAppleSdkVersion) {
+      return env;
+    }
+
+    ImmutableMap.Builder<String, String> newEnvBuilder = ImmutableMap.builder();
+    newEnvBuilder.putAll(env);
+    // Empty developer dir indicates to use the system default.
+    // TODO(bazel-team): Bazel's view of the xcode version and developer dir should be explicitly
+    // set for build hermeticity.
+    String developerDir = "";
+    if (containsXcodeVersion) {
+      String version = env.get(AppleConfiguration.XCODE_VERSION_ENV_NAME);
+      developerDir = getDeveloperDir(execRoot, DottedVersion.fromString(version), productName);
+      newEnvBuilder.put("DEVELOPER_DIR", developerDir);
+    }
+    if (containsAppleSdkVersion) {
+      // The Apple platform is needed to select the appropriate SDK.
+      if (!env.containsKey(AppleConfiguration.APPLE_SDK_PLATFORM_ENV_NAME)) {
+        throw new IOException("Could not resolve apple platform for determining SDK");
+      }
+      String iosSdkVersion = env.get(AppleConfiguration.APPLE_SDK_VERSION_ENV_NAME);
+      String appleSdkPlatform = env.get(AppleConfiguration.APPLE_SDK_PLATFORM_ENV_NAME);
+      newEnvBuilder.put(
+          "SDKROOT",
+          getSdkRoot(execRoot, developerDir, iosSdkVersion, appleSdkPlatform, productName));
+    }
+    return newEnvBuilder.build();
+  }
+
   /**
    * Returns the absolute root path of the target Apple SDK on the host system for a given
    * version of xcode (as defined by the given {@code developerDir}). This may spawn a
@@ -51,12 +93,15 @@ public class AppleHostInfo {
    * @param sdkVersion the sdk version, for example, "9.1"
    * @param appleSdkPlatform the sdk platform, for example, "iPhoneOS"
    * @param productName the product name
-   * @throws UserExecException if there is an issue with obtaining the root from the spawned
+   * @throws IOException if there is an issue with obtaining the root from the spawned
    *     process, either because the SDK platform/version pair doesn't exist, or there was an
    *     unexpected issue finding or running the tool
    */
-  public static String getSdkRoot(Path execRoot, String developerDir,
-      String sdkVersion, String appleSdkPlatform, String productName) throws UserExecException {
+  private static String getSdkRoot(Path execRoot, String developerDir,
+      String sdkVersion, String appleSdkPlatform, String productName) throws IOException {
+    if (OS.getCurrent() != OS.DARWIN) {
+      throw new IOException("Cannot locate iOS SDK on non-darwin operating system");
+    }
     try {
       CacheManager cacheManager =
           new CacheManager(execRoot.getRelative(
@@ -84,7 +129,7 @@ public static String getSdkRoot(Path execRoot, String developerDir,
       TerminationStatus terminationStatus = e.getResult().getTerminationStatus();
 
       if (terminationStatus.exited()) {
-        throw new UserExecException(
+        throw new IOException(
             String.format("xcrun failed with code %s.\n"
                 + "This most likely indicates that SDK version [%s] for platform [%s] is "
                 + "unsupported for the target version of xcode.\n"
@@ -98,9 +143,9 @@ public static String getSdkRoot(Path execRoot, String developerDir,
       String message = String.format("xcrun failed.\n%s\n%s",
           e.getResult().getTerminationStatus(),
           new String(e.getResult().getStderr(), StandardCharsets.UTF_8));
-      throw new UserExecException(message, e);
-    } catch (CommandException | IOException e) {
-      throw new UserExecException(e);
+      throw new IOException(message, e);
+    } catch (CommandException e) {
+      throw new IOException(e);
     }
   }
 
@@ -113,15 +158,20 @@ public static String getSdkRoot(Path execRoot, String developerDir,
    * @param execRoot the execution root path, used to locate the cache file
    * @param version the xcode version number to look up
    * @param productName the product name
-   * @throws UserExecException if there is an issue with obtaining the path from the spawned
+   * @throws IOException if there is an issue with obtaining the path from the spawned
    *     process, either because there is no installed xcode with the given version, or
    *     there was an unexpected issue finding or running the tool
    */
-  public static String getDeveloperDir(Path execRoot, DottedVersion version, String productName)
-      throws UserExecException {
+  private static String getDeveloperDir(Path execRoot, DottedVersion version, String productName)
+      throws IOException {
+    if (OS.getCurrent() != OS.DARWIN) {
+      throw new IOException(
+          "Cannot locate xcode developer directory on non-darwin operating system");
+    }
     try {
       CacheManager cacheManager =
-          new CacheManager(execRoot.getRelative(BlazeDirectories.getRelativeOutputPath(productName)),
+          new CacheManager(
+              execRoot.getRelative(BlazeDirectories.getRelativeOutputPath(productName)),
               XCODE_LOCATOR_CACHE_FILENAME);
 
       String cacheResult = cacheManager.getValue(version.toString());
@@ -157,9 +207,9 @@ public static String getDeveloperDir(Path execRoot, DottedVersion version, Strin
             e.getResult().getTerminationStatus(),
             new String(e.getResult().getStderr(), StandardCharsets.UTF_8));
       }
-      throw new UserExecException(message, e);
-    } catch (CommandException | IOException e) {
-      throw new UserExecException(e);
+      throw new IOException(message, e);
+    } catch (CommandException e) {
+      throw new IOException(e);
     }
   }
 }

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

@@ -0,0 +1,36 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// 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.local;
+
+import com.google.devtools.build.lib.vfs.Path;
+import java.io.IOException;
+import java.util.Map;
+
+/**
+ * Allows just-in-time rewriting of the environment used for local actions. Do not use! This class
+ * probably should not exist, but is currently necessary for our local MacOS support.
+ */
+public interface LocalEnvProvider {
+  public static final LocalEnvProvider UNMODIFIED = new LocalEnvProvider() {
+    @Override
+    public Map<String, String> rewriteLocalEnv(
+        Map<String, String> env, Path execRoot, String productName) throws IOException {
+      return env;
+    }
+  };
+
+  /** Rewrites the environment if necessary. */
+  Map<String, String> rewriteLocalEnv(Map<String, String> env, Path execRoot, String productName)
+      throws IOException;
+}

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

@@ -34,7 +34,6 @@
 import com.google.devtools.build.lib.shell.CommandException;
 import com.google.devtools.build.lib.shell.CommandResult;
 import com.google.devtools.build.lib.util.NetUtil;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.Path;
@@ -75,14 +74,19 @@ public final class LocalSpawnRunner implements SpawnRunner {
   private final boolean useProcessWrapper;
   private final String processWrapper;
 
+  private final String productName;
+  private final LocalEnvProvider localEnvProvider;
+
   public LocalSpawnRunner(
       Logger logger,
       AtomicInteger execCount,
       Path execRoot,
       ActionInputPrefetcher actionInputPrefetcher,
       LocalExecutionOptions localExecutionOptions,
       ResourceManager resourceManager,
-      boolean useProcessWrapper) {
+      boolean useProcessWrapper,
+      String productName,
+      LocalEnvProvider localEnvProvider) {
     this.logger = logger;
     this.execRoot = execRoot;
     this.actionInputPrefetcher = Preconditions.checkNotNull(actionInputPrefetcher);
@@ -92,25 +96,8 @@ public LocalSpawnRunner(
     this.execCount = execCount;
     this.resourceManager = resourceManager;
     this.useProcessWrapper = useProcessWrapper;
-  }
-
-  public LocalSpawnRunner(
-      Path execRoot,
-      ActionInputPrefetcher actionInputPrefetcher,
-      LocalExecutionOptions localExecutionOptions,
-      ResourceManager resourceManager) {
-    this(
-        null,
-        new AtomicInteger(),
-        execRoot,
-        actionInputPrefetcher,
-        localExecutionOptions,
-        resourceManager,
-        // TODO(bazel-team): process-wrapper seems to work on Windows, but requires additional setup
-        // as it is an msys2 binary, so it needs msys2 DLLs on %PATH%. Disable it for now to make
-        // the setup easier and to avoid further PATH hacks. Ideally we should have a native
-        // implementation of process-wrapper for Windows.
-        OS.getCurrent() != OS.WINDOWS);
+    this.productName = productName;
+    this.localEnvProvider = localEnvProvider;
   }
 
   @Override
@@ -247,14 +234,14 @@ private SpawnResult start() throws InterruptedException, IOException {
         cmdLine.addAll(spawn.getArguments());
         cmd = new Command(
             cmdLine.toArray(new String[0]),
-            spawn.getEnvironment(),
+            localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, productName),
             execRoot.getPathFile());
       } else {
         stdOut = outErr.getOutputStream();
         stdErr = outErr.getErrorStream();
         cmd = new Command(
             spawn.getArguments().toArray(new String[0]),
-            spawn.getEnvironment(),
+            localEnvProvider.rewriteLocalEnv(spawn.getEnvironment(), execRoot, productName),
             execRoot.getPathFile(),
             policy.getTimeoutMillis());
       }

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

@@ -10,18 +10,16 @@ java_library(
     ],
     deps = [
         "//src/main/java/com/google/devtools/build/lib:build-base",
-        "//src/main/java/com/google/devtools/build/lib:concurrent",
         "//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:packages-internal",
-        "//src/main/java/com/google/devtools/build/lib:process_util",
         "//src/main/java/com/google/devtools/build/lib:runtime",
         "//src/main/java/com/google/devtools/build/lib:shell",
-        "//src/main/java/com/google/devtools/build/lib:unix",
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib:vfs",
         "//src/main/java/com/google/devtools/build/lib/actions",
-        "//src/main/java/com/google/devtools/build/lib/rules/cpp",
+        "//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/standalone",
         "//src/main/java/com/google/devtools/common/options",
         "//third_party:guava",