From b7fdadeb0e820268175db9a2c62f5359d19e2f1f Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 20 Apr 2023 11:02:32 +0200 Subject: [PATCH 1/4] Wire up `PathMapper` in `SpawnInputsExpander` `PathMapper`s rewrite paths in command lines to make them more cache friendly, which requires executor support to stage files at the rewritten paths. This commit wires up the `PathMapper` used by a given `Spawn` in `SpawnInputsExpander`, which takes care of this for inputs to `Spawn`s executed in a sandbox or remotely. Constructs specific to Blaze, filesets and archived tree artifacts, are not covered by this change. Work towards #6526 --- .../build/lib/actions/PathMapper.java | 10 ++ .../analysis/actions/StrippingPathMapper.java | 5 + .../build/lib/exec/SpawnInputExpander.java | 75 ++++++++--- .../lib/exec/SpawnInputExpanderTest.java | 117 +++++++++++++++--- .../build/lib/exec/util/SpawnBuilder.java | 2 +- .../remote/RemoteExecutionServiceTest.java | 41 +++--- 6 files changed, 203 insertions(+), 47 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java b/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java index 1d006ed7678737..d0065344a8bbfe 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java +++ b/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java @@ -80,6 +80,16 @@ default boolean isNoop() { return this == NOOP; } + /** + * Returns an opaque object whose equality class encodes the behavior of this mapper for use in + * in-memory cache keys. + * + *

