Remove the on-disk caching of Xcode locations.

There is no need for the cache to be on disk.  Originally, there was a
desire to share this cache with other tools... but this never happened.
And, actually, because Bazel is in control of what it runs, it can just
inject the "cached" values into those tools via flags.

Instead, just store the cache in-memory.  This avoids having to open and
read the cache on every single action that is locally executed on a Mac.
Results when building a large iOS app from a clean slate show up to a
1% wall time improvement on my Mac Pro 2013 and a reduction in the
variance of the measurements.

This change also gets rid of the OS check from the action execution's
critical path.  There is not much use in checking this: if we instantiate
this by mistake, the actual calls will fail.  But sometimes we want to
actually run this code on non-macOS systems (e.g. for unit-testing with
mocked tools), so we should allow that.  And this change also ensures
that XcodeLocalEnvProviderTest builds and runs...

RELNOTES: None.
PiperOrigin-RevId: 194681802

2 files changed, 116 insertions, 191 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/exec/apple/CacheManager.java b/src/main/java/com/google/devtools/build/lib/exec/apple/CacheManager.java
deleted file mode 100644
index 0c7e52a328..0000000000
--- a/src/main/java/com/google/devtools/build/lib/exec/apple/CacheManager.java
+++ /dev/null
@@ -1,108 +0,0 @@
-// Copyright 2016 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.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;
-
-/**
- * General cache file manager for mapping one or more keys to host-related path information.
- *
- * <p> Cache management has some notable restrictions:
- * <ul>
- * <li>Each cache entry must have the same number of (string) keys, and one value.</li>
- * <li>An entry, once written to the cache, must be stable between builds. Clearing the cache
- *     requires a full clean of the bazel output directory.</li>
- * </ul>
- *
- * <p> Note that a single cache manager instance is not thread-safe, though multiple threads may
- * hold cache manager instances for the same cache file. As a result, it is possible multiple
- * threads may write the same entry to cache. This is fine, as retrieval from the cache will simply
- * return the first found entry.
- */
-final class CacheManager {
-
-  private final Path cacheFilePath;
-  private boolean cacheFileTouched;
-
-  /**
-   * @param outputRoot path to the bazel's output root
-   * @param cacheFilename name of the cache file
-   */
-  CacheManager(Path outputRoot, String cacheFilename) {
-    cacheFilePath = outputRoot.getRelative(cacheFilename);
-  }
-  
-  private void touchCacheFile() throws IOException {
-    if (!cacheFileTouched) {
-      FileSystemUtils.touchFile(cacheFilePath);
-      cacheFileTouched = true;
-    }
-  }
-  
-  /**
-   * Returns the value associated with the given list of string keys from the cache,
-   * or null if the entry is not present in the cache. If there is more than one value for the
-   * given key, the first value is returned.
-   */
-  @Nullable
-  public String getValue(String... keys) throws IOException {
-    Preconditions.checkArgument(keys.length > 0);
-    touchCacheFile();
-
-    List<String> keyList = ImmutableList.copyOf(keys);
-    Iterable<String> cacheContents =
-        FileSystemUtils.readLines(cacheFilePath, StandardCharsets.UTF_8);
-    for (String cacheLine : cacheContents) {
-      if (cacheLine.isEmpty()) {
-        continue;
-      }
-      List<String> cacheEntry = Splitter.on(':').splitToList(cacheLine);
-      List<String> cacheKeys = cacheEntry.subList(0, cacheEntry.size() - 1);
-      String cacheValue = cacheEntry.get(cacheEntry.size() - 1);
-      if (keyList.size() != cacheKeys.size()) {
-        throw new IllegalStateException(
-            String.format("cache file %s is malformed. Expected %s keys. line: '%s'",
-                cacheFilePath, keyList.size(), cacheLine));
-      }
-      if (keyList.equals(cacheKeys)) {
-        return cacheValue;
-      }
-    }
-    return null;
-  }
-
-  /**
-   * Write an entry to the cache. An entry consists of one or more keys and a single value.
-   * No validation is made regarding whether there are redundant or conflicting entries in the
-   * cache; it is thus the responsibility of the caller to ensure that any redundant entries
-   * (entries which have the same keys) also have the same value.
-   */
-  public void writeEntry(List<String> keys, String value) throws IOException {
-    Preconditions.checkArgument(!keys.isEmpty());
-
-    touchCacheFile();
-    FileSystemUtils.appendLinesAs(cacheFilePath, StandardCharsets.UTF_8,
-        Joiner.on(":").join(ImmutableList.builder().addAll(keys).add(value).build()));
-  }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProvider.java
index 55a2ba5ddc..a6baec70bb 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/apple/XcodeLocalEnvProvider.java
@@ -14,10 +14,8 @@
 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.common.collect.Maps;
-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;
@@ -26,11 +24,14 @@ 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.io.UncheckedIOException;
 import java.nio.charset.StandardCharsets;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.logging.Logger;
 
 /**
  * Adds to the given environment all variables that are dependent on system state of the host
@@ -44,19 +45,20 @@ import java.util.Map;
  * configuration.
  */
 public final class XcodeLocalEnvProvider implements LocalEnvProvider {
-  private static final String XCRUN_CACHE_FILENAME = "__xcruncache";
-  private static final String XCODE_LOCATOR_CACHE_FILENAME = "__xcodelocatorcache";
 
-  private final String productName;
+  private static final Logger log = Logger.getLogger(XcodeLocalEnvProvider.class.getName());
+
   private final Map<String, String> clientEnv;
 
+  private static final ConcurrentMap<String, String> sdkRootCache = new ConcurrentHashMap<>();
+  private static final ConcurrentMap<String, String> developerDirCache = new ConcurrentHashMap<>();
+
   /**
    * Creates a new {@link XcodeLocalEnvProvider}.
    *
    * @param clientEnv a map of the current Bazel command's environment
    */
-  public XcodeLocalEnvProvider(String productName, Map<String, String> clientEnv) {
-    this.productName = productName;
+  public XcodeLocalEnvProvider(Map<String, String> clientEnv) {
     this.clientEnv = clientEnv;
   }
 
@@ -89,7 +91,7 @@ public final class XcodeLocalEnvProvider implements LocalEnvProvider {
     String developerDir = "";
     if (containsXcodeVersion) {
       String version = env.get(AppleConfiguration.XCODE_VERSION_ENV_NAME);
-      developerDir = getDeveloperDir(execRoot, DottedVersion.fromString(version), productName);
+      developerDir = getDeveloperDir(execRoot, DottedVersion.fromString(version));
       newEnvBuilder.put("DEVELOPER_DIR", developerDir);
     }
     if (containsAppleSdkVersion) {
@@ -99,67 +101,46 @@ public final class XcodeLocalEnvProvider implements LocalEnvProvider {
       }
       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));
+      newEnvBuilder.put("SDKROOT", getSdkRoot(developerDir, iosSdkVersion, appleSdkPlatform));
     }
 
     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 process and use the
-   * {@code /usr/bin/xcrun} binary to locate the target SDK. This uses a local cache file under
-   * {@code bazel-out}, and will only spawn a new {@code xcrun} process in the case of a cache miss.
+   * Queries the path to the target Apple SDK on the host system for a given version of Xcode.
+   *
+   * <p>This spawns a subprocess to run the {@code /usr/bin/xcrun} binary to locate the target SDK.
+   * As this is a costly operation, always call {@link #getSdkRoot(String, String, String)} instead,
+   * which does caching.
    *
-   * @param execRoot the execution root path, used to locate the cache file
    * @param developerDir the value of {@code DEVELOPER_DIR} for the target version of xcode
-   * @param sdkVersion the sdk version, for example, "9.1"
-   * @param appleSdkPlatform the sdk platform, for example, "iPhoneOS"
-   * @param productName the product name
+   * @param sdkVersion the sdk version; for example, {@code 9.1}
+   * @param appleSdkPlatform the sdk platform; for example, {@code iPhoneOS}
+   * @return an absolute path to the root of the target Apple SDK
    * @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
    */
-  private static String getSdkRoot(
-      Path execRoot,
+  private static String querySdkRoot(
       String developerDir,
       String sdkVersion,
-      String appleSdkPlatform,
-      String productName)
+      String appleSdkPlatform)
       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(BlazeDirectories.getRelativeOutputPath(productName)),
-              XCRUN_CACHE_FILENAME);
-
       String sdkString = appleSdkPlatform.toLowerCase() + sdkVersion;
-      String cacheResult = cacheManager.getValue(developerDir, sdkString);
-      if (cacheResult != null) {
-        return cacheResult;
-      } else {
-        Map<String, String> env =
-            Strings.isNullOrEmpty(developerDir)
-                ? ImmutableMap.<String, String>of()
-                : ImmutableMap.of("DEVELOPER_DIR", developerDir);
-        CommandResult xcrunResult =
-            new Command(
-                    new String[] {"/usr/bin/xcrun", "--sdk", sdkString, "--show-sdk-path"},
-                    env,
-                    null)
-                .execute();
-
-        // calling xcrun via Command returns a value with a newline on the end.
-        String sdkRoot = new String(xcrunResult.getStdout(), StandardCharsets.UTF_8).trim();
+      Map<String, String> env =
+          Strings.isNullOrEmpty(developerDir)
+              ? ImmutableMap.<String, String>of()
+              : ImmutableMap.of("DEVELOPER_DIR", developerDir);
+      CommandResult xcrunResult =
+          new Command(
+              new String[] {"/usr/bin/xcrun", "--sdk", sdkString, "--show-sdk-path"},
+              env,
+              null)
+              .execute();
 
-        cacheManager.writeEntry(ImmutableList.of(developerDir, sdkString), sdkRoot);
-        return sdkRoot;
-      }
+      return new String(xcrunResult.getStdout(), StandardCharsets.UTF_8).trim();
     } catch (AbnormalTerminationException e) {
       TerminationStatus terminationStatus = e.getResult().getTerminationStatus();
 
@@ -189,47 +170,65 @@ public final class XcodeLocalEnvProvider implements LocalEnvProvider {
   }
 
   /**
-   * Returns the absolute root path of the xcode developer directory on the host system for the
-   * given xcode version. This may spawn a process and use the {@code xcode-locator} binary. This
-   * uses a local cache file under {@code bazel-out}, and will only spawn a new process in the case
-   * of a cache miss.
+   * Returns the path to the target Apple SDK on the host system for a given version of Xcode.
+   *
+   * <p>This may delegate to {@link #querySdkRoot(String, String, String)} to obtain the path from
+   * external sources in the system. Values are cached in-memory throughout the lifetime of the
+   * Bazel server.
+   *
+   * @param developerDir the value of {@code DEVELOPER_DIR} for the target version of xcode
+   * @param sdkVersion the sdk version; for example, {@code 9.1}
+   * @param appleSdkPlatform the sdk platform; for example, {@code iPhoneOS}
+   * @return an absolute path to the root of the target Apple SDK
+   * @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
+   */
+  private static String getSdkRoot(String developerDir, String sdkVersion, String appleSdkPlatform)
+      throws IOException {
+    try {
+      return sdkRootCache.computeIfAbsent(
+          developerDir + ":" + appleSdkPlatform.toLowerCase() + ":" + sdkVersion,
+          (key) -> {
+            try {
+              String sdkRoot = querySdkRoot(developerDir, sdkVersion, appleSdkPlatform);
+              log.info("Queried Xcode SDK root with key " + key + " and got " + sdkRoot);
+              return sdkRoot;
+            } catch (IOException e) {
+              throw new UncheckedIOException(e);
+            }
+          });
+    } catch (UncheckedIOException e) {
+      throw e.getCause();
+    }
+  }
+
+  /**
+   * Queries the path to the Xcode developer directory on the host system for the given Xcode
+   * version.
+   *
+   * <p>This spawns a subprocess to run the {@code xcode-locator} binary. As this is a costly
+   * operation, always call {@link #getDeveloperDir(Path, DottedVersion)} instead, which does
+   * caching.
    *
    * @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
+   * @return an absolute path to the root of the Xcode developer directory
    * @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
    */
-  private static String getDeveloperDir(Path execRoot, DottedVersion version, String productName)
+  private static String queryDeveloperDir(Path execRoot, DottedVersion version)
       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)),
