Make FileSystemUtils.moveFile always preserve symlinks and use it in …

…SandboxedSpawn.copyOutputs.

Previously, if moveFile fell back to copying from a true rename (e.g., if the
move is across file systems), it would fully dereference the source and produce
a regular file at the output location. This CL fixes moveFile to properly copy
symlinks. Note we don't preserve metadata in the symlink case mostly because the
Bazel VFS has no API to write metadata without dereferencing symlinks. (But,
also, it's not important for the current use cases.)

This breaks the backward-compatibility of FileSystemUtils.moveFile and
FileSystemUtils.moveTreeBelow. This seems okay because the new behavior makes
more sense, and sandbox is the only consumer of these APIs.

Switching SandboxedSpawn.copyOutputs to use FileSystemUtils.moveFile rather than
Guava's Files.move fixes bazelbuild#4987.

Change-Id: I0283e8aa11fadff5b0afd7bdfd0490aca12a1f6b

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

@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.sandbox;
 
-import com.google.common.io.Files;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -81,7 +80,7 @@ static void moveOutputs(Collection<PathFragment> outputs, Path sourceRoot, Path
         // have already been created, but the spawn outputs may be different from the overall action
         // outputs. This is the case for test actions.
         target.getParentDirectory().createDirectoryAndParents();
-        Files.move(source.getPathFile(), target.getPathFile());
+        FileSystemUtils.moveFile(source, target);
       } else if (source.isDirectory()) {
         try {
           source.renameTo(target);

src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java

@@ -409,38 +409,42 @@ public static void copyFile(Path from, Path to) throws IOException {
   }
 
   /**
-   * Moves the file from location "from" to location "to", while overwriting a
-   * potentially existing "to". File's last modified time, executable and
-   * writable bits are also preserved.
+   * Moves the file from location "from" to location "to", while overwriting a potentially existing
+   * "to". If "from" is a regular file, its last modified time, executable and writable bits are
+   * also preserved. Symlinks are also supported but not directories or special files.
    *
-   * <p>If no error occurs, the method returns normally. If a parent directory does
-   * not exist, a FileNotFoundException is thrown. An IOException is thrown when
-   * other erroneous situations occur. (e.g. read errors)
+   * <p>If no error occurs, the method returns normally. If a parent directory does not exist, a
+   * FileNotFoundException is thrown. {@link IOException} is thrown when other erroneous situations
+   * occur. (e.g. read errors)
    */
   @ThreadSafe  // but not atomic
   public static void moveFile(Path from, Path to) throws IOException {
-    long mtime = from.getLastModifiedTime();
-    boolean writable = from.isWritable();
-    boolean executable = from.isExecutable();
-
     // We don't try-catch here for better performance.
     to.delete();
     try {
       from.renameTo(to);
     } catch (IOException e) {
-      asByteSource(from).copyTo(asByteSink(to));
+      // Fallback to a copy.
+      FileStatus stat = from.stat(Symlinks.NOFOLLOW);
+      if (stat.isFile()) {
+        asByteSource(from).copyTo(asByteSink(to));
+        to.setLastModifiedTime(stat.getLastModifiedTime()); // Preserve mtime.
+        if (!from.isWritable()) {
+          to.setWritable(false); // Make file read-only if original was read-only.
+        }
+        to.setExecutable(from.isExecutable()); // Copy executable bit.
+      } else if (stat.isSymbolicLink()) {
+        to.createSymbolicLink(from.readSymbolicLink());
+      } else {
+        throw new IOException("Don't know how to copy " + from);
+      }
       if (!from.delete()) {
         if (!to.delete()) {
           throw new IOException("Unable to delete " + to);
         }
         throw new IOException("Unable to delete " + from);
       }
     }
-    to.setLastModifiedTime(mtime); // Preserve mtime.
-    if (!writable) {
-      to.setWritable(false); // Make file read-only if original was read-only.
-    }
-    to.setExecutable(executable); // Copy executable bit.
   }
 
   /**

src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java

@@ -369,6 +369,40 @@ public void testMoveFile() throws IOException {
     assertThat(originalFile.exists()).isFalse();
   }
 
+  @Test
+  public void testMoveFileAcrossDevices() throws Exception {
+    class MultipleDeviceFS extends InMemoryFileSystem {
+      @Override
+      public void renameTo(Path source, Path target) throws IOException {
+        if (!source.startsWith(target.asFragment().subFragment(0, 1))) {
+          throw new IOException("EXDEV");
+        }
+        super.renameTo(source, target);
+      }
+    }
+    FileSystem fs = new MultipleDeviceFS();
+    Path dev1 = fs.getPath("/fs1");
+    dev1.createDirectory();
+    Path dev2 = fs.getPath("/fs2");
+    dev2.createDirectory();
+    Path source = dev1.getChild("source");
+    Path target = dev2.getChild("target");
+
+    FileSystemUtils.writeContent(source, UTF_8, "hello, world");
+    source.setLastModifiedTime(142);
+    FileSystemUtils.moveFile(source, target);
+    assertThat(source.exists(Symlinks.NOFOLLOW)).isFalse();
+    assertThat(target.isFile(Symlinks.NOFOLLOW)).isTrue();
+    assertThat(FileSystemUtils.readContent(target, UTF_8)).isEqualTo("hello, world");
+    assertThat(target.getLastModifiedTime()).isEqualTo(142);
+
+    source.createSymbolicLink(PathFragment.create("link-target"));
+    FileSystemUtils.moveFile(source, target);
+    assertThat(source.exists(Symlinks.NOFOLLOW)).isFalse();
+    assertThat(target.isSymbolicLink()).isTrue();
+    assertThat(target.readSymbolicLink()).isEqualTo(PathFragment.create("link-target"));
+  }
+
   @Test
   public void testReadContentWithLimit() throws IOException {
     createTestDirectoryTree();