Skip to content

Commit a151116

Browse files
coeuvrecopybara-github
coeuvre
authored and
copybara-github
committed
Remote: Check declared outputs when downloading outputs.
An AC entry that misses declared outputs of an action is invalid because, if Bazel accepts this from remote cache, it will detect the missing declared outputs error at a later stage and fail the build. This PR adds a check for mandatory outputs when downloading outputs from remote cache. If a mandatory output is missing from AC entry, Bazel will ignore the cache entry so build can continue. Also fixes an issue introduced by bazelbuild#15016 that tests result are not uploaded to remote cache. Test spawns declare all the possible outputs but they usually don't generate them all. This change fixes that by only check for mandatory outputs via `spawn.isMandatoryOutput()`. Fixes bazelbuild#14543. Closes bazelbuild#15051. PiperOrigin-RevId: 435307260
1 parent 9515e9b commit a151116

5 files changed

Lines changed: 81 additions & 15 deletions

File tree

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

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1011,6 +1011,23 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
10111011
metadata = parseActionResultMetadata(action, result);
10121012
}
10131013

1014+
// Check that all mandatory outputs are created.
1015+
for (ActionInput output : action.spawn.getOutputFiles()) {
1016+
if (action.spawn.isMandatoryOutput(output)) {
1017+
Path localPath = execRoot.getRelative(output.getExecPath());
1018+
if (!metadata.files.containsKey(localPath)
1019+
&& !metadata.directories.containsKey(localPath)
1020+
&& !metadata.symlinks.containsKey(localPath)) {
1021+
throw new IOException(
1022+
"Invalid action cache entry "
1023+
+ action.actionKey.getDigest().getHash()
1024+
+ ": expected output "
1025+
+ prettyPrint(output)
1026+
+ " does not exist.");
1027+
}
1028+
}
1029+
}
1030+
10141031
FileOutErr outErr = action.spawnExecutionContext.getFileOutErr();
10151032

10161033
ImmutableList.Builder<ListenableFuture<FileMetadata>> downloadsBuilder =
@@ -1124,23 +1141,27 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re
11241141
return null;
11251142
}
11261143

