Skip to content

Commit 26f8783

Browse files
ckolli5fmeum
authored and
meteorcloudy
committed
Unify sandbox/remote handling of empty TreeArtifact inputs (bazelbuild#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 bazelbuild#15276. PiperOrigin-RevId: 446452587 Co-authored-by: Fabian Meumertzheim <[email protected]>
1 parent 027e9b2 commit 26f8783

18 files changed

+212
-135
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -103,12 +103,19 @@ public PathFragment getExecPath() {
103103
}
104104

105105
/**
106-
* Expands middleman artifacts in a sequence of {@link ActionInput}s.
106+
* Expands middleman and tree artifacts in a sequence of {@link ActionInput}s.
107107
*
108-
* <p>Non-middleman artifacts are returned untouched.
108+
* <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
109+
* is true, a tree artifact will be included in the constructed list when it expands into zero
110+
* file artifacts. Otherwise, only the file artifacts the tree artifact expands into will be
111+
* included.
112+
*
113+
* <p>Non-middleman, non-tree artifacts are returned untouched.
109114
*/
110115
public static List<ActionInput> expandArtifacts(
111-
NestedSet<? extends ActionInput> inputs, ArtifactExpander artifactExpander) {
116+
NestedSet<? extends ActionInput> inputs,
117+
ArtifactExpander artifactExpander,
118+
boolean keepEmptyTreeArtifacts) {
112119
List<ActionInput> result = new ArrayList<>();
113120
List<Artifact> containedArtifacts = new ArrayList<>();
114121
for (ActionInput input : inputs.toList()) {
@@ -118,7 +125,8 @@ public static List<ActionInput> expandArtifacts(
118125
}
119126
containedArtifacts.add((Artifact) input);
120127
}
121-
Artifact.addExpandedArtifacts(containedArtifacts, result, artifactExpander);
128+
Artifact.addExpandedArtifacts(
129+
containedArtifacts, result, artifactExpander, keepEmptyTreeArtifacts);
122130
return result;
123131
}
124132

‎src/main/java/com/google/devtools/build/lib/actions/Artifact.java

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1566,42 +1566,63 @@ public static String joinRootRelativePaths(String delimiter, Iterable<Artifact>
15661566
return Joiner.on(delimiter).join(toRootRelativePaths(artifacts));
15671567
}
15681568

1569-
/** Adds a collection of artifacts to a given collection, with middleman actions expanded once. */
1569+
/**
1570+
* Adds a collection of artifacts to a given collection, with middleman actions and tree artifacts
1571+
* expanded once.
1572+
*
1573+
* <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
1574+
* is true, a tree artifact will be included in the constructed list when it expands into zero
1575+
* file artifacts. Otherwise, only the file artifacts the tree artifact expands into will be
1576+
* included.
1577+
*/
15701578
static void addExpandedArtifacts(
15711579
Iterable<Artifact> artifacts,
15721580
Collection<? super Artifact> output,
1573-
ArtifactExpander artifactExpander) {
1574-
addExpandedArtifacts(artifacts, output, Functions.identity(), artifactExpander);
1581+
ArtifactExpander artifactExpander,
1582+
boolean keepEmptyTreeArtifacts) {
1583+
addExpandedArtifacts(
1584+
artifacts, output, Functions.identity(), artifactExpander, keepEmptyTreeArtifacts);
15751585
}
15761586

15771587
/**
1578-
* Converts a collection of artifacts into the outputs computed by
1579-
* outputFormatter and adds them to a given collection. Middleman artifacts
1580-
* are expanded once.
1588+
* Converts a collection of artifacts into the outputs computed by outputFormatter and adds them
1589+
* to a given collection. Middleman artifacts and tree artifacts are expanded once.
1590+
*
1591+
* <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
1592+
* is true, a tree artifact will be included in the constructed list when it expands into zero
1593+
* file artifacts. Otherwise, only the file artifacts the tree artifact expands into will be
1594+
* included.
15811595
*/
1582-
private static <E> void addExpandedArtifacts(Iterable<? extends Artifact> artifacts,
1583-
Collection<? super E> output,
1584-
Function<? super Artifact, E> outputFormatter,
1585-
ArtifactExpander artifactExpander) {
1596+
private static <E> void addExpandedArtifacts(
1597+
Iterable<? extends Artifact> artifacts,
1598+
Collection<? super E> output,
1599+
Function<? super Artifact, E> outputFormatter,
1600+
ArtifactExpander artifactExpander,
1601+
boolean keepEmptyTreeArtifacts) {
15861602
for (Artifact artifact : artifacts) {
15871603
if (artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()) {
1588-
expandArtifact(artifact, output, outputFormatter, artifactExpander);
1604+
expandArtifact(artifact, output, outputFormatter, artifactExpander, keepEmptyTreeArtifacts);
15891605
} else {
15901606
output.add(outputFormatter.apply(artifact));
15911607
}
15921608
}
15931609
}
15941610

1595-
private static <E> void expandArtifact(Artifact middleman,
1611+
private static <E> void expandArtifact(
1612+
Artifact middleman,
15961613
Collection<? super E> output,
15971614
Function<? super Artifact, E> outputFormatter,
1598-
ArtifactExpander artifactExpander) {
1615+
ArtifactExpander artifactExpander,
1616+
boolean keepEmptyTreeArtifacts) {
15991617
Preconditions.checkArgument(middleman.isMiddlemanArtifact() || middleman.isTreeArtifact());
16001618
List<Artifact> artifacts = new ArrayList<>();
16011619
artifactExpander.expand(middleman, artifacts);
16021620
for (Artifact artifact : artifacts) {
16031621
output.add(outputFormatter.apply(artifact));
16041622
}
1623+
if (keepEmptyTreeArtifacts && middleman.isTreeArtifact() && artifacts.isEmpty()) {
1624+
output.add(outputFormatter.apply(middleman));
1625+
}
16051626
}
16061627

16071628
/**

‎src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,9 @@ void addRunfilesToInputs(
130130
if (localArtifact.isTreeArtifact()) {
131131
List<ActionInput> expandedInputs =
132132
ActionInputHelper.expandArtifacts(
133-
NestedSetBuilder.create(Order.STABLE_ORDER, localArtifact), artifactExpander);
133+
NestedSetBuilder.create(Order.STABLE_ORDER, localArtifact),
134+
artifactExpander,
135+
/* keepEmptyTreeArtifacts= */ false);
134136
for (ActionInput input : expandedInputs) {
135137
addMapping(
136138
inputMap,
@@ -222,16 +224,23 @@ private static void addInputs(
222224
NestedSet<? extends ActionInput> inputFiles,
223225
ArtifactExpander artifactExpander,
224226
PathFragment baseDirectory) {
225-
List<ActionInput> inputs = ActionInputHelper.expandArtifacts(inputFiles, artifactExpander);
227+
// Actions that accept TreeArtifacts as inputs generally expect the directory corresponding
228+
// to the artifact to be created, even if it is empty. We explicitly keep empty TreeArtifacts
229+
// here to signal consumers that they should create the directory.
230+
List<ActionInput> inputs =
231+
ActionInputHelper.expandArtifacts(
232+
inputFiles, artifactExpander, /* keepEmptyTreeArtifacts= */ true);
226233
for (ActionInput input : inputs) {
227234
addMapping(inputMap, input.getExecPath(), input, baseDirectory);
228235
}
229236
}
230237

231238
/**
232239
* Convert the inputs and runfiles of the given spawn to a map from exec-root relative paths to
233-
* {@link ActionInput}s. The returned map does not contain tree artifacts as they are expanded to
234-
* file artifacts.
240+
* {@link ActionInput}s. The returned map does not contain non-empty tree artifacts as they are
241+
* expanded to file artifacts. Tree artifacts that would expand to the empty set under the
242+
* provided {@link ArtifactExpander} are left untouched so that their corresponding empty
243+
* directories can be created.
235244
*
236245
* <p>The returned map never contains {@code null} values.
237246
*

‎src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -591,7 +591,6 @@ private static Spawn createCoveragePostProcessingSpawn(
591591
.addTransitive(action.getInputs())
592592
.addAll(expandedCoverageDir)
593593
.add(action.getCollectCoverageScript())
594-
.add(action.getCoverageDirectoryTreeArtifact())
595594
.add(action.getCoverageManifest())
596595
.addTransitive(action.getLcovMergerFilesToRun().build())
597596
.build(),

‎src/main/java/com/google/devtools/build/lib/remote/merkletree/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ java_library(
2020
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
2121
"//src/main/java/com/google/devtools/build/lib/profiler",
2222
"//src/main/java/com/google/devtools/build/lib/remote/util",
23+
"//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
2324
"//src/main/java/com/google/devtools/build/lib/vfs",
2425
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
2526
"//third_party:guava",

‎src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,14 @@
1717
import com.google.common.base.Preconditions;
1818
import com.google.devtools.build.lib.actions.ActionInput;
1919
import com.google.devtools.build.lib.actions.ActionInputHelper;
20+
import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
2021
import com.google.devtools.build.lib.actions.FileArtifactValue;
2122
import com.google.devtools.build.lib.actions.MetadataProvider;
2223
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
2324
import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.DirectoryNode;
2425
import com.google.devtools.build.lib.remote.merkletree.DirectoryTree.FileNode;
2526
import com.google.devtools.build.lib.remote.util.DigestUtil;
27+
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
2628
import com.google.devtools.build.lib.vfs.Dirent;
2729
import com.google.devtools.build.lib.vfs.Path;
2830
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -33,6 +35,7 @@
3335
import java.util.Map;
3436
import java.util.SortedMap;
3537
import java.util.TreeMap;
38+
import javax.annotation.Nullable;
3639

3740
/** Builder for directory trees. */
3841
class DirectoryTreeBuilder {
@@ -94,6 +97,7 @@ private static int buildFromPaths(
9497
Map<PathFragment, DirectoryNode> tree)
9598
throws IOException {
9699
return build(
100+
null,
97101
inputs,
98102
tree,
99103
(input, path, currDir) -> {
@@ -122,6 +126,7 @@ private static int buildFromActionInputs(
122126
Map<PathFragment, DirectoryNode> tree)
123127
throws IOException {
124128
return build(
129+
metadataProvider,
125130
inputs,
126131
tree,
127132
(input, path, currDir) -> {
@@ -177,6 +182,7 @@ private static int buildFromActionInputs(
177182
}
178183

179184
private static <T> int build(
185+
@Nullable MetadataProvider metadataProvider,
180186
SortedMap<PathFragment, T> inputs,
181187
Map<PathFragment, DirectoryNode> tree,
182188
FileNodeVisitor<T> fileNodeVisitor)
@@ -192,6 +198,32 @@ private static <T> int build(
192198
// Path relative to the exec root
193199
PathFragment path = e.getKey();
194200
T input = e.getValue();
201+
202+
if (input instanceof DerivedArtifact && ((DerivedArtifact) input).isTreeArtifact()) {
203+
DerivedArtifact artifact = (DerivedArtifact) input;
204+
// MetadataProvider is provided by all callers for which T is a superclass of
205+
// DerivedArtifact.
206+
Preconditions.checkNotNull(metadataProvider);
207+
FileArtifactValue metadata =
208+
Preconditions.checkNotNull(
209+
metadataProvider.getMetadata(artifact),
210+
"missing metadata for '%s'",
211+
artifact.getExecPathString());
212+
Preconditions.checkState(
213+
metadata.equals(TreeArtifactValue.empty().getMetadata()),
214+
"Encountered non-empty TreeArtifact '%s' with metadata '%s', which should have"
215+
+ " been expanded by SpawnInputExpander. This is a bug.",
216+
path,
217+
metadata);
218+
// Create an empty directory and its parent directories but don't visit the TreeArtifact
219+
// input itself: A TreeArtifact's metadata has type REGULAR_FILE, not DIRECTORY, and would
220+
// thus lead to an empty file being created in the buildFromActionInputs visitor.
221+
DirectoryNode emptyDir = new DirectoryNode(path.getBaseName());
222+
tree.put(path, emptyDir);
223+
createParentDirectoriesIfNotExist(path, emptyDir, tree);
224+
continue;
225+
}
226+
195227
if (dirname == null || !path.getParentDirectory().equals(dirname)) {
196228
dirname = path.getParentDirectory();
197229
dir = tree.get(dirname);

‎src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,8 @@ private static MerkleTree build(DirectoryTree tree, DigestUtil digestUtil) {
252252
for (DirectoryTree.DirectoryNode dir : dirs) {
253253
PathFragment subDirname = dirname.getRelative(dir.getPathSegment());
254254
MerkleTree subMerkleTree =
255-
Preconditions.checkNotNull(m.remove(subDirname), "subMerkleTree was null");
255+
Preconditions.checkNotNull(
256+
m.remove(subDirname), "subMerkleTree at '%s' was null", subDirname);
256257
subDirs.put(dir.getPathSegment(), subMerkleTree);
257258
}
258259
MerkleTree mt = buildMerkleTree(new TreeSet<>(files), subDirs, digestUtil);

‎src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,8 +233,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
233233
SandboxInputs inputs =
234234
helpers.processInputFiles(
235235
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
236-
spawn,
237-
context.getArtifactExpander(),
238236
execRoot);
239237
SandboxOutputs outputs = helpers.getOutputs(spawn);
240238

‎src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -223,8 +223,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
223223
SandboxInputs inputs =
224224
helpers.processInputFiles(
225225
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
226-
spawn,
227-
context.getArtifactExpander(),
228226
execRoot);
229227
SandboxOutputs outputs = helpers.getOutputs(spawn);
230228

‎src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
184184
SandboxInputs inputs =
185185
helpers.processInputFiles(
186186
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
187-
spawn,
188-
context.getArtifactExpander(),
189187
execRoot);
190188
SandboxOutputs outputs = helpers.getOutputs(spawn);
191189

‎src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,8 +112,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
112112
SandboxInputs inputs =
113113
helpers.processInputFiles(
114114
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
115-
spawn,
116-
context.getArtifactExpander(),
117115
execRoot);
118116
SandboxOutputs outputs = helpers.getOutputs(spawn);
119117

‎src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import com.google.common.flogger.GoogleLogger;
2626
import com.google.devtools.build.lib.actions.ActionInput;
2727
import com.google.devtools.build.lib.actions.Artifact;
28-
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
2928
import com.google.devtools.build.lib.actions.Spawn;
3029
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
3130
import com.google.devtools.build.lib.actions.cache.VirtualActionInput.EmptyActionInput;
@@ -40,10 +39,8 @@
4039
import com.google.devtools.common.options.OptionsParsingResult;
4140
import java.io.IOException;
4241
import java.io.OutputStream;
43-
import java.util.ArrayList;
4442
import java.util.HashSet;
4543
import java.util.LinkedHashSet;
46-
import java.util.List;
4744
import java.util.Map;
4845
import java.util.Optional;
4946
import java.util.Set;
@@ -429,27 +426,8 @@ private static void writeVirtualInputTo(VirtualActionInput input, Path target)
429426
*/
430427
public SandboxInputs processInputFiles(
431428
Map<PathFragment, ActionInput> inputMap,
432-
Spawn spawn,
433-
ArtifactExpander artifactExpander,
434429
Path execRoot)
435430
throws IOException {
436-
// SpawnInputExpander#getInputMapping uses ArtifactExpander#expandArtifacts to expand
437-
// middlemen and tree artifacts, which expands empty tree artifacts to no entry. However,
438-
// actions that accept TreeArtifacts as inputs generally expect that the empty directory is
439-
// created. So we add those explicitly here.
440-
// TODO(ulfjack): Move this code to SpawnInputExpander.
441-
for (ActionInput input : spawn.getInputFiles().toList()) {
442-
if (input instanceof Artifact && ((Artifact) input).isTreeArtifact()) {
443-
List<Artifact> containedArtifacts = new ArrayList<>();
444-
artifactExpander.expand((Artifact) input, containedArtifacts);
445-
// Attempting to mount a non-empty directory results in ERR_DIRECTORY_NOT_EMPTY, so we
446-
// only mount empty TreeArtifacts as directories.
447-
if (containedArtifacts.isEmpty()) {
448-
inputMap.put(input.getExecPath(), input);
449-
}
450-
}
451-
}
452-
453431
Map<PathFragment, Path> inputFiles = new TreeMap<>();
454432
Set<VirtualActionInput> virtualInputs = new HashSet<>();
455433
Map<PathFragment, PathFragment> inputSymlinks = new TreeMap<>();

‎src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,6 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
7272
SandboxInputs readablePaths =
7373
helpers.processInputFiles(
7474
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
75-
spawn,
76-
context.getArtifactExpander(),
7775
execRoot);
7876

7977
readablePaths.materializeVirtualInputs(execRoot);

‎src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,8 @@ static SortedMap<PathFragment, HashCode> getWorkerFilesWithHashes(
5858
TreeMap<PathFragment, HashCode> workerFilesMap = new TreeMap<>();
5959

6060
List<ActionInput> tools =
61-
ActionInputHelper.expandArtifacts(spawn.getToolFiles(), artifactExpander);
61+
ActionInputHelper.expandArtifacts(
62+
spawn.getToolFiles(), artifactExpander, /* keepEmptyTreeArtifacts= */ false);
6263
for (ActionInput tool : tools) {
6364
workerFilesMap.put(
6465
tool.getExecPath(),

‎src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -192,8 +192,6 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
192192
inputFiles =
193193
helpers.processInputFiles(
194194
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
195-
spawn,
196-
context.getArtifactExpander(),
197195
execRoot);
198196
}
199197
SandboxOutputs outputs = helpers.getOutputs(spawn);
@@ -250,7 +248,10 @@ private WorkRequest createWorkRequest(
250248
}
251249

252250
List<ActionInput> inputs =
253-
ActionInputHelper.expandArtifacts(spawn.getInputFiles(), context.getArtifactExpander());
251+
ActionInputHelper.expandArtifacts(
252+
spawn.getInputFiles(),
253+
context.getArtifactExpander(),
254+
/* keepEmptyTreeArtifacts= */ false);
254255

255256
for (ActionInput input : inputs) {
256257
byte[] digestBytes = inputFileCache.getMetadata(input).getDigest();

0 commit comments

Comments
 (0)