diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index ebe3ec05f57943..b6ccc3adc071d4 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java @@ -1011,6 +1011,23 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re metadata = parseActionResultMetadata(action, result); } + // Check that all mandatory outputs are created. + for (ActionInput output : action.spawn.getOutputFiles()) { + if (action.spawn.isMandatoryOutput(output)) { + Path localPath = execRoot.getRelative(output.getExecPath()); + if (!metadata.files.containsKey(localPath) + && !metadata.directories.containsKey(localPath) + && !metadata.symlinks.containsKey(localPath)) { + throw new IOException( + "Invalid action cache entry " + + action.actionKey.getDigest().getHash() + + ": expected output " + + prettyPrint(output) + + " does not exist."); + } + } + } + FileOutErr outErr = action.spawnExecutionContext.getFileOutErr(); ImmutableList.Builder> downloadsBuilder = @@ -1124,23 +1141,27 @@ public InMemoryOutput downloadOutputs(RemoteAction action, RemoteActionResult re return null; } + private static String prettyPrint(ActionInput actionInput) { + if (actionInput instanceof Artifact) { + return ((Artifact) actionInput).prettyPrint(); + } else { + return actionInput.getExecPathString(); + } + } + private Single buildUploadManifestAsync( RemoteAction action, SpawnResult spawnResult) { return Single.fromCallable( () -> { ImmutableList.Builder outputFiles = ImmutableList.builder(); + // Check that all mandatory outputs are created. for (ActionInput outputFile : action.spawn.getOutputFiles()) { - Path outputPath = execRoot.getRelative(outputFile.getExecPath()); - if (!outputPath.exists()) { - String output; - if (outputFile instanceof Artifact) { - output = ((Artifact) outputFile).prettyPrint(); - } else { - output = outputFile.getExecPathString(); - } - throw new IOException("Expected output " + output + " was not created locally."); + Path localPath = execRoot.getRelative(outputFile.getExecPath()); + if (action.spawn.isMandatoryOutput(outputFile) && !localPath.exists()) { + throw new IOException( + "Expected output " + prettyPrint(outputFile) + " was not created locally."); } - outputFiles.add(outputPath); + outputFiles.add(localPath); } return UploadManifest.create( diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 9011e20c98149b..26615963cd5756 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java @@ -1101,9 +1101,21 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw ActionResult r = ActionResult.newBuilder().setExitCode(0).build(); RemoteActionResult result = RemoteActionResult.createFromCache(CachedActionResult.remote(r)); Artifact a1 = ActionsTestUtil.createArtifact(artifactRoot, "file1"); + // set file1 as declared output but not mandatory output Spawn spawn = - newSpawn( - ImmutableMap.of(REMOTE_EXECUTION_INLINE_OUTPUTS, "outputs/file1"), ImmutableSet.of(a1)); + new SimpleSpawn( + new FakeOwner("foo", "bar", "//dummy:label"), + /*arguments=*/ ImmutableList.of(), + /*environment=*/ ImmutableMap.of(), + /*executionInfo=*/ ImmutableMap.of(REMOTE_EXECUTION_INLINE_OUTPUTS, "outputs/file1"), + /*runfilesSupplier=*/ null, + /*filesetMappings=*/ ImmutableMap.of(), + /*inputs=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /*outputs=*/ ImmutableSet.of(a1), + /*mandatoryOutputs=*/ ImmutableSet.of(), + ResourceSet.ZERO); + MetadataInjector injector = mock(MetadataInjector.class); FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn, injector); RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); @@ -1120,6 +1132,32 @@ public void downloadOutputs_missingInMemoryOutputWithMinimal_returnsNull() throw verify(injector, never()).injectFile(eq(a1), remoteFileMatchingDigest(d1)); } + @Test + public void downloadOutputs_missingMandatoryOutputs_reportError() throws Exception { + // Test that an AC which misses mandatory outputs is correctly ignored. + Digest fooDigest = cache.addContents(remoteActionExecutionContext, "foo-contents"); + ActionResult.Builder builder = ActionResult.newBuilder(); + builder.addOutputFilesBuilder().setPath("outputs/foo").setDigest(fooDigest); + RemoteActionResult result = + RemoteActionResult.createFromCache(CachedActionResult.remote(builder.build())); + ImmutableSet.Builder outputs = ImmutableSet.builder(); + ImmutableList expectedOutputFiles = ImmutableList.of("outputs/foo", "outputs/bar"); + for (String outputFile : expectedOutputFiles) { + Path path = remotePathResolver.outputPathToLocalPath(outputFile); + Artifact output = ActionsTestUtil.createArtifact(artifactRoot, path); + outputs.add(output); + } + Spawn spawn = newSpawn(ImmutableMap.of(), outputs.build()); + FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn); + RemoteExecutionService service = newRemoteExecutionService(); + RemoteAction action = service.buildRemoteAction(spawn, context); + + IOException error = + assertThrows(IOException.class, () -> service.downloadOutputs(action, result)); + + assertThat(error).hasMessageThat().containsMatch("expected output .+ does not exist."); + } + @Test public void uploadOutputs_uploadDirectory_works() throws Exception { // Test that uploading a directory works. @@ -1368,7 +1406,7 @@ public void uploadOutputs_firesUploadEvents() throws Exception { } @Test - public void uploadOutputs_missingDeclaredOutputs_dontUpload() throws Exception { + public void uploadOutputs_missingMandatoryOutputs_dontUpload() throws Exception { Path file = execRoot.getRelative("outputs/file"); Artifact outputFile = ActionsTestUtil.createArtifact(artifactRoot, file); RemoteExecutionService service = newRemoteExecutionService(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index b359091f6ce5bc..c46e05d31733ab 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java @@ -20,6 +20,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; @@ -602,6 +603,7 @@ public void testDownloadMinimal() throws Exception { when(remoteCache.downloadActionResult( any(RemoteActionExecutionContext.class), any(), /* inlineOutErr= */ eq(false))) .thenReturn(CachedActionResult.remote(success)); + doReturn(null).when(cache.getRemoteExecutionService()).downloadOutputs(any(), any()); // act CacheHandle cacheHandle = cache.lookup(simpleSpawn, simplePolicy); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index d7ca873267073e..22afbf5d2e8334 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -1197,6 +1197,7 @@ public void testDownloadTopLevel() throws Exception { RemoteSpawnRunner runner = newSpawnRunner(ImmutableSet.of(topLevelOutput)); RemoteExecutionService service = runner.getRemoteExecutionService(); doReturn(cachedActionResult).when(service).lookupCache(any()); + doReturn(null).when(service).downloadOutputs(any(), any()); Spawn spawn = newSimpleSpawn(topLevelOutput); FakeSpawnExecutionContext policy = getSpawnContext(spawn); diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java index a59627a6b19349..57066fa8a9ae52 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java @@ -1,4 +1,4 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. +/// Copyright 2017 The Bazel Authors. All rights reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -196,9 +196,12 @@ public final void setUp() throws Exception { ImmutableList.of("/bin/echo", "Hi!"), ImmutableMap.of("VARIABLE", "value"), /*executionInfo=*/ ImmutableMap.of(), + /*runfilesSupplier=*/ null, + /*filesetMappings=*/ ImmutableMap.of(), /*inputs=*/ NestedSetBuilder.create( Order.STABLE_ORDER, ActionInputHelper.fromPath("input")), - /*outputs=*/ ImmutableSet.of( + /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /*outputs=*/ ImmutableSet.of( new ActionInput() { @Override public String getExecPathString() { @@ -231,6 +234,7 @@ public PathFragment getExecPath() { return PathFragment.create("bar"); } }), + /*mandatoryOutputs=*/ ImmutableSet.of(), ResourceSet.ZERO); Path stdout = fs.getPath("/tmp/stdout");