Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@fmeum
fmeum committed Oct 7, 2023
1 parent 2f4e9e9 commit 2a468b6
Show file tree
Hide file tree
Showing 2 changed files with 114 additions and 7 deletions.
14 changes: 9 additions & 5 deletions src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
};
}
Expand Down
107 changes: 105 additions & 2 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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();
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
Expand Down

0 comments on commit 2a468b6

Please sign in to comment.