From ab82887b0d3a55e41e4bd2012e4e5ef7914ae005 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 18 Apr 2022 13:56:15 +0200 Subject: [PATCH] Unify sandbox/remote handling of empty TreeArtifact inputs Actions that take a TreeArtifact as input should see a corresponding directory even if the TreeArtifact is empty. Previously, SandboxHelpers contained special handling for the case of empty TreeArtifact action inputs to ensure that they are added to the sandbox as empty directories. As pointed out in a comment, this logic should live in SpawnInputExpander, where it would also apply to remote execution. This commit adds a integration tests for this previously untested case, extracts the logic into SpawnInputExpander and adapts DirectoryTreeBuilder to handle empty TreeArtifacts when creating the Merkle trees. Note: The Merkle tree builder now reports an error when it encounters a non-empty TreeArtifact. Such an artifact should have been expanded by SpawnInputExpander and if it wasn't, e.g. because it wasn't properly registered with Skyframe, it can't be expanded at this point anyway. The tests uncovered that the spawn for split coverage postprocessing declared the coverage dir artifact as such an input. In this case, it can simply be removed as the coverage script creates the coverage dir if it doesn't exist. --- .../build/lib/actions/ActionInputHelper.java | 14 ++++-- .../devtools/build/lib/actions/Artifact.java | 39 ++++++++++----- .../build/lib/exec/SpawnInputExpander.java | 15 ++++-- .../lib/exec/StandaloneTestStrategy.java | 1 - .../build/lib/remote/merkletree/BUILD | 1 + .../merkletree/DirectoryTreeBuilder.java | 30 ++++++++++++ .../lib/remote/merkletree/MerkleTree.java | 3 +- .../sandbox/DarwinSandboxedSpawnRunner.java | 2 - .../sandbox/DockerSandboxedSpawnRunner.java | 2 - .../sandbox/LinuxSandboxedSpawnRunner.java | 2 - .../ProcessWrapperSandboxedSpawnRunner.java | 2 - .../build/lib/sandbox/SandboxHelpers.java | 22 --------- .../sandbox/WindowsSandboxedSpawnRunner.java | 2 - .../build/lib/worker/WorkerFilesHash.java | 3 +- .../build/lib/worker/WorkerSpawnRunner.java | 5 +- .../build/lib/sandbox/SandboxHelpersTest.java | 18 +++---- src/test/shell/bazel/bazel_sandboxing_test.sh | 34 ++++++++++++- .../bazel/remote/remote_execution_test.sh | 48 +++++++++++++++++++ 18 files changed, 173 insertions(+), 70 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java index ea5f758f408121..5749ef6d618d74 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java @@ -103,12 +103,17 @@ public PathFragment getExecPath() { } /** - * Expands middleman artifacts in a sequence of {@link ActionInput}s. + * Expands middleman and tree artifacts in a sequence of {@link ActionInput}s. * - *

Non-middleman artifacts are returned untouched. + * The returned list never contains middleman artifacts. It contains tree artifacts only if + * {@code keepEmptyTreeArtifacts} is true. In this case, all returned tree artifacts have empty + * expansion under the provided {@link ArtifactExpander}. + * + *

Non-middleman, non-tree artifacts are returned untouched. */ public static List expandArtifacts( - NestedSet inputs, ArtifactExpander artifactExpander) { + NestedSet inputs, ArtifactExpander artifactExpander, + boolean keepEmptyTreeArtifacts) { List result = new ArrayList<>(); List containedArtifacts = new ArrayList<>(); for (ActionInput input : inputs.toList()) { @@ -118,7 +123,8 @@ public static List expandArtifacts( } containedArtifacts.add((Artifact) input); } - Artifact.addExpandedArtifacts(containedArtifacts, result, artifactExpander); + Artifact.addExpandedArtifacts(containedArtifacts, result, artifactExpander, + keepEmptyTreeArtifacts); return result; } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index 780e263ea3ba27..d604b99ca74306 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java @@ -1520,26 +1520,39 @@ public static String joinRootRelativePaths(String delimiter, Iterable return Joiner.on(delimiter).join(toRootRelativePaths(artifacts)); } - /** Adds a collection of artifacts to a given collection, with middleman actions expanded once. */ + /** + * Adds a collection of artifacts to a given collection, with middleman actions and tree artifacts + * expanded once. + * + * The constructed list never contains middleman artifacts. It contains tree artifacts only if + * {@code keepEmptyTreeArtifacts} is true. In this case, all added tree artifacts have empty + * expansion under the provided {@link ArtifactExpander}. + */ static void addExpandedArtifacts( Iterable artifacts, Collection output, - ArtifactExpander artifactExpander) { - addExpandedArtifacts(artifacts, output, Functions.identity(), artifactExpander); + ArtifactExpander artifactExpander, + boolean keepEmptyTreeArtifacts) { + addExpandedArtifacts(artifacts, output, Functions.identity(), artifactExpander, + keepEmptyTreeArtifacts); } /** - * Converts a collection of artifacts into the outputs computed by - * outputFormatter and adds them to a given collection. Middleman artifacts - * are expanded once. + * Converts a collection of artifacts into the outputs computed by outputFormatter and adds them + * to a given collection. Middleman artifacts and tree artifacts are expanded once. + * + * The constructed list never contains middleman artifacts. It contains tree artifacts only if + * {@code keepEmptyTreeArtifacts} is true. In this case, all added tree artifacts have empty + * expansion under the provided {@link ArtifactExpander}. */ private static void addExpandedArtifacts(Iterable artifacts, - Collection output, - Function outputFormatter, - ArtifactExpander artifactExpander) { + Collection output, + Function outputFormatter, + ArtifactExpander artifactExpander, + boolean keepEmptyTreeArtifacts) { for (Artifact artifact : artifacts) { if (artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()) { - expandArtifact(artifact, output, outputFormatter, artifactExpander); + expandArtifact(artifact, output, outputFormatter, artifactExpander, keepEmptyTreeArtifacts); } else { output.add(outputFormatter.apply(artifact)); } @@ -1549,13 +1562,17 @@ private static void addExpandedArtifacts(Iterable artifa private static void expandArtifact(Artifact middleman, Collection output, Function outputFormatter, - ArtifactExpander artifactExpander) { + ArtifactExpander artifactExpander, + boolean keepEmptyTreeArtifacts) { Preconditions.checkArgument(middleman.isMiddlemanArtifact() || middleman.isTreeArtifact()); List artifacts = new ArrayList<>(); artifactExpander.expand(middleman, artifacts); for (Artifact artifact : artifacts) { output.add(outputFormatter.apply(artifact)); } + if (keepEmptyTreeArtifacts && middleman.isTreeArtifact() && artifacts.isEmpty()) { + output.add(outputFormatter.apply(middleman)); + } } /** 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 44cb0210b92584..8af8fd06bc51f6 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 @@ -130,7 +130,8 @@ void addRunfilesToInputs( if (localArtifact.isTreeArtifact()) { List expandedInputs = ActionInputHelper.expandArtifacts( - NestedSetBuilder.create(Order.STABLE_ORDER, localArtifact), artifactExpander); + NestedSetBuilder.create(Order.STABLE_ORDER, localArtifact), artifactExpander, + /* keepEmptyTreeArtifacts= */ false); for (ActionInput input : expandedInputs) { addMapping( inputMap, @@ -222,7 +223,11 @@ private static void addInputs( NestedSet inputFiles, ArtifactExpander artifactExpander, PathFragment baseDirectory) { - List inputs = ActionInputHelper.expandArtifacts(inputFiles, artifactExpander); + // 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 + // here to signal consumers that they should create the directory. + List inputs = ActionInputHelper.expandArtifacts(inputFiles, artifactExpander, + /* keepEmptyTreeArtifacts= */ true); for (ActionInput input : inputs) { addMapping(inputMap, input.getExecPath(), input, baseDirectory); } @@ -230,8 +235,10 @@ private static void addInputs( /** * Convert the inputs and runfiles of the given spawn to a map from exec-root relative paths to - * {@link ActionInput}s. The returned map does not contain tree artifacts as they are expanded to - * file artifacts. + * {@link ActionInput}s. The returned map does not contain non-empty tree artifacts as they are + * expanded to file artifacts. Tree artifacts that would expand to the empty set under the + * provided {@link ArtifactExpander} are left untouched so that their corresponding empty + * directories can be created. * *

The returned map never contains {@code null} values. * diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index 3f58e71dd23755..885ff46774c44c 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -586,7 +586,6 @@ private static Spawn createCoveragePostProcessingSpawn( .addTransitive(action.getInputs()) .addAll(expandedCoverageDir) .add(action.getCollectCoverageScript()) - .add(action.getCoverageDirectoryTreeArtifact()) .add(action.getCoverageManifest()) .addTransitive(action.getLcovMergerFilesToRun().build()) .build(), diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD b/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD index 5a1a76569c4f9d..bca6b54363c084 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD @@ -20,6 +20,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/remote/util", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/util:string", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java index a29304b1ba40cd..c3d0223151c1cb 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java @@ -17,12 +17,14 @@ import com.google.common.base.Preconditions; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputHelper; +import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.MetadataProvider; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.DirectoryNode; import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.FileNode; import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -33,6 +35,7 @@ import java.util.Map; import java.util.SortedMap; import java.util.TreeMap; +import javax.annotation.Nullable; /** Builder for directory trees. */ class DirectoryTreeBuilder { @@ -94,6 +97,7 @@ private static int buildFromPaths( Map tree) throws IOException { return build( + null, inputs, tree, (input, path, currDir) -> { @@ -122,6 +126,7 @@ private static int buildFromActionInputs( Map tree) throws IOException { return build( + metadataProvider, inputs, tree, (input, path, currDir) -> { @@ -177,6 +182,7 @@ private static int buildFromActionInputs( } private static int build( + @Nullable MetadataProvider metadataProvider, SortedMap inputs, Map tree, FileNodeVisitor fileNodeVisitor) @@ -192,6 +198,30 @@ private static int build( // Path relative to the exec root PathFragment path = e.getKey(); T input = e.getValue(); + + if (input instanceof DerivedArtifact && ((DerivedArtifact) input).isTreeArtifact()) { + DerivedArtifact artifact = (DerivedArtifact) input; + // MetadataProvider is provided by all callers for which T is a superclass of + // DerivedArtifact. + Preconditions.checkNotNull(metadataProvider); + FileArtifactValue metadata = + Preconditions.checkNotNull( + metadataProvider.getMetadata(artifact), + "missing metadata for '%s'", + artifact.getExecPathString()); + Preconditions.checkState(metadata.equals(TreeArtifactValue.empty().getMetadata()), + "Encountered non-empty TreeArtifact '%s' with metadata '%s', which should have" + + " been expanded by SpawnInputExpander. This is a bug.", + path, metadata); + // Create an empty directory and its parent directories but don't visit the TreeArtifact + // input itself: A TreeArtifact's metadata has type REGULAR_FILE, not DIRECTORY, and would + // thus lead to an empty file being created in the buildFromActionInputs visitor. + DirectoryNode emptyDir = new DirectoryNode(path.getBaseName()); + tree.put(path, emptyDir); + createParentDirectoriesIfNotExist(path, emptyDir, tree); + continue; + } + if (dirname == null || !path.getParentDirectory().equals(dirname)) { dirname = path.getParentDirectory(); dir = tree.get(dirname); diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java index 08ce6d6cc7d8d5..82e905c4ed9578 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java @@ -254,7 +254,8 @@ private static MerkleTree build(DirectoryTree tree, DigestUtil digestUtil) { for (DirectoryTree.DirectoryNode dir : dirs) { PathFragment subDirname = dirname.getRelative(dir.getPathSegment()); MerkleTree subMerkleTree = - Preconditions.checkNotNull(m.remove(subDirname), "subMerkleTree was null"); + Preconditions.checkNotNull(m.remove(subDirname), "subMerkleTree at '%s' was null", + subDirname); subDirs.put(dir.getPathSegment(), subMerkleTree); } MerkleTree mt = buildMerkleTree(new TreeSet<>(files), subDirs, digestUtil); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java index d1f5ec24143e56..630a4551485e7f 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java @@ -233,8 +233,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT), - spawn, - context.getArtifactExpander(), execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java index f56eafd8cf9573..1a46186e76c991 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java @@ -223,8 +223,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT), - spawn, - context.getArtifactExpander(), execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java index ffb7a5f3b7fd02..b9fdbf3c69f69a 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java @@ -184,8 +184,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT), - spawn, - context.getArtifactExpander(), execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java index 79b625ea896c0c..6b6835eff2b549 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java @@ -112,8 +112,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs inputs = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT), - spawn, - context.getArtifactExpander(), execRoot); SandboxOutputs outputs = helpers.getOutputs(spawn); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java index ea9f071b89ba3c..0bf2dd42cf8379 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java @@ -27,7 +27,6 @@ import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.actions.cache.VirtualActionInput.EmptyActionInput; @@ -42,10 +41,8 @@ import com.google.devtools.common.options.OptionsParsingResult; import java.io.IOException; import java.io.OutputStream; -import java.util.ArrayList; import java.util.HashSet; import java.util.LinkedHashSet; -import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -487,27 +484,8 @@ private static void writeVirtualInputTo(VirtualActionInput input, Path target) */ public SandboxInputs processInputFiles( Map inputMap, - Spawn spawn, - ArtifactExpander artifactExpander, Path execRoot) throws IOException { - // SpawnInputExpander#getInputMapping uses ArtifactExpander#expandArtifacts to expand - // middlemen and tree artifacts, which expands empty tree artifacts to no entry. However, - // actions that accept TreeArtifacts as inputs generally expect that the empty directory is - // created. So we add those explicitly here. - // TODO(ulfjack): Move this code to SpawnInputExpander. - for (ActionInput input : spawn.getInputFiles().toList()) { - if (input instanceof Artifact && ((Artifact) input).isTreeArtifact()) { - List containedArtifacts = new ArrayList<>(); - artifactExpander.expand((Artifact) input, containedArtifacts); - // Attempting to mount a non-empty directory results in ERR_DIRECTORY_NOT_EMPTY, so we - // only mount empty TreeArtifacts as directories. - if (containedArtifacts.isEmpty()) { - inputMap.put(input.getExecPath(), input); - } - } - } - Map inputFiles = new TreeMap<>(); Set virtualInputs = new HashSet<>(); Map inputSymlinks = new TreeMap<>(); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java index f602fab1adbc8a..3be3a5496bedcd 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java @@ -72,8 +72,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context SandboxInputs readablePaths = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT), - spawn, - context.getArtifactExpander(), execRoot); readablePaths.materializeVirtualInputs(execRoot); diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java index e3afa41371c43c..6a2637b5d71328 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java @@ -64,7 +64,8 @@ public static SortedMap getWorkerFilesWithDigests( TreeMap workerFilesMap = new TreeMap<>(); List tools = - ActionInputHelper.expandArtifacts(spawn.getToolFiles(), artifactExpander); + ActionInputHelper.expandArtifacts(spawn.getToolFiles(), + artifactExpander, /* keepEmptyTreeArtifacts= */ false); for (ActionInput tool : tools) { @Nullable FileArtifactValue metadata = actionInputFileCache.getMetadata(tool); if (metadata == null) { diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java index ec16d6a1ddb14d..3fcc223457d8ff 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java @@ -202,8 +202,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) inputFiles = helpers.processInputFiles( context.getInputMapping(PathFragment.EMPTY_FRAGMENT), - spawn, - context.getArtifactExpander(), execRoot); } SandboxOutputs outputs = helpers.getOutputs(spawn); @@ -259,7 +257,8 @@ private WorkRequest createWorkRequest( } List inputs = - ActionInputHelper.expandArtifacts(spawn.getInputFiles(), context.getArtifactExpander()); + ActionInputHelper.expandArtifacts(spawn.getInputFiles(), context.getArtifactExpander(), + /* keepEmptyTreeArtifacts= */ false); for (ActionInput input : inputs) { byte[] digestBytes = inputFileCache.getMetadata(input).getDigest(); diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java index 61fa76665192c1..9083a1e36f57e6 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxHelpersTest.java @@ -24,14 +24,11 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput; import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; -import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.exec.BinTools; -import com.google.devtools.build.lib.exec.util.SpawnBuilder; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs; import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxOutputs; import com.google.devtools.build.lib.testutil.Scratch; @@ -66,9 +63,6 @@ @RunWith(JUnit4.class) public class SandboxHelpersTest { - private static final ArtifactExpander EMPTY_EXPANDER = (ignored1, ignored2) -> {}; - private static final Spawn SPAWN = new SpawnBuilder().build(); - private final Scratch scratch = new Scratch(); private Path execRoot; @Nullable private ExecutorService executorToCleanup; @@ -99,7 +93,7 @@ public void processInputFiles_materializesParamFile() throws Exception { UTF_8); SandboxInputs inputs = - sandboxHelpers.processInputFiles(inputMap(paramFile), SPAWN, EMPTY_EXPANDER, execRoot); + sandboxHelpers.processInputFiles(inputMap(paramFile), execRoot); assertThat(inputs.getFiles()) .containsExactly(PathFragment.create("paramFile"), execRoot.getChild("paramFile")); @@ -119,7 +113,7 @@ public void processInputFiles_materializesBinToolsFile() throws Exception { PathFragment.create("_bin/say_hello")); SandboxInputs inputs = - sandboxHelpers.processInputFiles(inputMap(tool), SPAWN, EMPTY_EXPANDER, execRoot); + sandboxHelpers.processInputFiles(inputMap(tool), execRoot); assertThat(inputs.getFiles()) .containsExactly( @@ -143,7 +137,7 @@ public void processInputFiles_delayVirtualInputMaterialization_doesNotStoreVirtu UTF_8); SandboxInputs inputs = - sandboxHelpers.processInputFiles(inputMap(paramFile), SPAWN, EMPTY_EXPANDER, execRoot); + sandboxHelpers.processInputFiles(inputMap(paramFile), execRoot); assertThat(inputs.getFiles()).isEmpty(); assertThat(inputs.getSymlinks()).isEmpty(); @@ -165,7 +159,7 @@ public void sandboxInputMaterializeVirtualInputs_delayMaterialization_writesCorr scratch.file("tool", "tool_code"), PathFragment.create("tools/tool")); SandboxInputs inputs = sandboxHelpers.processInputFiles( - inputMap(paramFile, tool), SPAWN, EMPTY_EXPANDER, execRoot); + inputMap(paramFile, tool), execRoot); inputs.materializeVirtualInputs(scratch.dir("/sandbox")); @@ -213,13 +207,13 @@ protected void setExecutable(PathFragment path, boolean executable) throws IOExc () -> { try { sandboxHelpers.processInputFiles( - inputMap(input), SPAWN, EMPTY_EXPANDER, customExecRoot); + inputMap(input), customExecRoot); finishProcessingSemaphore.release(); } catch (IOException e) { throw new IllegalArgumentException(e); } }); - sandboxHelpers.processInputFiles(inputMap(input), SPAWN, EMPTY_EXPANDER, customExecRoot); + sandboxHelpers.processInputFiles(inputMap(input), customExecRoot); finishProcessingSemaphore.release(); future.get(); diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh index 3e421533d977df..41541fb2e626b0 100755 --- a/src/test/shell/bazel/bazel_sandboxing_test.sh +++ b/src/test/shell/bazel/bazel_sandboxing_test.sh @@ -794,7 +794,7 @@ EOF } # regression test for https://github.com/bazelbuild/bazel/issues/6262 -function test_create_tree_artifact_inputs() { +function test_create_tree_artifact_outputs() { create_workspace_with_default_repos WORKSPACE cat > def.bzl <<'EOF' @@ -818,6 +818,38 @@ EOF bazel build --test_output=streamed :a &>$TEST_log || fail "expected build to succeed" } +function test_create_tree_artifact_inputs() { + create_workspace_with_default_repos WORKSPACE + + mkdir -p pkg + + cat > pkg/def.bzl <<'EOF' +def _r(ctx): + empty_d = ctx.actions.declare_directory("%s/empty_dir" % ctx.label.name) + ctx.actions.run_shell( + outputs = [empty_d], + command = "mkdir -p %s" % empty_d.path, + ) + f = ctx.actions.declare_file("%s/file" % ctx.label.name) + ctx.actions.run_shell( + inputs = [empty_d], + outputs = [f], + command = "touch %s && cd %s && pwd" % (f.path, empty_d.path), + ) + return [DefaultInfo(files = depset([f]))] + +r = rule(implementation = _r) +EOF + +cat > pkg/BUILD <<'EOF' +load(":def.bzl", "r") + +r(name = "a") +EOF + + bazel build //pkg:a &>$TEST_log || fail "expected build to succeed" +} + # The test shouldn't fail if the environment doesn't support running it. check_sandbox_allowed || exit 0 diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index b9ccff357fa831..5a8a1c17b85f1f 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3076,6 +3076,54 @@ end_of_record" assert_equals "$expected_result" "$(cat bazel-testlogs/java/factorial/fact-test/coverage.dat)" } +function test_create_tree_artifact_inputs() { + touch WORKSPACE + mkdir -p pkg + + cat > pkg/def.bzl <<'EOF' +def _r(ctx): + empty_d = ctx.actions.declare_directory("%s/empty_dir" % ctx.label.name) + ctx.actions.run_shell( + outputs = [empty_d], + command = "mkdir -p %s" % empty_d.path, + ) + f = ctx.actions.declare_file("%s/file" % ctx.label.name) + ctx.actions.run_shell( + inputs = [empty_d], + outputs = [f], + command = "touch %s && cd %s && pwd" % (f.path, empty_d.path), + ) + return [DefaultInfo(files = depset([f]))] + +r = rule(implementation = _r) +EOF + +cat > pkg/BUILD <<'EOF' +load(":def.bzl", "r") + +r(name = "a") +EOF + + bazel build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + //pkg:a &>$TEST_log || fail "expected build to succeed" + + bazel clean --expunge + bazel build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_remote_merkle_tree_cache \ + //pkg:a &>$TEST_log || fail "expected build to succeed with Merkle tree cache" + + bazel clean --expunge + bazel build \ + --spawn_strategy=remote \ + --remote_executor=grpc://localhost:${worker_port} \ + --experimental_sibling_repository_layout \ + //pkg:a &>$TEST_log || fail "expected build to succeed with sibling repository layout" +} + # Runs coverage with `cc_test` and RE then checks the coverage file is returned. # Older versions of gcov are not supported with bazel coverage and so will be skipped. # See the above `test_java_rbe_coverage_produces_report` for more information.