From d3a7b6afeeb7c4d4ca1432938191fbfe45cb62ba Mon Sep 17 00:00:00 2001 From: Benjamin Peterson Date: Mon, 9 Apr 2018 18:37:10 -0700 Subject: [PATCH] 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 https://github.com/bazelbuild/bazel/issues/4987. Change-Id: I0283e8aa11fadff5b0afd7bdfd0490aca12a1f6b --- .../build/lib/sandbox/SandboxedSpawn.java | 3 +- .../build/lib/vfs/FileSystemUtils.java | 36 ++++++++++--------- .../build/lib/vfs/FileSystemUtilsTest.java | 34 ++++++++++++++++++ 3 files changed, 55 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java index e6408dc3ccecdc..1600190a1129a1 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxedSpawn.java +++ b/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 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); diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java index b7fd8d2a431520..c87ba7f6ee56c0 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java @@ -409,26 +409,35 @@ 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. * - *

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) + *

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); @@ -436,11 +445,6 @@ public static void moveFile(Path from, Path to) throws IOException { 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. } /** diff --git a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java index f9cc1a9031472c..cd6be682a255cb 100644 --- a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java +++ b/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();