Unify sandbox/remote handling of empty TreeArtifact inputs (#15449)
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.
Closes #15276.
PiperOrigin-RevId: 446452587
Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
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 ea5f758..bc3d15c 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,19 @@
}
/**
- * Expands middleman artifacts in a sequence of {@link ActionInput}s.
+ * Expands middleman and tree artifacts in a sequence of {@link ActionInput}s.
*
- * <p>Non-middleman artifacts are returned untouched.
+ * <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
+ * is true, a tree artifact will be included in the constructed list when it expands into zero
+ * file artifacts. Otherwise, only the file artifacts the tree artifact expands into will be
+ * included.
+ *
+ * <p>Non-middleman, non-tree artifacts are returned untouched.
*/
public static List<ActionInput> expandArtifacts(
- NestedSet<? extends ActionInput> inputs, ArtifactExpander artifactExpander) {
+ NestedSet<? extends ActionInput> inputs,
+ ArtifactExpander artifactExpander,
+ boolean keepEmptyTreeArtifacts) {
List<ActionInput> result = new ArrayList<>();
List<Artifact> containedArtifacts = new ArrayList<>();
for (ActionInput input : inputs.toList()) {
@@ -118,7 +125,8 @@
}
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 cb0afd2..3082064 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
@@ -1566,42 +1566,63 @@
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.
+ *
+ * <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
+ * is true, a tree artifact will be included in the constructed list when it expands into zero
+ * file artifacts. Otherwise, only the file artifacts the tree artifact expands into will be
+ * included.
+ */
static void addExpandedArtifacts(
Iterable<Artifact> artifacts,
Collection<? super Artifact> 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.
+ *
+ * <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
+ * is true, a tree artifact will be included in the constructed list when it expands into zero
+ * file artifacts. Otherwise, only the file artifacts the tree artifact expands into will be
+ * included.
*/
- private static <E> void addExpandedArtifacts(Iterable<? extends Artifact> artifacts,
- Collection<? super E> output,
- Function<? super Artifact, E> outputFormatter,
- ArtifactExpander artifactExpander) {
+ private static <E> void addExpandedArtifacts(
+ Iterable<? extends Artifact> artifacts,
+ Collection<? super E> output,
+ Function<? super Artifact, E> 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));
}
}
}
- private static <E> void expandArtifact(Artifact middleman,
+ private static <E> void expandArtifact(
+ Artifact middleman,
Collection<? super E> output,
Function<? super Artifact, E> outputFormatter,
- ArtifactExpander artifactExpander) {
+ ArtifactExpander artifactExpander,
+ boolean keepEmptyTreeArtifacts) {
Preconditions.checkArgument(middleman.isMiddlemanArtifact() || middleman.isTreeArtifact());
List<Artifact> 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 44cb021..944892e 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,9 @@
if (localArtifact.isTreeArtifact()) {
List<ActionInput> 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 +224,12 @@
NestedSet<? extends ActionInput> inputFiles,
ArtifactExpander artifactExpander,
PathFragment baseDirectory) {
- List<ActionInput> 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<ActionInput> inputs =
+ ActionInputHelper.expandArtifacts(
+ inputFiles, artifactExpander, /* keepEmptyTreeArtifacts= */ true);
for (ActionInput input : inputs) {
addMapping(inputMap, input.getExecPath(), input, baseDirectory);
}
@@ -230,8 +237,10 @@
/**
* 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.
*
* <p>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 ef99dae..24405a4 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
@@ -591,7 +591,6 @@
.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 46283f2..b7831a8 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 @@
"//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/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//third_party:guava",
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 a29304b..c641f80 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 @@
Map<PathFragment, DirectoryNode> tree)
throws IOException {
return build(
+ null,
inputs,
tree,
(input, path, currDir) -> {
@@ -122,6 +126,7 @@
Map<PathFragment, DirectoryNode> tree)
throws IOException {
return build(
+ metadataProvider,
inputs,
tree,
(input, path, currDir) -> {
@@ -177,6 +182,7 @@
}
private static <T> int build(
+ @Nullable MetadataProvider metadataProvider,
SortedMap<PathFragment, T> inputs,
Map<PathFragment, DirectoryNode> tree,
FileNodeVisitor<T> fileNodeVisitor)
@@ -192,6 +198,32 @@
// 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 f519402..97df073 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
@@ -252,7 +252,8 @@
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 d1f5ec2..630a455 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 @@
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 f56eafd..1a46186 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 @@
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 ffb7a5f..b9fdbf3 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 @@
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 79b625e..6b6835e 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 @@
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 4d088b0..26335ec 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
@@ -25,7 +25,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;
@@ -40,10 +39,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;
@@ -429,27 +426,8 @@
*/
public SandboxInputs processInputFiles(
Map<PathFragment, ActionInput> 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<Artifact> 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<PathFragment, Path> inputFiles = new TreeMap<>();
Set<VirtualActionInput> virtualInputs = new HashSet<>();
Map<PathFragment, PathFragment> 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 f602fab..3be3a54 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 @@
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 f0246c3..c01a7ea 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
@@ -58,7 +58,8 @@
TreeMap<PathFragment, HashCode> workerFilesMap = new TreeMap<>();
List<ActionInput> tools =
- ActionInputHelper.expandArtifacts(spawn.getToolFiles(), artifactExpander);
+ ActionInputHelper.expandArtifacts(
+ spawn.getToolFiles(), artifactExpander, /* keepEmptyTreeArtifacts= */ false);
for (ActionInput tool : tools) {
workerFilesMap.put(
tool.getExecPath(),
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 6eb0b73..a5466f6 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
@@ -192,8 +192,6 @@
inputFiles =
helpers.processInputFiles(
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
- spawn,
- context.getArtifactExpander(),
execRoot);
}
SandboxOutputs outputs = helpers.getOutputs(spawn);
@@ -250,7 +248,10 @@
}
List<ActionInput> 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 61fa766..f8ed2c3 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;
@@ -98,8 +92,7 @@
ParameterFileType.UNQUOTED,
UTF_8);
- SandboxInputs inputs =
- sandboxHelpers.processInputFiles(inputMap(paramFile), SPAWN, EMPTY_EXPANDER, execRoot);
+ SandboxInputs inputs = sandboxHelpers.processInputFiles(inputMap(paramFile), execRoot);
assertThat(inputs.getFiles())
.containsExactly(PathFragment.create("paramFile"), execRoot.getChild("paramFile"));
@@ -118,8 +111,7 @@
scratch.file("tool", "#!/bin/bash", "echo hello"),
PathFragment.create("_bin/say_hello"));
- SandboxInputs inputs =
- sandboxHelpers.processInputFiles(inputMap(tool), SPAWN, EMPTY_EXPANDER, execRoot);
+ SandboxInputs inputs = sandboxHelpers.processInputFiles(inputMap(tool), execRoot);
assertThat(inputs.getFiles())
.containsExactly(
@@ -142,8 +134,7 @@
ParameterFileType.UNQUOTED,
UTF_8);
- SandboxInputs inputs =
- sandboxHelpers.processInputFiles(inputMap(paramFile), SPAWN, EMPTY_EXPANDER, execRoot);
+ SandboxInputs inputs = sandboxHelpers.processInputFiles(inputMap(paramFile), execRoot);
assertThat(inputs.getFiles()).isEmpty();
assertThat(inputs.getSymlinks()).isEmpty();
@@ -163,9 +154,7 @@
BinTools.PathActionInput tool =
new BinTools.PathActionInput(
scratch.file("tool", "tool_code"), PathFragment.create("tools/tool"));
- SandboxInputs inputs =
- sandboxHelpers.processInputFiles(
- inputMap(paramFile, tool), SPAWN, EMPTY_EXPANDER, execRoot);
+ SandboxInputs inputs = sandboxHelpers.processInputFiles(inputMap(paramFile, tool), execRoot);
inputs.materializeVirtualInputs(scratch.dir("/sandbox"));
@@ -212,14 +201,13 @@
executorToCleanup.submit(
() -> {
try {
- sandboxHelpers.processInputFiles(
- inputMap(input), SPAWN, EMPTY_EXPANDER, customExecRoot);
+ sandboxHelpers.processInputFiles(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 3e42153..a11e712 100755
--- a/src/test/shell/bazel/bazel_sandboxing_test.sh
+++ b/src/test/shell/bazel/bazel_sandboxing_test.sh
@@ -794,7 +794,7 @@
}
# 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,40 @@
bazel build --test_output=streamed :a &>$TEST_log || fail "expected build to succeed"
}
+function test_empty_tree_artifact_as_inputs() {
+ # Test that when an empty tree artifact is the input, an empty directory is
+ # created in the sandbox for action to read.
+ 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 d84afa5..8731a7b 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -966,63 +966,6 @@
|| fail "Failed to run //a:starlark_output_dir_test with remote execution"
}
-function generate_empty_treeartifact_build() {
- mkdir -p a
- cat > a/BUILD <<'EOF'
-load(":output_dir.bzl", "gen_output_dir")
-gen_output_dir(
- name = "output-dir",
- outdir = "dir",
-)
-EOF
- cat > a/output_dir.bzl <<'EOF'
-def _gen_output_dir_impl(ctx):
- output_dir = ctx.actions.declare_directory(ctx.attr.outdir)
- ctx.actions.run_shell(
- outputs = [output_dir],
- inputs = [],
- command = "",
- arguments = [output_dir.path],
- )
- return [
- DefaultInfo(files = depset(direct = [output_dir])),
- ]
-
-gen_output_dir = rule(
- implementation = _gen_output_dir_impl,
- attrs = {
- "outdir": attr.string(mandatory = True),
- },
-)
-EOF
-}
-
-function test_empty_treeartifact_works_with_remote_execution() {
- # Test that empty tree artifact works with remote execution
- generate_empty_treeartifact_build
-
- bazel build \
- --remote_executor=grpc://localhost:${worker_port} \
- //a:output-dir >& $TEST_log || fail "Failed to build"
-}
-
-function test_empty_treeartifact_works_with_remote_cache() {
- # Test that empty tree artifact works with remote cache
- generate_empty_treeartifact_build
-
- bazel build \
- --remote_cache=grpc://localhost:${worker_port} \
- //a:output-dir >& $TEST_log || fail "Failed to build"
-
- bazel clean
-
- bazel build \
- --remote_cache=grpc://localhost:${worker_port} \
- //a:output-dir >& $TEST_log || fail "Failed to build"
-
- expect_log "remote cache hit"
-}
-
function test_downloads_minimal() {
# Test that genrule outputs are not downloaded when using
# --remote_download_minimal
@@ -2997,6 +2940,77 @@
assert_equals "$expected_result" "$(cat bazel-testlogs/java/factorial/fact-test/coverage.dat)"
}
+function generate_empty_tree_artifact_as_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
+}
+
+function test_empty_tree_artifact_as_inputs() {
+ # Test that when an empty tree artifact is the input, an empty directory is
+ # created in the remote executor for action to read.
+ generate_empty_tree_artifact_as_inputs
+
+ 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"
+}
+
+function test_empty_tree_artifact_as_inputs_remote_cache() {
+ # Test that when empty tree artifact works for remote cache.
+ generate_empty_tree_artifact_as_inputs
+
+ bazel build \
+ --remote_cache=grpc://localhost:${worker_port} \
+ //pkg:a &>$TEST_log || fail "expected build to succeed"
+
+ bazel clean
+
+ bazel build \
+ --remote_cache=grpc://localhost:${worker_port} \
+ //pkg:a &>$TEST_log || fail "expected build to succeed"
+
+ expect_log "remote cache hit"
+}
+
# 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.
