Make persistent action cache work on Windows

CompactPersistentActionCache didn't work because it trys to rename a file to an existing file which JavaIoFileSystem doesn't support. Solution is deleting the file before renaming.

--
Change-Id: I8d476cd2835c8cddbeed8f556361e0e35d1a2bd4
Reviewed-on: https://bazel-review.googlesource.com/#/c/3802
MOS_MIGRATED_REVID=124828957

2 files changed, 45 insertions, 38 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeWorkspace.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeWorkspace.java
index 6652bd74ab..bbc54d204d 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeWorkspace.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeWorkspace.java
@@ -21,7 +21,7 @@ import com.google.common.eventbus.EventBus;
 import com.google.common.eventbus.SubscriberExceptionHandler;
 import com.google.devtools.build.lib.actions.cache.ActionCache;
 import com.google.devtools.build.lib.actions.cache.CompactPersistentActionCache;
-import com.google.devtools.build.lib.actions.cache.NullActionCache;
+
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
 import com.google.devtools.build.lib.analysis.config.BinTools;
@@ -31,7 +31,6 @@ import com.google.devtools.build.lib.profiler.AutoProfiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
 import com.google.devtools.build.lib.util.LoggingUtil;
-import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -66,14 +65,17 @@ public final class BlazeWorkspace {
   /** The action cache is loaded lazily on the first build command. */
   private ActionCache actionCache;
   /** The execution time range of the previous build command in this server, if any. */
-  @Nullable
-  private Range<Long> lastExecutionRange = null;
+  @Nullable private Range<Long> lastExecutionRange = null;
 
   private final String outputBaseFilesystemTypeName;
 
-  public BlazeWorkspace(BlazeRuntime runtime, BlazeDirectories directories,
-      SkyframeExecutor skyframeExecutor, SubscriberExceptionHandler eventBusExceptionHandler,
-      WorkspaceStatusAction.Factory workspaceStatusActionFactory, BinTools binTools) {
+  public BlazeWorkspace(
+      BlazeRuntime runtime,
+      BlazeDirectories directories,
+      SkyframeExecutor skyframeExecutor,
+      SubscriberExceptionHandler eventBusExceptionHandler,
+      WorkspaceStatusAction.Factory workspaceStatusActionFactory,
+      BinTools binTools) {
     this.runtime = runtime;
     this.eventBusExceptionHandler = eventBusExceptionHandler;
     this.workspaceStatusActionFactory = workspaceStatusActionFactory;
@@ -171,9 +173,10 @@ public final class BlazeWorkspace {
 
   void recordLastExecutionTime(long commandStartTime) {
     long currentTimeMillis = runtime.getClock().currentTimeMillis();
-    lastExecutionRange = currentTimeMillis >= commandStartTime
-        ? Range.closed(commandStartTime, currentTimeMillis)
-        : null;
+    lastExecutionRange =
+        currentTimeMillis >= commandStartTime
+            ? Range.closed(commandStartTime, currentTimeMillis)
+            : null;
   }
 
   /**
@@ -210,22 +213,21 @@ public final class BlazeWorkspace {
    */
   public ActionCache getPersistentActionCache(Reporter reporter) throws IOException {
     if (actionCache == null) {
-      if (OS.getCurrent() == OS.WINDOWS) {
-        // TODO(bazel-team): Add support for a persistent action cache on Windows.
-        actionCache = new NullActionCache();
-        return actionCache;
-      }
       try (AutoProfiler p = profiledAndLogged("Loading action cache", ProfilerTask.INFO, LOG)) {
         try {
           actionCache = new CompactPersistentActionCache(getCacheDirectory(), runtime.getClock());
         } catch (IOException e) {
           LOG.log(Level.WARNING, "Failed to load action cache: " + e.getMessage(), e);
-          LoggingUtil.logToRemote(Level.WARNING, "Failed to load action cache: "
-              + e.getMessage(), e);
+          LoggingUtil.logToRemote(
+              Level.WARNING, "Failed to load action cache: " + e.getMessage(), e);
           reporter.handle(
-              Event.error("Error during action cache initialization: " + e.getMessage()
-              + ". Corrupted files were renamed to '" + getCacheDirectory() + "/*.bad'. "
-              + "Blaze will now reset action cache data, causing a full rebuild"));
+              Event.error(
+                  "Error during action cache initialization: "
+                      + e.getMessage()
+                      + ". Corrupted files were renamed to '"
+                      + getCacheDirectory()
+                      + "/*.bad'. "
+                      + "Blaze will now reset action cache data, causing a full rebuild"));
           actionCache = new CompactPersistentActionCache(getCacheDirectory(), runtime.getClock());
         }
       }
@@ -242,14 +244,19 @@ public final class BlazeWorkspace {
     Preconditions.checkNotNull(getWorkspace());
     Path outputBaseReadmeFile = getOutputBase().getRelative("README");
     try {
-      FileSystemUtils.writeIsoLatin1(outputBaseReadmeFile, "WORKSPACE: " + getWorkspace(), "",
+      FileSystemUtils.writeIsoLatin1(
+          outputBaseReadmeFile,
+          "WORKSPACE: " + getWorkspace(),
+          "",
           "The first line of this file is intentionally easy to parse for various",
           "interactive scripting and debugging purposes.  But please DO NOT write programs",
           "that exploit it, as they will be broken by design: it is not possible to",
           "reverse engineer the set of source trees or the --package_path from the output",
           "tree, and if you attempt it, you will fail, creating subtle and",
           "hard-to-diagnose bugs, that will no doubt get blamed on changes made by the",
-          "Blaze team.", "", "This directory was generated by Blaze.",
+          "Blaze team.",
+          "",
+          "This directory was generated by Blaze.",
           "Do not attempt to modify or delete any files in this directory.",
           "Among other issues, Blaze's file system caching assumes that",
           "only Blaze will modify this directory and the files in it,",
@@ -272,8 +279,8 @@ public final class BlazeWorkspace {
     Preconditions.checkNotNull(getWorkspace());
     writeDoNotBuildHereFile(getOutputBase().getRelative(DO_NOT_BUILD_FILE_NAME));
     if (startupOptions.getOptions(BlazeServerStartupOptions.class).deepExecRoot) {
-      writeDoNotBuildHereFile(getOutputBase().getRelative("execroot").getRelative(
-          DO_NOT_BUILD_FILE_NAME));
+      writeDoNotBuildHereFile(
+          getOutputBase().getRelative("execroot").getRelative(DO_NOT_BUILD_FILE_NAME));
     }
   }
 
@@ -284,8 +291,8 @@ public final class BlazeWorkspace {
     try {
       FileSystemUtils.createDirectoryAndParents(directories.getExecRoot());
     } catch (IOException e) {
-      LOG.warning("failed to create execution root '" + directories.getExecRoot() + "': "
-          + e.getMessage());
+      LOG.warning(
+          "failed to create execution root '" + directories.getExecRoot() + "': " + e.getMessage());
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/util/PersistentMap.java b/src/main/java/com/google/devtools/build/lib/util/PersistentMap.java
index 2835c5cce0..56ac9a1b2b 100644
--- a/src/main/java/com/google/devtools/build/lib/util/PersistentMap.java
+++ b/src/main/java/com/google/devtools/build/lib/util/PersistentMap.java
@@ -114,7 +114,9 @@ public abstract class PersistentMap<K, V> extends ForwardingMap<K, V> {
     delegate = map;
   }
 
-  @Override protected Map<K, V> delegate() {
+
+  @Override
+  protected Map<K, V> delegate() {
     return delegate;
   }
 
@@ -222,7 +224,7 @@ public abstract class PersistentMap<K, V> extends ForwardingMap<K, V> {
         }
         // Force the map to be dirty, so that we can save it to disk.
         dirty = true;
-        save(/*fullSave=*/true);
+        save(/*fullSave=*/ true);
       } else {
         dirty = false;
       }
@@ -236,7 +238,7 @@ public abstract class PersistentMap<K, V> extends ForwardingMap<K, V> {
    * @throws IOException
    */
   public void load() throws IOException {
-    load(/*throwOnLoadFailure=*/false);
+    load(/*throwOnLoadFailure=*/ false);
   }
 
   @Override
@@ -289,6 +291,7 @@ public abstract class PersistentMap<K, V> extends ForwardingMap<K, V> {
             mapFile.getRelative(FileSystemUtils.replaceExtension(mapFile.asFragment(), ".tmp"));
         try {
           saveEntries(delegate(), mapTemp);
+          mapFile.delete();
           mapTemp.renameTo(mapFile);
         } finally {
           mapTemp.delete();
@@ -331,8 +334,7 @@ public abstract class PersistentMap<K, V> extends ForwardingMap<K, V> {
     if (!mapFile.exists()) {
       return;
     }
-    DataInputStream in =
-      new DataInputStream(new BufferedInputStream(mapFile.getInputStream()));
+    DataInputStream in = new DataInputStream(new BufferedInputStream(mapFile.getInputStream()));
     try {
       long fileSize = mapFile.getFileSize();
       if (fileSize < (16)) {
@@ -383,7 +385,7 @@ public abstract class PersistentMap<K, V> extends ForwardingMap<K, V> {
   private DataOutputStream createMapFile(Path mapFile) throws IOException {
     FileSystemUtils.createDirectoryAndParents(mapFile.getParentDirectory());
     DataOutputStream out =
-      new DataOutputStream(new BufferedOutputStream(mapFile.getOutputStream()));
+        new DataOutputStream(new BufferedOutputStream(mapFile.getOutputStream()));
     out.writeLong(MAGIC);
     out.writeLong(version);
     return out;
@@ -397,8 +399,7 @@ public abstract class PersistentMap<K, V> extends ForwardingMap<K, V> {
    *        DataOutputStream.
    * @throws IOException
    */
-  private void writeEntries(DataOutputStream out, Map<K, V> map)
-      throws IOException {
+  private void writeEntries(DataOutputStream out, Map<K, V> map) throws IOException {
     for (Map.Entry<K, V> entry : map.entrySet()) {
       out.writeByte(ENTRY_MAGIC);
       writeKey(entry.getKey(), out);
@@ -453,8 +454,7 @@ public abstract class PersistentMap<K, V> extends ForwardingMap<K, V> {
    * @param out the DataOutputStream to write the entry to.
    * @throws IOException
    */
-  protected abstract void writeKey(K key, DataOutputStream out)
-      throws IOException;
+  protected abstract void writeKey(K key, DataOutputStream out) throws IOException;
 
   /**
    * Writes a value of this map into the specified DataOutputStream.
@@ -463,8 +463,7 @@ public abstract class PersistentMap<K, V> extends ForwardingMap<K, V> {
    * @param out the DataOutputStream to write the entry to.
    * @throws IOException
    */
-  protected abstract void writeValue(V value, DataOutputStream out)
-      throws IOException;
+  protected abstract void writeValue(V value, DataOutputStream out) throws IOException;
 
   /**
    * Reads an entry of this map from the specified DataInputStream.
@@ -484,3 +483,4 @@ public abstract class PersistentMap<K, V> extends ForwardingMap<K, V> {
    */
   protected abstract V readValue(DataInputStream in) throws IOException;
 }
+