The default implementation returns the {@link Class} of the mapper. + */ + default Object cacheKey() { + return this.getClass(); + } + /** A {@link PathMapper} that doesn't change paths. */ PathMapper NOOP = execPath -> execPath; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java index cfdd6d54b8e475..47086e5b18dd9d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java @@ -176,6 +176,11 @@ public ExceptionlessMapFn getMapFn(@Nullable String previousFlag) { } return MapFn.DEFAULT; } + + @Override + public Object cacheKey() { + return GUID; + } }; } diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java index 7ee6986efe5392..5711ccbcacdabf 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java @@ -33,6 +33,7 @@ import com.google.devtools.build.lib.actions.FilesetOutputSymlink; import com.google.devtools.build.lib.actions.ForbiddenActionInputException; import com.google.devtools.build.lib.actions.InputMetadataProvider; +import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.RunfilesSupplier; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; @@ -125,6 +126,7 @@ void addRunfilesToInputs( RunfilesSupplier runfilesSupplier, InputMetadataProvider actionFileCache, ArtifactExpander artifactExpander, + PathMapper pathMapper, PathFragment baseDirectory) throws IOException, ForbiddenActionInputException { Map> rootsAndMappings = @@ -155,7 +157,8 @@ void addRunfilesToInputs( for (ActionInput input : expandedInputs) { addMapping( inputMap, - location.getRelative(((TreeFileArtifact) input).getParentRelativePath()), + mapForRunfiles(pathMapper, root, location).getRelative( + ((TreeFileArtifact) input).getParentRelativePath()), input, baseDirectory); } @@ -172,10 +175,12 @@ void addRunfilesToInputs( if (strict) { failIfDirectory(actionFileCache, localArtifact); } - addMapping(inputMap, location, localArtifact, baseDirectory); + addMapping(inputMap, mapForRunfiles(pathMapper, root, location), localArtifact, + baseDirectory); } } else { - addMapping(inputMap, location, VirtualActionInput.EMPTY_MARKER, baseDirectory); + addMapping(inputMap, mapForRunfiles(pathMapper, root, location), + VirtualActionInput.EMPTY_MARKER, baseDirectory); } } } @@ -186,11 +191,12 @@ public Map addRunfilesToInputs( RunfilesSupplier runfilesSupplier, InputMetadataProvider actionFileCache, ArtifactExpander artifactExpander, + PathMapper pathMapper, PathFragment baseDirectory) throws IOException, ForbiddenActionInputException { Map inputMap = new HashMap<>(); addRunfilesToInputs( - inputMap, runfilesSupplier, actionFileCache, artifactExpander, baseDirectory); + inputMap, runfilesSupplier, actionFileCache, artifactExpander, pathMapper, baseDirectory); return inputMap; } @@ -243,6 +249,7 @@ private static void addInputs( Map inputMap, NestedSet inputFiles, ArtifactExpander artifactExpander, + PathMapper pathMapper, PathFragment baseDirectory) { // Actions that accept TreeArtifacts as inputs generally expect the directory corresponding // to the artifact to be created, even if it is empty. We explicitly keep empty TreeArtifacts @@ -251,7 +258,14 @@ private static void addInputs( ActionInputHelper.expandArtifacts( inputFiles, artifactExpander, /* keepEmptyTreeArtifacts= */ true); for (ActionInput input : inputs) { - addMapping(inputMap, input.getExecPath(), input, baseDirectory); + if (input instanceof TreeFileArtifact) { + addMapping(inputMap, + pathMapper.map(((TreeFileArtifact) input).getParent().getExecPath()) + .getRelative(((TreeFileArtifact) input).getParentRelativePath()), input, + baseDirectory); + } else { + addMapping(inputMap, pathMapper.map(input.getExecPath()), input, baseDirectory); + } } } @@ -273,17 +287,35 @@ public SortedMap getInputMapping( InputMetadataProvider actionInputFileCache) throws IOException, ForbiddenActionInputException { TreeMap inputMap = new TreeMap<>(); - addInputs(inputMap, spawn.getInputFiles(), artifactExpander, baseDirectory); + addInputs(inputMap, spawn.getInputFiles(), artifactExpander, spawn.getPathMapper(), + baseDirectory); addRunfilesToInputs( inputMap, spawn.getRunfilesSupplier(), actionInputFileCache, artifactExpander, + spawn.getPathMapper(), baseDirectory); addFilesetManifests(spawn.getFilesetMappings(), inputMap, baseDirectory); return inputMap; } + private static PathFragment mapForRunfiles( + PathMapper pathMapper, PathFragment runfilesDir, PathFragment execPath) { + if (pathMapper.isNoop()) { + return execPath; + } + String runfilesDirName = runfilesDir.getBaseName(); + Preconditions.checkArgument(runfilesDirName.endsWith(".runfiles")); + // Derive the path of the executable, apply the path mapping to it and then rederive the path + // of the runfiles dir. + PathFragment executable = + runfilesDir.replaceName( + runfilesDirName.substring(0, runfilesDirName.length() - ".runfiles".length())); + return pathMapper.map(executable).replaceName(runfilesDirName) + .getRelative(execPath.relativeTo(runfilesDir)); + } + /** The interface for accessing part of the input hierarchy. */ public interface InputWalker { @@ -326,19 +358,25 @@ public void walkInputs( InputMetadataProvider actionInputFileCache, InputVisitor visitor) throws IOException, ForbiddenActionInputException { - walkNestedSetInputs(baseDirectory, spawn.getInputFiles(), artifactExpander, visitor); + walkNestedSetInputs(baseDirectory, spawn.getInputFiles(), artifactExpander, + spawn.getPathMapper(), visitor); RunfilesSupplier runfilesSupplier = spawn.getRunfilesSupplier(); visitor.visit( // Cache key for the sub-mapping containing the runfiles inputs for this spawn. - ImmutableList.of(runfilesSupplier, baseDirectory), + ImmutableList.of(runfilesSupplier, baseDirectory, spawn.getPathMapper().cacheKey()), new InputWalker() { @Override public SortedMap getLeavesInputMapping() throws IOException, ForbiddenActionInputException { TreeMap inputMap = new TreeMap<>(); addRunfilesToInputs( - inputMap, runfilesSupplier, actionInputFileCache, artifactExpander, baseDirectory); + inputMap, + runfilesSupplier, + actionInputFileCache, + artifactExpander, + spawn.getPathMapper(), + baseDirectory); return inputMap; } }); @@ -348,7 +386,7 @@ public SortedMap getLeavesInputMapping() // improved runtime. visitor.visit( // Cache key for the sub-mapping containing the fileset inputs for this spawn. - ImmutableList.of(filesetMappings, baseDirectory), + ImmutableList.of(filesetMappings, baseDirectory, spawn.getPathMapper().cacheKey()), new InputWalker() { @Override public SortedMap getLeavesInputMapping() @@ -365,11 +403,12 @@ private void walkNestedSetInputs( PathFragment baseDirectory, NestedSet someInputFiles, ArtifactExpander artifactExpander, + PathMapper pathMapper, InputVisitor visitor) throws IOException, ForbiddenActionInputException { visitor.visit( // Cache key for the sub-mapping containing the files in this nested set. - ImmutableList.of(someInputFiles.toNode(), baseDirectory), + ImmutableList.of(someInputFiles.toNode(), baseDirectory, pathMapper.cacheKey()), new InputWalker() { @Override public SortedMap getLeavesInputMapping() { @@ -384,6 +423,7 @@ public SortedMap getLeavesInputMapping() { inputMap, NestedSetBuilder.wrap(someInputFiles.getOrder(), leaves), artifactExpander, + pathMapper, baseDirectory); return inputMap; } @@ -394,11 +434,16 @@ public void visitNonLeaves(InputVisitor childVisitor) for (ActionInput input : someInputFiles.getLeaves()) { if (isTreeArtifact(input)) { walkTreeInputs( - baseDirectory, (SpecialArtifact) input, artifactExpander, childVisitor); + baseDirectory, + (SpecialArtifact) input, + artifactExpander, + pathMapper, + childVisitor); } } for (NestedSet subInputs : someInputFiles.getNonLeaves()) { - walkNestedSetInputs(baseDirectory, subInputs, artifactExpander, childVisitor); + walkNestedSetInputs( + baseDirectory, subInputs, artifactExpander, pathMapper, childVisitor); } } }); @@ -409,11 +454,12 @@ private void walkTreeInputs( PathFragment baseDirectory, SpecialArtifact tree, ArtifactExpander artifactExpander, + PathMapper pathMapper, InputVisitor visitor) throws IOException, ForbiddenActionInputException { visitor.visit( // Cache key for the sub-mapping containing the files in this tree artifact. - ImmutableList.of(tree, baseDirectory), + ImmutableList.of(tree, baseDirectory, pathMapper.cacheKey()), new InputWalker() { @Override public SortedMap getLeavesInputMapping() { @@ -422,6 +468,7 @@ public SortedMap getLeavesInputMapping() { inputMap, NestedSetBuilder.create(Order.STABLE_ORDER, tree), artifactExpander, + pathMapper, baseDirectory); return inputMap; } diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java index f1ffe6e5b00dec..ee2fc71157b5aa 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java @@ -37,6 +37,7 @@ import com.google.devtools.build.lib.actions.FilesetManifest; import com.google.devtools.build.lib.actions.FilesetOutputSymlink; import com.google.devtools.build.lib.actions.ForbiddenActionInputException; +import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.RunfilesSupplier; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; @@ -81,8 +82,8 @@ public class SpawnInputExpanderTest { public void testEmptyRunfiles() throws Exception { RunfilesSupplier supplier = EmptyRunfilesSupplier.INSTANCE; FakeActionInputFileCache mockCache = new FakeActionInputFileCache(); - expander.addRunfilesToInputs( - inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, PathFragment.EMPTY_FRAGMENT); + expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, + PathMapper.NOOP, PathFragment.EMPTY_FRAGMENT); assertThat(inputMappings).isEmpty(); } @@ -100,8 +101,8 @@ public void testRunfilesSingleFile() throws Exception { artifact, FileArtifactValue.createForNormalFile(FAKE_DIGEST, /*proxy=*/ null, /*size=*/ 0L)); - expander.addRunfilesToInputs( - inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, PathFragment.EMPTY_FRAGMENT); + expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, + PathMapper.NOOP, PathFragment.EMPTY_FRAGMENT); assertThat(inputMappings).hasSize(1); assertThat(inputMappings) .containsEntry(PathFragment.create("runfiles/workspace/dir/file"), artifact); @@ -135,8 +136,8 @@ public ImmutableList getFileset(Artifact artifact) { } }; - expander.addRunfilesToInputs( - inputMappings, supplier, mockCache, filesetExpander, PathFragment.EMPTY_FRAGMENT); + expander.addRunfilesToInputs(inputMappings, supplier, mockCache, filesetExpander, + PathMapper.NOOP, PathFragment.EMPTY_FRAGMENT); assertThat(inputMappings).hasSize(1); assertThat(inputMappings) .containsEntry( @@ -165,6 +166,7 @@ public void testRunfilesDirectoryStrict() { supplier, mockCache, NO_ARTIFACT_EXPANDER, + PathMapper.NOOP, PathFragment.EMPTY_FRAGMENT)); assertThat(expected).hasMessageThat().isEqualTo("Not a file: dir/file"); } @@ -182,8 +184,8 @@ public void testRunfilesDirectoryNonStrict() throws Exception { mockCache.put(artifact, FileArtifactValue.createForDirectoryWithMtime(-1)); expander = new SpawnInputExpander(execRoot, /*strict=*/ false); - expander.addRunfilesToInputs( - inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, PathFragment.EMPTY_FRAGMENT); + expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, + PathMapper.NOOP, PathFragment.EMPTY_FRAGMENT); assertThat(inputMappings).hasSize(1); assertThat(inputMappings) .containsEntry(PathFragment.create("runfiles/workspace/dir/file"), artifact); @@ -211,8 +213,8 @@ public void testRunfilesTwoFiles() throws Exception { artifact2, FileArtifactValue.createForNormalFile(FAKE_DIGEST, /*proxy=*/ null, /*size=*/ 12L)); - expander.addRunfilesToInputs( - inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, PathFragment.EMPTY_FRAGMENT); + expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, + PathMapper.NOOP, PathFragment.EMPTY_FRAGMENT); assertThat(inputMappings).hasSize(2); assertThat(inputMappings) .containsEntry(PathFragment.create("runfiles/workspace/dir/file"), artifact1); @@ -220,6 +222,45 @@ public void testRunfilesTwoFiles() throws Exception { .containsEntry(PathFragment.create("runfiles/workspace/dir/baz"), artifact2); } + @Test + public void testRunfilesTwoFiles_pathMapped() throws Exception { + Artifact artifact1 = + ActionsTestUtil.createArtifact( + ArtifactRoot.asSourceRoot(Root.fromPath(fs.getPath("/root"))), + fs.getPath("/root/dir/file")); + Artifact artifact2 = + ActionsTestUtil.createArtifact( + ArtifactRoot.asSourceRoot(Root.fromPath(fs.getPath("/root"))), + fs.getPath("/root/dir/baz")); + Runfiles runfiles = + new Runfiles.Builder("workspace").addArtifact(artifact1).addArtifact(artifact2).build(); + RunfilesSupplier supplier = + AnalysisTestUtil.createRunfilesSupplier( + PathFragment.create("bazel-out/k8-opt/bin/foo.runfiles"), runfiles); + FakeActionInputFileCache mockCache = new FakeActionInputFileCache(); + mockCache.put( + artifact1, + FileArtifactValue.createForNormalFile(FAKE_DIGEST, /* proxy= */ null, /* size= */ 1L)); + mockCache.put( + artifact2, + FileArtifactValue.createForNormalFile(FAKE_DIGEST, /* proxy= */ null, /* size= */ 12L)); + + expander.addRunfilesToInputs( + inputMappings, + supplier, + mockCache, + NO_ARTIFACT_EXPANDER, + execPath -> PathFragment.create(execPath.getPathString().replace("k8-opt/", "")), + PathFragment.EMPTY_FRAGMENT); + assertThat(inputMappings).hasSize(2); + assertThat(inputMappings) + .containsEntry( + PathFragment.create("bazel-out/bin/foo.runfiles/workspace/dir/file"), artifact1); + assertThat(inputMappings) + .containsEntry( + PathFragment.create("bazel-out/bin/foo.runfiles/workspace/dir/baz"), artifact2); + } + @Test public void testRunfilesSymlink() throws Exception { Artifact artifact = @@ -237,8 +278,8 @@ public void testRunfilesSymlink() throws Exception { artifact, FileArtifactValue.createForNormalFile(FAKE_DIGEST, /*proxy=*/ null, /*size=*/ 1L)); - expander.addRunfilesToInputs( - inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, PathFragment.EMPTY_FRAGMENT); + expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, + PathMapper.NOOP, PathFragment.EMPTY_FRAGMENT); assertThat(inputMappings).hasSize(1); assertThat(inputMappings) .containsEntry(PathFragment.create("runfiles/workspace/symlink"), artifact); @@ -261,8 +302,8 @@ public void testRunfilesRootSymlink() throws Exception { artifact, FileArtifactValue.createForNormalFile(FAKE_DIGEST, /*proxy=*/ null, /*size=*/ 1L)); - expander.addRunfilesToInputs( - inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, PathFragment.EMPTY_FRAGMENT); + expander.addRunfilesToInputs(inputMappings, supplier, mockCache, NO_ARTIFACT_EXPANDER, + PathMapper.NOOP, PathFragment.EMPTY_FRAGMENT); assertThat(inputMappings).hasSize(2); assertThat(inputMappings).containsEntry(PathFragment.create("runfiles/symlink"), artifact); // If there's no other entry, Runfiles adds an empty file in the workspace to make sure the @@ -294,8 +335,8 @@ public void testRunfilesWithTreeArtifacts() throws Exception { fakeCache.put(file1, FileArtifactValue.createForTesting(file1)); fakeCache.put(file2, FileArtifactValue.createForTesting(file2)); - expander.addRunfilesToInputs( - inputMappings, supplier, fakeCache, artifactExpander, PathFragment.EMPTY_FRAGMENT); + expander.addRunfilesToInputs(inputMappings, supplier, fakeCache, artifactExpander, + PathMapper.NOOP, PathFragment.EMPTY_FRAGMENT); assertThat(inputMappings).hasSize(2); assertThat(inputMappings) .containsEntry(PathFragment.create("runfiles/workspace/treeArtifact/file1"), file1); @@ -303,6 +344,45 @@ public void testRunfilesWithTreeArtifacts() throws Exception { .containsEntry(PathFragment.create("runfiles/workspace/treeArtifact/file2"), file2); } + @Test + public void testRunfilesWithTreeArtifacts_pathMapped() throws Exception { + SpecialArtifact treeArtifact = createTreeArtifact("treeArtifact"); + assertThat(treeArtifact.isTreeArtifact()).isTrue(); + TreeFileArtifact file1 = TreeFileArtifact.createTreeOutput(treeArtifact, "file1"); + TreeFileArtifact file2 = TreeFileArtifact.createTreeOutput(treeArtifact, "file2"); + FileSystemUtils.writeContentAsLatin1(file1.getPath(), "foo"); + FileSystemUtils.writeContentAsLatin1(file2.getPath(), "bar"); + + Runfiles runfiles = new Runfiles.Builder("workspace").addArtifact(treeArtifact).build(); + ArtifactExpander artifactExpander = + (Artifact artifact, Collection output) -> { + if (artifact.equals(treeArtifact)) { + output.addAll(Arrays.asList(file1, file2)); + } + }; + RunfilesSupplier supplier = + AnalysisTestUtil.createRunfilesSupplier( + PathFragment.create("bazel-out/k8-opt/bin/foo.runfiles"), runfiles); + FakeActionInputFileCache fakeCache = new FakeActionInputFileCache(); + fakeCache.put(file1, FileArtifactValue.createForTesting(file1)); + fakeCache.put(file2, FileArtifactValue.createForTesting(file2)); + + expander.addRunfilesToInputs( + inputMappings, + supplier, + fakeCache, + artifactExpander, + execPath -> PathFragment.create(execPath.getPathString().replace("k8-opt/", "")), + PathFragment.EMPTY_FRAGMENT); + assertThat(inputMappings).hasSize(2); + assertThat(inputMappings) + .containsEntry( + PathFragment.create("bazel-out/bin/foo.runfiles/workspace/treeArtifact/file1"), file1); + assertThat(inputMappings) + .containsEntry( + PathFragment.create("bazel-out/bin/foo.runfiles/workspace/treeArtifact/file2"), file2); + } + @Test public void testRunfilesWithArchivedTreeArtifacts() throws Exception { SpecialArtifact treeArtifact = createTreeArtifact("treeArtifact"); @@ -335,6 +415,7 @@ public ArchivedTreeArtifact getArchivedTreeArtifact(SpecialArtifact treeArtifact supplier, new FakeActionInputFileCache(), artifactExpander, + PathMapper.NOOP, PathFragment.EMPTY_FRAGMENT); assertThat(inputMappings).hasSize(1); assertThat(inputMappings) @@ -366,8 +447,8 @@ public void testRunfilesWithTreeArtifactsInSymlinks() throws Exception { fakeCache.put(file1, FileArtifactValue.createForTesting(file1)); fakeCache.put(file2, FileArtifactValue.createForTesting(file2)); - expander.addRunfilesToInputs( - inputMappings, supplier, fakeCache, artifactExpander, PathFragment.EMPTY_FRAGMENT); + expander.addRunfilesToInputs(inputMappings, supplier, fakeCache, artifactExpander, + PathMapper.NOOP, PathFragment.EMPTY_FRAGMENT); assertThat(inputMappings).hasSize(2); assertThat(inputMappings) .containsEntry(PathFragment.create("runfiles/workspace/symlink/file1"), file1); diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java b/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java index 294ba3396ac4a6..0a8b407d64124a 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java +++ b/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java @@ -61,7 +61,7 @@ public final class SpawnBuilder { private RunfilesSupplier runfilesSupplier = EmptyRunfilesSupplier.INSTANCE; private ResourceSet resourceSet = ResourceSet.ZERO; - private PathMapper pathMapper; + private PathMapper pathMapper = PathMapper.NOOP; private boolean builtForToolConfiguration; /** 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 3780956a765056..bbd92830f55d3f 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 @@ -67,6 +67,7 @@ import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; import com.google.devtools.build.lib.actions.EmptyRunfilesSupplier; import com.google.devtools.build.lib.actions.ExecutionRequirements; +import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.actions.SimpleSpawn; import com.google.devtools.build.lib.actions.Spawn; @@ -2081,12 +2082,17 @@ public void buildMerkleTree_withMemoization_works() throws Exception { verify(service, times(6)).uncachedBuildMerkleTreeVisitor(any(), any(), any()); assertThat(service.getMerkleTreeCache().asMap().keySet()) .containsExactly( - ImmutableList.of(ImmutableMap.of(), PathFragment.EMPTY_FRAGMENT), // fileset mapping - ImmutableList.of(EmptyRunfilesSupplier.INSTANCE, PathFragment.EMPTY_FRAGMENT), - ImmutableList.of(tree, PathFragment.EMPTY_FRAGMENT), - ImmutableList.of(nodeRoot1.toNode(), PathFragment.EMPTY_FRAGMENT), - ImmutableList.of(nodeFoo1.toNode(), PathFragment.EMPTY_FRAGMENT), - ImmutableList.of(nodeBar.toNode(), PathFragment.EMPTY_FRAGMENT)); + ImmutableList.of(ImmutableMap.of(), PathFragment.EMPTY_FRAGMENT, + PathMapper.NOOP.getClass()), // fileset mapping + ImmutableList.of(EmptyRunfilesSupplier.INSTANCE, PathFragment.EMPTY_FRAGMENT, + PathMapper.NOOP.getClass()), + ImmutableList.of(tree, PathFragment.EMPTY_FRAGMENT, PathMapper.NOOP.getClass()), + ImmutableList.of(nodeRoot1.toNode(), PathFragment.EMPTY_FRAGMENT, + PathMapper.NOOP.getClass()), + ImmutableList.of(nodeFoo1.toNode(), PathFragment.EMPTY_FRAGMENT, + PathMapper.NOOP.getClass()), + ImmutableList.of(nodeBar.toNode(), PathFragment.EMPTY_FRAGMENT, + PathMapper.NOOP.getClass())); // act second time service.buildRemoteAction(spawn2, context2); @@ -2095,14 +2101,21 @@ public void buildMerkleTree_withMemoization_works() throws Exception { verify(service, times(6 + 2)).uncachedBuildMerkleTreeVisitor(any(), any(), any()); assertThat(service.getMerkleTreeCache().asMap().keySet()) .containsExactly( - ImmutableList.of(ImmutableMap.of(), PathFragment.EMPTY_FRAGMENT), // fileset mapping - ImmutableList.of(EmptyRunfilesSupplier.INSTANCE, PathFragment.EMPTY_FRAGMENT), - ImmutableList.of(tree, PathFragment.EMPTY_FRAGMENT), - ImmutableList.of(nodeRoot1.toNode(), PathFragment.EMPTY_FRAGMENT), - ImmutableList.of(nodeRoot2.toNode(), PathFragment.EMPTY_FRAGMENT), - ImmutableList.of(nodeFoo1.toNode(), PathFragment.EMPTY_FRAGMENT), - ImmutableList.of(nodeFoo2.toNode(), PathFragment.EMPTY_FRAGMENT), - ImmutableList.of(nodeBar.toNode(), PathFragment.EMPTY_FRAGMENT)); + ImmutableList.of(ImmutableMap.of(), PathFragment.EMPTY_FRAGMENT, + PathMapper.NOOP.getClass()), // fileset mapping + ImmutableList.of(EmptyRunfilesSupplier.INSTANCE, PathFragment.EMPTY_FRAGMENT, + PathMapper.NOOP.getClass()), + ImmutableList.of(tree, PathFragment.EMPTY_FRAGMENT, PathMapper.NOOP.getClass()), + ImmutableList.of(nodeRoot1.toNode(), PathFragment.EMPTY_FRAGMENT, + PathMapper.NOOP.getClass()), + ImmutableList.of(nodeRoot2.toNode(), PathFragment.EMPTY_FRAGMENT, + PathMapper.NOOP.getClass()), + ImmutableList.of(nodeFoo1.toNode(), PathFragment.EMPTY_FRAGMENT, + PathMapper.NOOP.getClass()), + ImmutableList.of(nodeFoo2.toNode(), PathFragment.EMPTY_FRAGMENT, + PathMapper.NOOP.getClass()), + ImmutableList.of(nodeBar.toNode(), PathFragment.EMPTY_FRAGMENT, + PathMapper.NOOP.getClass())); } @Test From 790169a324f3c3396f83e99f4bf5cb479e781522 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 5 Oct 2023 09:06:28 +0200 Subject: [PATCH 2/4] Address review comments --- .../devtools/build/lib/actions/PathMapper.java | 17 +++++++++++++---- .../analysis/actions/StrippingPathMapper.java | 5 ----- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java b/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java index d0065344a8bbfe..0e7dcc546dd531 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java +++ b/src/main/java/com/google/devtools/build/lib/actions/PathMapper.java @@ -33,7 +33,15 @@ * part (e.g. "k8-fastbuild") from exec paths to allow for cross-configuration cache hits. */ public interface PathMapper { - /** Returns the exec path with the path mapping applied. */ + /** + * Returns the exec path with the path mapping applied. + * + *

Path mappers may return paths with different roots for two paths that have the same root + * (e.g., they may map an artifact at {@code bazel-out/k8-fastbuild/bin/pkg/foo} to {@code + * bazel-out//bin/pkg/foo}). Paths of artifacts that should share the same + * parent directory, such as runfiles or tree artifact files, should thus be derived from the + * mapped path of their parent. + */ PathFragment map(PathFragment execPath); /** Returns the exec path of the input with the path mapping applied. */ @@ -81,10 +89,11 @@ default boolean isNoop() { } /** - * Returns an opaque object whose equality class encodes the behavior of this mapper for use in - * in-memory cache keys. + * Returns an opaque object whose equality class should encode all information that goes into the + * behavior of the {@link #map(PathFragment)} function of this path mapper. This is used as a key + * for in-memory caches. * - *

The default implementation returns the {@link Class} of the mapper. + *

The default implementation returns the {@link Class} of the path mapper. */ default Object cacheKey() { return this.getClass(); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java index 47086e5b18dd9d..cfdd6d54b8e475 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java @@ -176,11 +176,6 @@ public ExceptionlessMapFn getMapFn(@Nullable String previousFlag) { } return MapFn.DEFAULT; } - - @Override - public Object cacheKey() { - return GUID; - } }; } From a6aa1f02a2a4c68082c011e815c5c1dee814aa6d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 5 Oct 2023 15:16:16 +0200 Subject: [PATCH 3/4] Add TODOs --- .../com/google/devtools/build/lib/exec/SpawnInputExpander.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java index 5711ccbcacdabf..0efaf28d79a996 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java @@ -147,6 +147,7 @@ void addRunfilesToInputs( ? null : artifactExpander.getArchivedTreeArtifact((SpecialArtifact) localArtifact); if (archivedTreeArtifact != null) { + // TODO(bazel-team): Add path mapping support for archived tree artifacts. addMapping(inputMap, location, localArtifact, baseDirectory); } else { List expandedInputs = @@ -170,6 +171,7 @@ void addRunfilesToInputs( } catch (MissingExpansionException e) { throw new IllegalStateException(e); } + // TODO(bazel-team): Add path mapping support for filesets. addFilesetManifest(location, localArtifact, filesetLinks, inputMap, baseDirectory); } else { if (strict) { @@ -241,6 +243,7 @@ void addFilesetManifest( value == null ? VirtualActionInput.EMPTY_MARKER : ActionInputHelper.fromPath(execRoot.getRelative(value).asFragment()); + // TODO(bazel-team): Add path mapping support for filesets. addMapping(inputMappings, mapping.getKey(), artifact, baseDirectory); } } From da78f156cde54f96509108e46365f5a10cea142c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 5 Oct 2023 16:40:57 +0200 Subject: [PATCH 4/4] Make test more specific --- .../lib/exec/SpawnInputExpanderTest.java | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java index ee2fc71157b5aa..8d38930c8d48cb 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java @@ -22,6 +22,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.hash.HashFunction; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.Artifact; @@ -54,6 +55,7 @@ import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Collection; import java.util.HashMap; @@ -367,20 +369,39 @@ public void testRunfilesWithTreeArtifacts_pathMapped() throws Exception { fakeCache.put(file1, FileArtifactValue.createForTesting(file1)); fakeCache.put(file2, FileArtifactValue.createForTesting(file2)); + PathMapper pathMapper = + execPath -> { + // Replace the config segment "k8-opt" in "bazel-bin/k8-opt/bin" with a hash of the full + // path to verify that the new paths are constructed by appending the child paths to the + // mapped parent path, not by mapping the child paths directly. + PathFragment runfilesPath = execPath.subFragment(3); + String runfilesPathHash = + DigestHashFunction.SHA256 + .getHashFunction() + .hashString(runfilesPath.getPathString(), StandardCharsets.UTF_8) + .toString(); + return execPath + .subFragment(0, 1) + .getRelative(runfilesPathHash.substring(0, 8)) + .getRelative(execPath.subFragment(2)); + }; + expander.addRunfilesToInputs( inputMappings, supplier, fakeCache, artifactExpander, - execPath -> PathFragment.create(execPath.getPathString().replace("k8-opt/", "")), + pathMapper, PathFragment.EMPTY_FRAGMENT); assertThat(inputMappings).hasSize(2); assertThat(inputMappings) .containsEntry( - PathFragment.create("bazel-out/bin/foo.runfiles/workspace/treeArtifact/file1"), file1); + PathFragment.create("bazel-out/2c26b46b/bin/foo.runfiles/workspace/treeArtifact/file1"), + file1); assertThat(inputMappings) .containsEntry( - PathFragment.create("bazel-out/bin/foo.runfiles/workspace/treeArtifact/file2"), file2); + PathFragment.create("bazel-out/2c26b46b/bin/foo.runfiles/workspace/treeArtifact/file2"), + file2); } @Test