diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index c3cae384b5e7ee..e258594981f804 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -210,7 +210,7 @@ public RemoteExecutionService( this.tempPathGenerator = tempPathGenerator; this.captureCorruptedOutputsDir = captureCorruptedOutputsDir; - this.scheduler = Schedulers.from(executor, /*interruptibleWorker=*/ true); + this.scheduler = Schedulers.from(executor, /* interruptibleWorker= */ true); } static Command buildCommand( @@ -639,6 +639,10 @@ public List getOutputDirectorySymlinks() { return actionResult.getOutputDirectorySymlinksList(); } + public List getOutputSymlinks() { + return actionResult.getOutputSymlinksList(); + } + /** * Returns the freeform informational message with details on the execution of the action that * may be displayed to the user upon failure or when requested explicitly. @@ -1005,7 +1009,7 @@ ActionResultMetadata parseActionResultMetadata( directExecutor())); } - waitForBulkTransfer(dirMetadataDownloads.values(), /* cancelRemainingOnInterrupt=*/ true); + waitForBulkTransfer(dirMetadataDownloads.values(), /* cancelRemainingOnInterrupt= */ true); ImmutableMap.Builder directories = ImmutableMap.builder(); for (Map.Entry> metadataDownload : @@ -1029,18 +1033,33 @@ ActionResultMetadata parseActionResultMetadata( new FileMetadata(localPath, outputFile.getDigest(), outputFile.getIsExecutable())); } - ImmutableMap.Builder symlinks = ImmutableMap.builder(); - Iterable outputSymlinks = - Iterables.concat(result.getOutputFileSymlinks(), result.getOutputDirectorySymlinks()); - for (OutputSymlink symlink : outputSymlinks) { - Path localPath = + var symlinkMap = new HashMap(); + var outputSymlinks = + Iterables.concat( + result.getOutputFileSymlinks(), + result.getOutputDirectorySymlinks(), + result.getOutputSymlinks()); + for (var symlink : outputSymlinks) { + var localPath = remotePathResolver.outputPathToLocalPath(encodeBytestringUtf8(symlink.getPath())); - symlinks.put( - localPath, new SymlinkMetadata(localPath, PathFragment.create(symlink.getTarget()))); + var target = PathFragment.create(symlink.getTarget()); + var existingMetadata = symlinkMap.get(localPath); + if (existingMetadata != null) { + if (!target.equals(existingMetadata.target())) { + throw new IOException( + String.format( + "Symlink path collision: '%s' is mapped to both '%s' and '%s'. Action Result" + + " should not contain multiple targets for the same symlink.", + localPath, existingMetadata.target(), target)); + } + continue; + } + + symlinkMap.put(localPath, new SymlinkMetadata(localPath, target)); } return new ActionResultMetadata( - files.buildOrThrow(), symlinks.buildOrThrow(), directories.buildOrThrow()); + files.buildOrThrow(), ImmutableMap.copyOf(symlinkMap), directories.buildOrThrow()); } /** @@ -1156,7 +1175,7 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re ListenableFuture inMemoryOutputDownload = remoteCache.downloadBlob(context, inMemoryOutputDigest); waitForBulkTransfer( - ImmutableList.of(inMemoryOutputDownload), /* cancelRemainingOnInterrupt=*/ true); + ImmutableList.of(inMemoryOutputDownload), /* cancelRemainingOnInterrupt= */ true); byte[] data = getFromFuture(inMemoryOutputDownload); return new InMemoryOutput(inMemoryOutput, ByteString.copyFrom(data)); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 2ee40dc591f572..793020b9ced4b4 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -575,6 +575,54 @@ public void downloadOutputs_relativeDirectorySymlink_success() throws Exception assertThat(context.isLockOutputFilesCalled()).isTrue(); } + @Test + public void downloadOutputs_relativeOutputSymlinks_success() throws Exception { + // Test that download outputs works when the action result only contains output_symlinks + // and not output_file_symlinks or output_directory_symlinks, which are deprecated. + ActionResult.Builder builder = ActionResult.newBuilder(); + builder.addOutputSymlinksBuilder().setPath("outputs/a/b/link").setTarget("../../foo"); + RemoteActionResult result = + RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build())); + Spawn spawn = newSpawnFromResult(result); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(); + RemoteAction action = service.buildRemoteAction(spawn, context); + + // Doesn't check for dangling links, hence download succeeds. + service.downloadOutputs(action, result); + + Path path = execRoot.getRelative("outputs/a/b/link"); + assertThat(path.isSymbolicLink()).isTrue(); + assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("../../foo")); + assertThat(context.isLockOutputFilesCalled()).isTrue(); + } + + @Test + public void downloadOutputs_outputSymlinksCompatibility_success() throws Exception { + // Test that download outputs works when the action result contains both output_symlinks + // and output_file_symlinks (or output_directory_symlinks). + // + // Remote Execution Server may set both fields to ensure backward compatibility with + // clients that don't support output_symlinks. + ActionResult.Builder builder = ActionResult.newBuilder(); + builder.addOutputFileSymlinksBuilder().setPath("outputs/a/b/link").setTarget("foo"); + builder.addOutputSymlinksBuilder().setPath("outputs/a/b/link").setTarget("foo"); + RemoteActionResult result = + RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build())); + Spawn spawn = newSpawnFromResult(result); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(); + RemoteAction action = service.buildRemoteAction(spawn, context); + + // Doesn't check for dangling links, hence download succeeds. + service.downloadOutputs(action, result); + + Path path = execRoot.getRelative("outputs/a/b/link"); + assertThat(path.isSymbolicLink()).isTrue(); + assertThat(path.readSymbolicLink()).isEqualTo(PathFragment.create("foo")); + assertThat(context.isLockOutputFilesCalled()).isTrue(); + } + @Test public void downloadOutputs_relativeSymlinkInDirectory_success() throws Exception { Tree tree = @@ -666,6 +714,28 @@ public void downloadOutputs_absoluteSymlinkInDirectory_error() throws Exception assertThat(context.isLockOutputFilesCalled()).isTrue(); } + @Test + public void downloadOutputs_symlinkCollision_error() throws Exception { + ActionResult.Builder builder = ActionResult.newBuilder(); + builder.addOutputDirectorySymlinksBuilder().setPath("outputs/foo").setTarget("foo1"); + builder.addOutputSymlinksBuilder().setPath("outputs/foo").setTarget("foo2"); + RemoteActionResult result = + RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build())); + Spawn spawn = newSpawnFromResult(result); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(); + RemoteAction action = service.buildRemoteAction(spawn, context); + + IOException expected = + assertThrows(IOException.class, () -> service.downloadOutputs(action, result)); + + assertThat(expected.getSuppressed()).isEmpty(); + assertThat(expected).hasMessageThat().contains("Symlink path collision"); + assertThat(expected).hasMessageThat().contains("outputs/foo"); + assertThat(expected).hasMessageThat().contains("foo1"); + assertThat(expected).hasMessageThat().contains("foo2"); + } + @Test public void downloadOutputs_onFailure_maintainDirectories() throws Exception { // Test that output directories are not deleted on download failure. See @@ -1948,6 +2018,12 @@ private Spawn newSpawnFromResult( outputs.add(output); } + for (OutputSymlink symlink : result.getOutputSymlinks()) { + Path path = remotePathResolver.outputPathToLocalPath(symlink.getPath()); + Artifact output = ActionsTestUtil.createArtifact(artifactRoot, path); + outputs.add(output); + } + return newSpawn(executionInfo, outputs.build()); }