1144+
private static String prettyPrint(ActionInput actionInput) {
1145+
if (actionInput instanceof Artifact) {
1146+
return ((Artifact) actionInput).prettyPrint();
1147+
} else {
1148+
return actionInput.getExecPathString();
1149+
}
1150+
}
1151+
11271152
private Single<UploadManifest> buildUploadManifestAsync(
11281153
RemoteAction action, SpawnResult spawnResult) {
11291154
return Single.fromCallable(
11301155
() -> {
11311156
ImmutableList.Builder<Path> outputFiles = ImmutableList.builder();
1157+
// Check that all mandatory outputs are created.
11321158
for (ActionInput outputFile : action.spawn.getOutputFiles()) {
1133-
Path outputPath = execRoot.getRelative(outputFile.getExecPath());
1134-
if (!outputPath.exists()) {
1135-
String output;
1136-
if (outputFile instanceof Artifact) {
1137-
output = ((Artifact) outputFile).prettyPrint();
1138-
} else {
1139-
output = outputFile.getExecPathString();
1140-
}
1141-
throw new IOException("Expected output " + output + " was not created locally.");
1159+
Path localPath = execRoot.getRelative(outputFile.getExecPath());
1160+
if (action.spawn.isMandatoryOutput(outputFile) && !localPath.exists()) {
1161+
throw new IOException(
1162+
"Expected output " + prettyPrint(outputFile) + " was not created locally.");
11421163
}
1143-
outputFiles.add(outputPath);
1164+
outputFiles.add(localPath);
11441165
}
11451166

11461167
return UploadManifest.create(

‎src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java‎

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1101,9 +1101,21 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw
11011101
ActionResult r = ActionResult.newBuilder().setExitCode(0).build();
11021102
RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(r));
11031103
Artifact a1 = ActionsTestUtil.createArtifact(artifactRoot, "file1");
1104+
// set file1 as declared output but not mandatory output
11041105
Spawn spawn =
1105-
newSpawn(
1106-
ImmutableMap.of(REMOTE_EXECUTION_INLINE_OUTPUTS, "outputs/file1"), ImmutableSet.of(a1));
1106+
new SimpleSpawn(
1107+
new FakeOwner("foo", "bar", "//dummy:label"),
1108+
/*arguments=*/ ImmutableList.of(),
1109+
/*environment=*/ ImmutableMap.of(),
1110+
/*executionInfo=*/ ImmutableMap.of(REMOTE_EXECUTION_INLINE_OUTPUTS, "outputs/file1"),
1111+
/*runfilesSupplier=*/ null,
1112+
/*filesetMappings=*/ ImmutableMap.of(),
1113+
/*inputs=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
1114+
/*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
1115+
/*outputs=*/ ImmutableSet.of(a1),
1116+
/*mandatoryOutputs=*/ ImmutableSet.of(),
1117+
ResourceSet.ZERO);
1118+
11071119
MetadataInjector injector = mock(MetadataInjector.class);
11081120
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn, injector);
11091121
RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class);
@@ -1120,6 +1132,32 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw
11201132
verify(injector, never()).injectFile(eq(a1), remoteFileMatchingDigest(d1));
11211133
}
11221134

1135+
@Test
1136+
public void downloadOutputs_missingMandatoryOutputs_reportError() throws Exception {
1137+
// Test that an AC which misses mandatory outputs is correctly ignored.
1138+
Digest fooDigest = cache.addContents(remoteActionExecutionContext, "foo-contents");
1139+
ActionResult.Builder builder = ActionResult.newBuilder();
1140+
builder.addOutputFilesBuilder().setPath("outputs/foo").setDigest(fooDigest);
1141+
RemoteActionResult result =
1142+
RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build()));
1143+
ImmutableSet.Builder<Artifact> outputs = ImmutableSet.builder();
1144+
ImmutableList<String> expectedOutputFiles = ImmutableList.of("outputs/foo", "outputs/bar");
1145+
for (String outputFile : expectedOutputFiles) {
1146+
Path path = remotePathResolver.outputPathToLocalPath(outputFile);
1147+
Artifact output = ActionsTestUtil.createArtifact(artifactRoot, path);
1148+
outputs.add(output);
1149+
}
1150+
Spawn spawn = newSpawn(ImmutableMap.of(), outputs.build());
1151+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
1152+
RemoteExecutionService service = newRemoteExecutionService();
1153+
RemoteAction action = service.buildRemoteAction(spawn, context);
1154+
1155+
IOException error =
1156+
assertThrows(IOException.class, () -> service.downloadOutputs(action, result));
1157+
1158+
assertThat(error).hasMessageThat().containsMatch("expected output .+ does not exist.");
1159+
}
1160+
11231161
@Test
11241162
public void uploadOutputs_uploadDirectory_works() throws Exception {
11251163
// Test that uploading a directory works.
@@ -1368,7 +1406,7 @@ public void uploadOutputs_firesUploadEvents() throws Exception {
13681406
}
13691407

13701408
@Test
1371-
public void uploadOutputs_missingDeclaredOutputs_dontUpload() throws Exception {
1409+
public void uploadOutputs_missingMandatoryOutputs_dontUpload() throws Exception {
13721410
Path file = execRoot.getRelative("outputs/file");
13731411
Artifact outputFile = ActionsTestUtil.createArtifact(artifactRoot, file);
13741412
RemoteExecutionService service = newRemoteExecutionService();

‎src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import static org.mockito.ArgumentMatchers.eq;
2121
import static org.mockito.Mockito.doAnswer;
2222
import static org.mockito.Mockito.doNothing;
23+
import static org.mockito.Mockito.doReturn;
2324
import static org.mockito.Mockito.doThrow;
2425
import static org.mockito.Mockito.never;
2526
import static org.mockito.Mockito.spy;
@@ -602,6 +603,7 @@ public void testDownloadMinimal() throws Exception {
602603
when(remoteCache.downloadActionResult(
603604
any(RemoteActionExecutionContext.class), any(), /* inlineOutErr= */ eq(false)))
604605
.thenReturn(CachedActionResult.remote(success));
606+
doReturn(null).when(cache.getRemoteExecutionService()).downloadOutputs(any(), any());
605607

606608
// act
607609
CacheHandle cacheHandle = cache.lookup(simpleSpawn, simplePolicy);

‎src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1197,6 +1197,7 @@ public void testDownloadTopLevel() throws Exception {
11971197
RemoteSpawnRunner runner = newSpawnRunner(ImmutableSet.of(topLevelOutput));
11981198
RemoteExecutionService service = runner.getRemoteExecutionService();
11991199
doReturn(cachedActionResult).when(service).lookupCache(any());
1200+
doReturn(null).when(service).downloadOutputs(any(), any());
12001201

12011202
Spawn spawn = newSimpleSpawn(topLevelOutput);
12021203
FakeSpawnExecutionContext policy = getSpawnContext(spawn);

‎src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright 2017 The Bazel Authors. All rights reserved.
1+
/// Copyright 2017 The Bazel Authors. All rights reserved.
22
//
33
// Licensed under the Apache License, Version 2.0 (the "License");
44
// you may not use this file except in compliance with the License.
@@ -196,9 +196,12 @@ public final void setUp() throws Exception {
196196
ImmutableList.of("/bin/echo", "Hi!"),
197197
ImmutableMap.of("VARIABLE", "value"),
198198
/*executionInfo=*/ ImmutableMap.<String, String>of(),
199+
/*runfilesSupplier=*/ null,
200+
/*filesetMappings=*/ ImmutableMap.of(),
199201
/*inputs=*/ NestedSetBuilder.create(
200202
Order.STABLE_ORDER, ActionInputHelper.fromPath("input")),
201-
/*outputs=*/ ImmutableSet.<ActionInput>of(
203+
/*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
204+
/*outputs=*/ ImmutableSet.of(
202205
new ActionInput() {
203206
@Override
204207
public String getExecPathString() {
@@ -231,6 +234,7 @@ public PathFragment getExecPath() {
231234
return PathFragment.create("bar");
232235
}
233236
}),
237+
/*mandatoryOutputs=*/ ImmutableSet.of(),
234238
ResourceSet.ZERO);
235239

236240
Path stdout = fs.getPath("/tmp/stdout");

0 commit comments

Comments
 (0)