From 0481d12d0f61ea8b95da04382d3a74cb33049cba Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 23 Aug 2023 06:02:02 -0700 Subject: [PATCH] Canonicalize the parent path in readSymbolicLink/createSymbolicLink and use the ActionInputMap as a source of information for readSymbolicLink. Even though readSymbolicLink/createSymbolic link don't canonicalize the full path, they still need to canonicalize the parent path to be correct in the presence of cross-filesystem symlinks. Files in the ActionInputMap must participate in the resolveSymbolicLinks algorithm even when they're not present in the local filesystem. PiperOrigin-RevId: 559400472 Change-Id: Ia0354b57d0ef543e64301fdacadcb3b91363ea4d --- .../lib/remote/RemoteActionFileSystem.java | 28 ++++++++ .../remote/RemoteActionFileSystemTest.java | 66 +++++++++++++++++++ 2 files changed, 94 insertions(+) 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 142b18ef417241..1a963d6ccc5591 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 @@ -36,6 +36,7 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.UnresolvedSymlinkArtifactValue; import com.google.devtools.build.lib.actions.FileStatusWithMetadata; import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.cache.MetadataInjector; @@ -549,6 +550,27 @@ protected void chmod(PathFragment path, int mode) throws IOException { @Override protected PathFragment readSymbolicLink(PathFragment path) throws IOException { + PathFragment parentPath = path.getParentDirectory(); + if (parentPath != null) { + path = resolveSymbolicLinks(parentPath).asFragment().getChild(path.getBaseName()); + } + + if (path.startsWith(execRoot)) { + var execPath = path.relativeTo(execRoot); + var metadata = inputArtifactData.getMetadata(execPath); + if (metadata instanceof UnresolvedSymlinkArtifactValue) { + return PathFragment.create(((UnresolvedSymlinkArtifactValue) metadata).getSymlinkTarget()); + } + if (metadata != null) { + // Other input artifacts are never symlinks. + throw new NotASymlinkException(path); + } + if (inputTreeArtifactDirectoryCache.get(execPath) != null) { + // Tree artifacts never contain symlinks. + throw new NotASymlinkException(path); + } + } + if (isOutput(path)) { try { return remoteOutputTree.getPath(path).readSymbolicLink(); @@ -556,12 +578,18 @@ protected PathFragment readSymbolicLink(PathFragment path) throws IOException { // Intentionally ignored. } } + return localFs.getPath(path).readSymbolicLink(); } @Override protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment) throws IOException { + PathFragment parentPath = linkPath.getParentDirectory(); + if (parentPath != null) { + linkPath = resolveSymbolicLinks(parentPath).asFragment().getChild(linkPath.getBaseName()); + } + if (isOutput(linkPath)) { remoteOutputTree.getPath(linkPath).createSymbolicLink(targetFragment); } 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 d56e4fde962045..5574697dfbf3ba 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 @@ -681,6 +681,50 @@ public void readSymbolicLink_fromRemoteFilesystem() throws Exception { assertThrows(NotASymlinkException.class, () -> actionFs.readSymbolicLink(filePath)); } + @Test + public void readSymbolicLink_fromInputArtifactData_regularFile() throws Exception { + ActionInputMap inputs = new ActionInputMap(1); + Artifact artifact = createRemoteArtifact("file", "contents", inputs); + RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs); + + assertThrows( + NotASymlinkException.class, + () -> actionFs.readSymbolicLink(artifact.getPath().asFragment())); + } + + @Test + public void readSymbolicLink_fromInputArtifactData_treeSubDir() throws Exception { + ActionInputMap inputs = new ActionInputMap(1); + SpecialArtifact tree = + createRemoteTreeArtifact("tree", ImmutableMap.of("subdir/file", ""), inputs); + RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs); + + assertThrows( + NotASymlinkException.class, () -> actionFs.readSymbolicLink(tree.getPath().asFragment())); + + assertThrows( + NotASymlinkException.class, + () -> actionFs.readSymbolicLink(tree.getPath().getRelative("subdir").asFragment())); + } + + @Test + public void readSymbolicLink_fromInputArtifactData_unresolvedSymlink() throws Exception { + ActionInputMap inputs = new ActionInputMap(1); + RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs); + + Artifact symlink = ActionsTestUtil.createUnresolvedSymlinkArtifact(outputRoot, "symlink"); + PathFragment targetPath = PathFragment.create("/some/path"); + // Create symlink on the filesystem so we can digest it, then delete it to verify that its + // presence in the ActionInputMap is sufficient for readSymbolicLink to work. Note that this is + // an unrealistic scenario, as symlinks are always materialized even when produced remotely. + Path symlinkPath = getLocalFileSystem(actionFs).getPath(symlink.getPath().getPathString()); + symlinkPath.createSymbolicLink(targetPath); + inputs.putWithNoDepOwner(symlink, FileArtifactValue.createForUnresolvedSymlink(symlinkPath)); + symlinkPath.delete(); + + assertThat(actionFs.readSymbolicLink(getOutputPath("symlink"))).isEqualTo(targetPath); + } + @Test public void readSymbolicLink_notFound() throws Exception { RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(); @@ -857,6 +901,28 @@ public void createSymbolicLink_unresolvedSymlink() throws Exception { verifyNoInteractions(metadataInjector); } + @Test + public void createAndReadSymbolicLink_followSymlinks(@TestParameter FilesystemTestParam from) + throws Exception { + // createSymbolicLink writes to both the local and remote filesystem, so it makes no sense to + // parameterize on the symlink's destination filesystem. + RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(); + FileSystem fromFs = from.getFilesystem(actionFs); + + PathFragment parentLinkPath = getOutputPath("parent_link"); + PathFragment parentTargetPath = getOutputPath("parent_target"); + fromFs + .getPath(parentLinkPath) + .createSymbolicLink(execRoot.getRelative(parentTargetPath).asFragment()); + actionFs.getPath(parentTargetPath).createDirectoryAndParents(); + + PathFragment linkPath = getOutputPath("parent_target/link"); + PathFragment targetPath = PathFragment.create("/some/path"); + actionFs.getPath(linkPath).createSymbolicLink(targetPath); + + assertThat(actionFs.getPath(linkPath).readSymbolicLink()).isEqualTo(targetPath); + } + @Test public void resolveSymbolicLinks( @TestParameter FilesystemTestParam a, @TestParameter FilesystemTestParam b) throws Exception {