From 2a468b6035841a98da3f78b888f5ad05533e1b26 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sat, 7 Oct 2023 13:01:49 +0200 Subject: [PATCH] Address review comments --- .../lib/remote/common/RemotePathResolver.java | 14 ++- .../remote/RemoteExecutionServiceTest.java | 107 +++++++++++++++++- 2 files changed, 114 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java index 78b46decc5955c..3a2c708477c316 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java @@ -279,15 +279,19 @@ public Path outputPathToLocalPath(String outputPath) { private PathFragment map(PathFragment path) { PathFragment mappedPath = pathMapper.map(path); - inverse.put(mappedPath, path); + PathFragment previousPath = inverse.put(mappedPath, path); + Preconditions.checkState( + previousPath == null || previousPath.equals(path), + "Two different paths %s and %s map to the same path %s", + previousPath, + path, + mappedPath); return mappedPath; } private PathFragment inverseMap(PathFragment path) { - PathFragment originalPath = inverse.get(path); - Preconditions.checkState( - originalPath != null, "Failed to find original path for mapped path %s", path); - return originalPath; + return Preconditions.checkNotNull( + inverse.get(path), "Failed to find original path for mapped path %s", path); } }; } 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 bbd92830f55d3f..ca8eba38fc08df 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 @@ -95,6 +95,7 @@ import com.google.devtools.build.lib.remote.common.RemotePathResolver.DefaultRemotePathResolver; import com.google.devtools.build.lib.remote.common.RemotePathResolver.SiblingRepositoryLayoutResolver; import com.google.devtools.build.lib.remote.options.RemoteOptions; +import com.google.devtools.build.lib.remote.options.RemoteOutputsMode; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.remote.util.FakeSpawnExecutionContext; import com.google.devtools.build.lib.remote.util.InMemoryCacheClient; @@ -113,6 +114,8 @@ import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import com.google.devtools.common.options.Options; import com.google.protobuf.ByteString; +import com.google.testing.junit.testparameterinjector.TestParameter; +import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.io.IOException; import java.util.Random; import java.util.concurrent.CountDownLatch; @@ -124,14 +127,13 @@ import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; import org.mockito.ArgumentMatchers; import org.mockito.Mock; import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; /** Tests for {@link RemoteExecutionService}. */ -@RunWith(JUnit4.class) +@RunWith(TestParameterInjector.class) public class RemoteExecutionServiceTest { @Rule public final MockitoRule mockito = MockitoJUnit.rule(); @Rule public final RxNoGlobalErrorsRule rxNoGlobalErrorsRule = new RxNoGlobalErrorsRule(); @@ -1538,6 +1540,52 @@ public void downloadOutputs_missingMandatoryOutputs_reportError() throws Excepti assertThat(error).hasMessageThat().containsMatch("expected output .+ does not exist."); } + @Test + public void downloadOutputs_pathUnmapped() throws Exception { + // Test that the output of a remote action with path mapping applied is downloaded into the + // correct unmapped local path. + Digest d1 = cache.addContents(remoteActionExecutionContext, "content1"); + Digest d2 = cache.addContents(remoteActionExecutionContext, "content2"); + Artifact output1 = ActionsTestUtil.createArtifact(artifactRoot, "bin/config/dir/output1"); + Artifact output2 = ActionsTestUtil.createArtifact(artifactRoot, "bin/other_dir/output2"); + ActionResult r = + ActionResult.newBuilder() + .setExitCode(0) + // The action result includes the mapped paths. + .addOutputFiles( + OutputFile.newBuilder().setPath("outputs/bin/dir/output1").setDigest(d1)) + .addOutputFiles( + OutputFile.newBuilder().setPath("outputs/bin/other_dir/output2").setDigest(d2)) + .build(); + PathMapper pathMapper = + execPath -> PathFragment.create(execPath.getPathString().replaceAll("config/", "")); + Spawn spawn = + new SpawnBuilder("unused") + .withOutput(output1) + .withOutput(output2) + .setPathMapper(pathMapper) + .build(); + RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(r)); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + remoteOutputChecker = + new RemoteOutputChecker( + new JavaClock(), "build", RemoteOutputsMode.TOPLEVEL, ImmutableList.of()); + remoteOutputChecker.addOutputToDownload(output1); + remoteOutputChecker.addOutputToDownload(output2); + RemoteExecutionService service = newRemoteExecutionService(); + RemoteAction action = service.buildRemoteAction(spawn, context); + + InMemoryOutput inMemoryOutput = service.downloadOutputs(action, result); + + assertThat(inMemoryOutput).isNull(); + RemoteActionFileSystem actionFs = context.getActionFileSystem(); + assertThat(actionFs.getDigest(output1.getPath().asFragment())).isEqualTo(toBinaryDigest(d1)); + assertThat(readContent(output1.getPath(), UTF_8)).isEqualTo("content1"); + assertThat(actionFs.getDigest(output2.getPath().asFragment())).isEqualTo(toBinaryDigest(d2)); + assertThat(readContent(output2.getPath(), UTF_8)).isEqualTo("content2"); + assertThat(context.isLockOutputFilesCalled()).isTrue(); + } + @Test public void uploadOutputs_uploadDirectory_works() throws Exception { // Test that uploading a directory works. @@ -2195,6 +2243,61 @@ public void buildRemoteActionForRemotePersistentWorkers() throws Exception { .build()); } + @Test + public void buildRemoteActionWithPathMapping(@TestParameter boolean remoteMerkleTreeCache) + throws Exception { + remoteOptions.remoteMerkleTreeCache = remoteMerkleTreeCache; + + var mappedInput = ActionsTestUtil.createArtifact(artifactRoot, "bin/config/input1"); + fakeFileCache.createScratchInput(mappedInput, "value1"); + var unmappedInput = ActionsTestUtil.createArtifact(artifactRoot, "bin/input2"); + fakeFileCache.createScratchInput(unmappedInput, "value2"); + var outputDir = + ActionsTestUtil.createTreeArtifactWithGeneratingAction( + artifactRoot, "bin/config/output_dir"); + PathMapper pathMapper = + execPath -> PathFragment.create(execPath.getPathString().replaceAll("config/", "")); + Spawn spawn = + new SpawnBuilder("unused") + .withInputs(mappedInput, unmappedInput) + .withOutputs("outputs/bin/config/dir/output1", "outputs/bin/other_dir/output2") + .withOutputs(outputDir) + .setPathMapper(pathMapper) + .build(); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(remoteOptions); + + // Check that inputs and outputs of the remote action are mapped correctly. + var remoteAction = service.buildRemoteAction(spawn, context); + assertThat(remoteAction.getInputMap(false)) + .containsExactly( + PathFragment.create("outputs/bin/input1"), mappedInput, + PathFragment.create("outputs/bin/input2"), unmappedInput); + assertThat(remoteAction.getCommand().getOutputFilesList()) + .containsExactly("outputs/bin/dir/output1", "outputs/bin/other_dir/output2"); + assertThat(remoteAction.getCommand().getOutputDirectoriesList()) + .containsExactly("outputs/bin/output_dir"); + assertThat(remoteAction.getCommand().getOutputPathsList()) + .containsExactly( + "outputs/bin/dir/output1", "outputs/bin/other_dir/output2", "outputs/bin/output_dir"); + + // Check that the Merkle tree nodes are mapped correctly, including the output directory. + var merkleTree = remoteAction.getMerkleTree(); + var outputsDirectory = + merkleTree.getDirectoryByDigest(merkleTree.getRootProto().getDirectories(0).getDigest()); + assertThat(outputsDirectory.getDirectoriesCount()).isEqualTo(1); + var binDirectory = + merkleTree.getDirectoryByDigest(outputsDirectory.getDirectories(0).getDigest()); + assertThat( + binDirectory.getFilesList().stream().map(FileNode::getName).collect(toImmutableList())) + .containsExactly("input1", "input2"); + assertThat( + binDirectory.getDirectoriesList().stream() + .map(DirectoryNode::getName) + .collect(toImmutableList())) + .containsExactly("output_dir"); + } + private Spawn newSpawnFromResult(RemoteActionResult result) { return newSpawnFromResult(ImmutableMap.of(), result); }