-              XCODE_LOCATOR_CACHE_FILENAME);
-
-      String cacheResult = cacheManager.getValue(version.toString());
-      if (cacheResult != null) {
-        return cacheResult;
-      } else {
-        CommandResult xcodeLocatorResult =
-            new Command(
-                    new String[] {
-                      execRoot.getRelative("_bin/xcode-locator").getPathString(), version.toString()
-                    })
-                .execute();
-
-        String developerDir =
-            new String(xcodeLocatorResult.getStdout(), StandardCharsets.UTF_8).trim();
+      CommandResult xcodeLocatorResult =
+          new Command(
+                  new String[] {
+                    execRoot.getRelative("_bin/xcode-locator").getPathString(), version.toString()
+                  })
+              .execute();
 
-        cacheManager.writeEntry(ImmutableList.of(version.toString()), developerDir);
-        return developerDir;
-      }
+      return new String(xcodeLocatorResult.getStdout(), StandardCharsets.UTF_8).trim();
     } catch (AbnormalTerminationException e) {
       TerminationStatus terminationStatus = e.getResult().getTerminationStatus();
 
@@ -258,4 +257,38 @@ public final class XcodeLocalEnvProvider implements LocalEnvProvider {
       throw new IOException(e);
     }
   }
+
+  /**
+   * Returns the absolute root path of the xcode developer directory on the host system for the
+   * given Xcode version.
+   *
+   * <p>This may delegate to {@link #queryDeveloperDir(Path, DottedVersion)} to obtain the path from
+   * external sources in the system. Values are cached in-memory throughout the lifetime of the
+   * Bazel server.
+   *
+   * @param execRoot the execution root path, used to locate the cache file
+   * @param version the xcode version number to look up
+   * @return an absolute path to the root of the Xcode developer directory
+   * @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
+   */
+  private static String getDeveloperDir(Path execRoot, DottedVersion version)
+      throws IOException {
+    try {
+      return developerDirCache.computeIfAbsent(
+          version.toString(),
+          (key) -> {
+            try {
+              String developerDir = queryDeveloperDir(execRoot, version);
+              log.info("Queried Xcode developer dir with key " + key + " and got " + developerDir);
+              return developerDir;
+            } catch (IOException e) {
+              throw new UncheckedIOException(e);
+            }
+          });
+    } catch (UncheckedIOException e) {
+      throw e.getCause();
+    }
+  }
 }