From f64e6fa7258fb5012ef615b34fabd19a6237f808 Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Mon, 12 Feb 2024 19:21:30 +0100 Subject: [PATCH] [7.1.0] Canonicalize the parent path in RemoteActionFileSystem#renameTo. (#21285) FileSystem#renameTo is documented as not following symlinks, but that refers to the last component only; we are still required to canonicalize the parent path, possibly taking into account symlinks that straddle underlying sources. PiperOrigin-RevId: 605606357 Change-Id: I6508c07413ceccc9947e0a03cce93d769b31f2b9 --- .../lib/remote/RemoteActionFileSystem.java | 13 ++-- .../remote/RemoteActionFileSystemTest.java | 59 +++++++++++++++++++ .../RemoteActionFileSystemTestBase.java | 12 +++- 3 files changed, 78 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index 449faadcacb9fb..03e7f0827dd3dd 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java @@ -734,20 +734,23 @@ private boolean isOutput(PathFragment path) { } @Override - public void renameTo(PathFragment sourcePath, PathFragment targetPath) throws IOException { - checkArgument(isOutput(sourcePath), "sourcePath must be an output path"); - checkArgument(isOutput(targetPath), "targetPath must be an output path"); + public void renameTo(PathFragment srcPath, PathFragment dstPath) throws IOException { + srcPath = resolveSymbolicLinksForParent(srcPath); + dstPath = resolveSymbolicLinksForParent(dstPath); + + checkArgument(isOutput(srcPath), "srcPath must be an output path"); + checkArgument(isOutput(dstPath), "dstPath must be an output path"); FileNotFoundException remoteException = null; try { - remoteOutputTree.renameTo(sourcePath, targetPath); + remoteOutputTree.renameTo(srcPath, dstPath); } catch (FileNotFoundException e) { remoteException = e; } FileNotFoundException localException = null; try { - localFs.renameTo(sourcePath, targetPath); + localFs.renameTo(srcPath, dstPath); } catch (FileNotFoundException e) { localException = e; } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java index d123c1d5062dcb..8085501d3417c7 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java @@ -1167,6 +1167,65 @@ public void renameTo_onlyLocalFile_renameLocalFile() throws Exception { assertThat(getLocalFileSystem(actionFs).exists(newPath)).isTrue(); } + @Test + public void renameTo_moveSymlink() throws Exception { + FileSystem actionFs = createActionFileSystem(); + PathFragment oldLinkPath = getOutputPath("oldLink"); + PathFragment newLinkPath = getOutputPath("newLink"); + PathFragment targetPath = getOutputPath("target"); + actionFs.getPath(oldLinkPath).createSymbolicLink(execRoot.getRelative(targetPath).asFragment()); + writeLocalFile(actionFs, targetPath, "content"); + + actionFs.renameTo(oldLinkPath, newLinkPath); + + assertThat(actionFs.getPath(oldLinkPath).exists(Symlinks.NOFOLLOW)).isFalse(); + assertThat(actionFs.getPath(newLinkPath).exists(Symlinks.NOFOLLOW)).isTrue(); + assertThat(actionFs.getPath(newLinkPath).readSymbolicLink()).isEqualTo(targetPath); + assertThat(actionFs.getPath(targetPath).exists()).isTrue(); + } + + @Test + public void renameTo_followSymlinks( + @TestParameter FilesystemTestParam from, @TestParameter FilesystemTestParam to) + throws Exception { + RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(); + FileSystem fromFs = from.getFilesystem(actionFs); + FileSystem toFs = to.getFilesystem(actionFs); + + PathFragment srcDirLinkPath = getOutputPath("srcDirLink"); + PathFragment srcDirTargetPath = getOutputPath("srcDirTarget"); + PathFragment naiveSrcPath = srcDirLinkPath.getChild("oldFile"); + PathFragment canonicalSrcPath = srcDirTargetPath.getChild("oldFile"); + + PathFragment dstDirLinkPath = getOutputPath("dstDirLink"); + PathFragment dstDirTargetPath = getOutputPath("dstDirTarget"); + PathFragment naiveDstPath = dstDirLinkPath.getChild("newFile"); + PathFragment canonicalDstPath = dstDirTargetPath.getChild("newFile"); + + actionFs.getPath(srcDirTargetPath).createDirectoryAndParents(); + actionFs.getPath(dstDirTargetPath).createDirectoryAndParents(); + + fromFs + .getPath(srcDirLinkPath) + .createSymbolicLink(execRoot.getRelative(srcDirTargetPath).asFragment()); + fromFs + .getPath(dstDirLinkPath) + .createSymbolicLink(execRoot.getRelative(dstDirTargetPath).asFragment()); + + if (toFs.equals(actionFs.getLocalFileSystem())) { + writeLocalFile(actionFs, canonicalSrcPath, "content"); + } else { + injectRemoteFile(actionFs, canonicalSrcPath, "content"); + } + + actionFs.renameTo(naiveSrcPath, naiveDstPath); + + assertThat(actionFs.exists(naiveSrcPath)).isFalse(); + assertThat(actionFs.exists(canonicalSrcPath)).isFalse(); + assertThat(actionFs.exists(naiveDstPath)).isTrue(); + assertThat(actionFs.exists(canonicalDstPath)).isTrue(); + } + @Override @CanIgnoreReturnValue protected FileArtifactValue injectRemoteFile( diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java index 6fce32f5a0ec1d..77382e84f47fd0 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTestBase.java @@ -161,7 +161,7 @@ public void delete_localAndRemoteFile_succeeds() throws Exception { } @Test - public void renameTo_fileDoesNotExist_throwError() throws Exception { + public void renameTo_sourceFileDoesNotExist_throwError() throws Exception { FileSystem actionFs = createActionFileSystem(); PathFragment path = getOutputPath("file"); PathFragment newPath = getOutputPath("file-new"); @@ -169,6 +169,16 @@ public void renameTo_fileDoesNotExist_throwError() throws Exception { assertThrows(FileNotFoundException.class, () -> actionFs.renameTo(path, newPath)); } + @Test + public void renameTo_targetDirectoryDoesNotExist_throwError() throws Exception { + FileSystem actionFs = createActionFileSystem(); + PathFragment path = getOutputPath("file"); + PathFragment newPath = getOutputPath("dir/file-new"); + writeLocalFile(actionFs, path, "local-content"); + + assertThrows(FileNotFoundException.class, () -> actionFs.renameTo(path, newPath)); + } + @Test public void renameTo_onlyRemoteFile_renameRemoteFile() throws Exception { FileSystem actionFs = createActionFileSystem();