diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java index dd4cdea7f4b4fb..0a0a2a811b519a 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java @@ -41,6 +41,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.time.Duration; +import java.util.ArrayList; import java.util.List; import java.util.SortedMap; import java.util.concurrent.atomic.AtomicInteger; @@ -94,7 +95,8 @@ public List exec( // Actual execution. spawnResult = spawnRunner.exec(spawn, policy); if (cacheHandle.willStore()) { - cacheHandle.store(spawnResult); + cacheHandle.store( + spawnResult, listExistingOutputFiles(spawn, actionExecutionContext.getExecRoot())); } } } @@ -118,6 +120,19 @@ public List exec( return ImmutableList.of(spawnResult); } + private List listExistingOutputFiles(Spawn spawn, Path execRoot) { + ArrayList outputFiles = new ArrayList<>(); + for (ActionInput output : spawn.getOutputFiles()) { + Path outputPath = execRoot.getRelative(output.getExecPathString()); + // TODO(ulfjack): Store the actual list of output files in SpawnResult and use that instead + // of statting the files here again. + if (outputPath.exists()) { + outputFiles.add(outputPath); + } + } + return outputFiles; + } + private final class SpawnExecutionPolicyImpl implements SpawnExecutionPolicy { private final Spawn spawn; private final ActionExecutionContext actionExecutionContext; diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java index 206aa58bfc4b7c..1374b47fd84d7f 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java @@ -19,8 +19,10 @@ import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy; +import com.google.devtools.build.lib.vfs.Path; import java.io.Closeable; import java.io.IOException; +import java.util.Collection; import java.util.NoSuchElementException; /** @@ -31,31 +33,32 @@ */ public interface SpawnCache extends ActionContext { /** A no-op implementation that has no result, and performs no upload. */ - public static CacheHandle NO_RESULT_NO_STORE = - new CacheHandle() { - @Override - public boolean hasResult() { - return false; - } - - @Override - public SpawnResult getResult() { - throw new NoSuchElementException(); - } - - @Override - public boolean willStore() { - return false; - } - - @Override - public void store(SpawnResult result) throws InterruptedException, IOException { - // Do nothing. - } - - @Override - public void close() {} - }; + public static CacheHandle NO_RESULT_NO_STORE = new CacheHandle() { + @Override + public boolean hasResult() { + return false; + } + + @Override + public SpawnResult getResult() { + throw new NoSuchElementException(); + } + + @Override + public boolean willStore() { + return false; + } + + @Override + public void store(SpawnResult result, Collection files) + throws InterruptedException, IOException { + // Do nothing. + } + + @Override + public void close() { + } + }; /** * Helper method to create a {@link CacheHandle} from a successful {@link SpawnResult} instance. @@ -78,12 +81,14 @@ public boolean willStore() { } @Override - public void store(SpawnResult result) throws InterruptedException, IOException { + public void store(SpawnResult result, Collection files) + throws InterruptedException, IOException { throw new IllegalStateException(); } @Override - public void close() {} + public void close() { + } }; } @@ -143,7 +148,8 @@ interface CacheHandle extends Closeable { *

If the current thread is interrupted, then this method should return as quickly as * possible with an {@link InterruptedException}. */ - void store(SpawnResult result) throws ExecException, InterruptedException, IOException; + void store(SpawnResult result, Collection files) + throws InterruptedException, IOException; } /** diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java index 3bd935ca63769d..be81ad6cfe6883 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java @@ -15,16 +15,13 @@ import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; -import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.concurrent.ThreadSafety; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.Dirent; -import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.remoteexecution.v1test.ActionResult; import com.google.devtools.remoteexecution.v1test.Command; import com.google.devtools.remoteexecution.v1test.Digest; @@ -96,7 +93,7 @@ abstract void upload( Collection files, FileOutErr outErr, boolean uploadAction) - throws ExecException, IOException, InterruptedException; + throws IOException, InterruptedException; /** * Download a remote blob to a local destination. @@ -256,9 +253,10 @@ private void downloadOutErr(ActionResult result, FileOutErr outErr) } } - /** UploadManifest adds output metadata to a {@link ActionResult}. */ - static class UploadManifest { - private final DigestUtil digestUtil; + /** + * The UploadManifest is used to mutualize upload between the RemoteActionCache implementations. + */ + public class UploadManifest { private final ActionResult.Builder result; private final Path execRoot; private final Map digestToFile; @@ -268,8 +266,7 @@ static class UploadManifest { * Create an UploadManifest from an ActionResult builder and an exec root. The ActionResult * builder is populated through a call to {@link #addFile(Digest, Path)}. */ - public UploadManifest(DigestUtil digestUtil, ActionResult.Builder result, Path execRoot) { - this.digestUtil = digestUtil; + public UploadManifest(ActionResult.Builder result, Path execRoot) { this.result = result; this.execRoot = execRoot; @@ -278,31 +275,24 @@ public UploadManifest(DigestUtil digestUtil, ActionResult.Builder result, Path e } /** - * Add a collection of files or directories to the UploadManifest. Adding a directory has the + * Add a collection of files (and directories) to the UploadManifest. Adding a directory has the * effect of 1) uploading a {@link Tree} protobuf message from which the whole structure of the * directory, including the descendants, can be reconstructed and 2) uploading all the * non-directory descendant files. - * - *

Attempting to a upload symlink results in a {@link - * com.google.build.lib.actions.ExecException}, since cachable actions shouldn't emit symlinks. */ - public void addFiles(Collection files) - throws ExecException, IOException, InterruptedException { + public void addFiles(Collection files) throws IOException, InterruptedException { for (Path file : files) { // TODO(ulfjack): Maybe pass in a SpawnResult here, add a list of output files to that, and // rely on the local spawn runner to stat the files, instead of statting here. - FileStatus stat = file.statIfFound(Symlinks.NOFOLLOW); - if (stat == null) { + if (!file.exists()) { // We ignore requested results that have not been generated by the action. continue; } - if (stat.isDirectory()) { + if (file.isDirectory()) { addDirectory(file); - } else if (stat.isFile()) { - Digest digest = digestUtil.compute(file, stat.getSize()); - addFile(digest, file); } else { - illegalOutput(file); + Digest digest = digestUtil.compute(file); + addFile(digest, file); } } } @@ -331,7 +321,7 @@ private void addFile(Digest digest, Path file) throws IOException { digestToFile.put(digest, file); } - private void addDirectory(Path dir) throws ExecException, IOException { + private void addDirectory(Path dir) throws IOException { Tree.Builder tree = Tree.newBuilder(); Directory root = computeDirectory(dir, tree); tree.setRoot(root); @@ -350,8 +340,7 @@ private void addDirectory(Path dir) throws ExecException, IOException { digestToChunkers.put(chunker.digest(), chunker); } - private Directory computeDirectory(Path path, Tree.Builder tree) - throws ExecException, IOException { + private Directory computeDirectory(Path path, Tree.Builder tree) throws IOException { Directory.Builder b = Directory.newBuilder(); List sortedDirent = new ArrayList<>(path.readdir(TreeNodeRepository.SYMLINK_POLICY)); @@ -364,27 +353,15 @@ private Directory computeDirectory(Path path, Tree.Builder tree) Directory dir = computeDirectory(child, tree); b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir)); tree.addChildren(dir); - } else if (dirent.getType() == Dirent.Type.FILE) { + } else { Digest digest = digestUtil.compute(child); b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable()); digestToFile.put(digest, child); - } else { - illegalOutput(child); } } return b.build(); } - - private void illegalOutput(Path what) throws ExecException, IOException { - String kind = what.isSymbolicLink() ? "symbolic link" : "special file"; - throw new UserExecException( - String.format( - "Output %s is a %s. Only regular files and directories may be " - + "uploaded to a remote cache. " - + "Change the file type or add the \"no-cache\" tag/execution requirement.", - what.relativeTo(execRoot), kind)); - } } /** Release resources associated with the cache. The cache may not be used after calling this. */ diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java index a29d7d23c41a5c..3f5a48176b50d9 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java @@ -25,7 +25,6 @@ import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.MetadataProvider; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.remote.Retrier.RetryException; @@ -241,7 +240,7 @@ public void upload( Collection files, FileOutErr outErr, boolean uploadAction) - throws ExecException, IOException, InterruptedException { + throws IOException, InterruptedException { ActionResult.Builder result = ActionResult.newBuilder(); upload(execRoot, files, outErr, result); if (!uploadAction) { @@ -267,8 +266,8 @@ public void upload( } void upload(Path execRoot, Collection files, FileOutErr outErr, ActionResult.Builder result) - throws ExecException, IOException, InterruptedException { - UploadManifest manifest = new UploadManifest(digestUtil, result, execRoot); + throws IOException, InterruptedException { + UploadManifest manifest = new UploadManifest(result, execRoot); manifest.addFiles(files); List filesToUpload = new ArrayList<>(); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index b4b722a6465ada..80aa8f227a3510 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java @@ -160,8 +160,8 @@ public boolean willStore() { } @Override - public void store(SpawnResult result) - throws ExecException, InterruptedException, IOException { + public void store(SpawnResult result, Collection files) + throws InterruptedException, IOException { if (options.experimentalGuardAgainstConcurrentChanges) { try { checkForConcurrentModifications(); @@ -175,8 +175,6 @@ public void store(SpawnResult result) && Status.SUCCESS.equals(result.status()) && result.exitCode() == 0; Context previous = withMetadata.attach(); - Collection files = - RemoteSpawnRunner.resolveActionInputs(execRoot, spawn.getOutputFiles()); try { remoteCache.upload(actionKey, execRoot, files, policy.getFileOutErr(), uploadAction); } catch (IOException e) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index d6d879642e7337..cbbae37dfad89f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -16,7 +16,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Throwables; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputFileCache; @@ -442,12 +441,12 @@ SpawnResult execLocallyAndUpload( return result; } } - boolean uploadAction = - Spawns.mayBeCached(spawn) - && Status.SUCCESS.equals(result.status()) - && result.exitCode() == 0; - Collection outputFiles = resolveActionInputs(execRoot, spawn.getOutputFiles()); + List outputFiles = listExistingOutputFiles(execRoot, spawn); try { + boolean uploadAction = + Spawns.mayBeCached(spawn) + && Status.SUCCESS.equals(result.status()) + && result.exitCode() == 0; remoteCache.upload(actionKey, execRoot, outputFiles, policy.getFileOutErr(), uploadAction); } catch (IOException e) { if (verboseFailures) { @@ -471,12 +470,16 @@ private void report(Event evt) { } } - /** Resolve a collection of {@link com.google.build.lib.actions.ActionInput}s to {@link Path}s. */ - static Collection resolveActionInputs( - Path execRoot, Collection actionInputs) { - return actionInputs - .stream() - .map((inp) -> execRoot.getRelative(inp.getExecPath())) - .collect(ImmutableList.toImmutableList()); + static List listExistingOutputFiles(Path execRoot, Spawn spawn) { + ArrayList outputFiles = new ArrayList<>(); + for (ActionInput output : spawn.getOutputFiles()) { + Path outputPath = execRoot.getRelative(output.getExecPathString()); + // TODO(ulfjack): Store the actual list of output files in SpawnResult and use that instead + // of statting the files here again. + if (outputPath.exists()) { + outputFiles.add(outputPath); + } + } + return outputFiles; } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java index 845cf227b8a389..2d4f4e9bbc8885 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.remote; import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.MetadataProvider; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; @@ -115,7 +114,7 @@ public void upload( Collection files, FileOutErr outErr, boolean uploadAction) - throws ExecException, IOException, InterruptedException { + throws IOException, InterruptedException { ActionResult.Builder result = ActionResult.newBuilder(); upload(result, execRoot, files); if (outErr.getErrorPath().exists()) { @@ -132,8 +131,8 @@ public void upload( } public void upload(ActionResult.Builder result, Path execRoot, Collection files) - throws ExecException, IOException, InterruptedException { - UploadManifest manifest = new UploadManifest(digestUtil, result, execRoot); + throws IOException, InterruptedException { + UploadManifest manifest = new UploadManifest(result, execRoot); manifest.addFiles(files); for (Map.Entry entry : manifest.getDigestToFile().entrySet()) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java index e4dc92a46f6339..afd31ccdebcdaa 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java @@ -60,11 +60,9 @@ public Digest compute(byte[] blob) { } public Digest compute(Path file) throws IOException { - return compute(file, file.getFileSize()); - } - - public Digest compute(Path file, long fileSize) throws IOException { - return buildDigest(DigestUtils.getDigestOrFail(file, fileSize), fileSize); + long fileSize = file.getFileSize(); + byte[] digest = DigestUtils.getDigestOrFail(file, fileSize); + return buildDigest(digest, fileSize); } public Digest compute(VirtualActionInput input) throws IOException { diff --git a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java index 42de712e40221f..e9731210d3ec7d 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java @@ -34,6 +34,7 @@ import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import java.util.Collection; import java.util.List; import org.junit.Before; import org.junit.Test; @@ -146,7 +147,7 @@ public void testCacheMiss() throws Exception { // Must only be called exactly once. verify(spawnRunner).exec(any(Spawn.class), any(SpawnExecutionPolicy.class)); - verify(entry).store(eq(spawnResult)); + verify(entry).store(eq(spawnResult), any(Collection.class)); } @SuppressWarnings("unchecked") @@ -177,6 +178,6 @@ public void testCacheMissWithNonZeroExit() throws Exception { } // Must only be called exactly once. verify(spawnRunner).exec(any(Spawn.class), any(SpawnExecutionPolicy.class)); - verify(entry).store(eq(result)); + verify(entry).store(eq(result), any(Collection.class)); } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java deleted file mode 100644 index b4092a043ed1e3..00000000000000 --- a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java +++ /dev/null @@ -1,71 +0,0 @@ -// Copyright 2018 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. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -package com.google.devtools.build.lib.remote; - -import static com.google.common.truth.Truth.assertThat; -import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows; - -import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.actions.ExecException; -import com.google.devtools.build.lib.clock.JavaClock; -import com.google.devtools.build.lib.remote.AbstractRemoteActionCache.UploadManifest; -import com.google.devtools.build.lib.remote.util.DigestUtil; -import com.google.devtools.build.lib.vfs.FileSystem; -import com.google.devtools.build.lib.vfs.FileSystem.HashFunction; -import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; -import com.google.devtools.remoteexecution.v1test.ActionResult; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests for {@link AbstractRemoteActionCache}. */ -@RunWith(JUnit4.class) -public class AbstractRemoteActionCacheTests { - - private FileSystem fs; - private Path execRoot; - private final DigestUtil digestUtil = new DigestUtil(HashFunction.SHA256); - - @Before - public void setUp() throws Exception { - fs = new InMemoryFileSystem(new JavaClock(), HashFunction.SHA256); - execRoot = fs.getPath("/execroot"); - execRoot.createDirectory(); - } - - @Test - public void uploadSymlinkAsFile() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - UploadManifest um = new UploadManifest(digestUtil, result, execRoot); - Path link = fs.getPath("/execroot/link"); - link.createSymbolicLink(fs.getPath("/execroot/target")); - assertThat(assertThrows(ExecException.class, () -> um.addFiles(ImmutableList.of(link)))) - .hasMessageThat() - .contains("Only regular files and directories may be uploaded to a remote cache."); - } - - @Test - public void uploadSymlinkInDirectory() throws Exception { - ActionResult.Builder result = ActionResult.newBuilder(); - UploadManifest um = new UploadManifest(digestUtil, result, fs.getPath("/execroot")); - Path dir = fs.getPath("/execroot/dir"); - dir.createDirectory(); - fs.getPath("/execroot/dir/link").createSymbolicLink(fs.getPath("/execroot/target")); - assertThat(assertThrows(ExecException.class, () -> um.addFiles(ImmutableList.of(dir)))) - .hasMessageThat() - .contains("Only regular files and directories may be uploaded to a remote cache."); - } -} 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 a41f9c4f96537f..0374e3dc388aa8 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 @@ -165,7 +165,7 @@ public final void setUp() throws Exception { ImmutableMap.of("VARIABLE", "value"), /*executionInfo=*/ ImmutableMap.of(), /*inputs=*/ ImmutableList.of(ActionInputHelper.fromPath("input")), - /*outputs=*/ ImmutableList.of(ActionInputHelper.fromPath("/random/file")), + /*outputs=*/ ImmutableList.of(), ResourceSet.ZERO); Path stdout = fs.getPath("/tmp/stdout"); @@ -267,7 +267,7 @@ public Void answer(InvocationOnMock invocation) { }) .when(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true)); - entry.store(result); + entry.store(result, outputFiles); verify(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true)); } @@ -277,15 +277,14 @@ public void noCacheSpawns() throws Exception { // Checks that spawns that have mayBeCached false are not looked up in the remote cache, // and also that their result is not uploaded to the remote cache. The artifacts, however, // are uploaded. - SimpleSpawn uncacheableSpawn = - new SimpleSpawn( - new FakeOwner("foo", "bar"), - /*arguments=*/ ImmutableList.of(), - /*environment=*/ ImmutableMap.of(), - ImmutableMap.of(ExecutionRequirements.NO_CACHE, ""), - /*inputs=*/ ImmutableList.of(), - /*outputs=*/ ImmutableList.of(ActionInputHelper.fromPath("/random/file")), - ResourceSet.ZERO); + SimpleSpawn uncacheableSpawn = new SimpleSpawn( + new FakeOwner("foo", "bar"), + /*arguments=*/ ImmutableList.of(), + /*environment=*/ ImmutableMap.of(), + ImmutableMap.of(ExecutionRequirements.NO_CACHE, ""), + /*inputs=*/ ImmutableList.of(), + /*outputs=*/ ImmutableList.of(), + ResourceSet.ZERO); CacheHandle entry = cache.lookup(uncacheableSpawn, simplePolicy); verify(remoteCache, never()) .getCachedActionResult(any(ActionKey.class)); @@ -296,8 +295,8 @@ public void noCacheSpawns() throws Exception { .setStatus(Status.SUCCESS) .setRunnerName("test") .build(); - entry.store(result); ImmutableList outputFiles = ImmutableList.of(fs.getPath("/random/file")); + entry.store(result, outputFiles); verify(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false)); } @@ -316,7 +315,7 @@ public void noCacheSpawnsNoResultStore() throws Exception { .setRunnerName("test") .build(); ImmutableList outputFiles = ImmutableList.of(fs.getPath("/random/file")); - entry.store(result); + entry.store(result, outputFiles); verify(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false)); } @@ -337,7 +336,7 @@ public void printWarningIfUploadFails() throws Exception { .when(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true)); - entry.store(result); + entry.store(result, outputFiles); verify(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true)); diff --git a/src/test/shell/bazel/remote/remote_execution_http_test.sh b/src/test/shell/bazel/remote/remote_execution_http_test.sh index 4bd27555f50c4f..fa21adab536ba8 100755 --- a/src/test/shell/bazel/remote/remote_execution_http_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_http_test.sh @@ -127,52 +127,6 @@ EOF fi } -function test_refuse_to_upload_symlink() { - cat > BUILD <<'EOF' -genrule( - name = 'make-link', - outs = ['l', 't'], - cmd = 'touch $(location t) && ln -s t $(location l)', -) -EOF - bazel build \ - --genrule_strategy=remote \ - --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ - //:make-link &> $TEST_log \ - && fail "should have failed" || true - expect_log "/l is a symbolic link" - - bazel build \ - --experimental_remote_spawn_cache \ - --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ - //:make-link &> $TEST_log \ - && fail "should have failed" || true - expect_log "/l is a symbolic link" -} - -function test_refuse_to_upload_symlink_in_directory() { - cat > BUILD <<'EOF' -genrule( - name = 'make-link', - outs = ['dir'], - cmd = 'mkdir $(location dir) && touch $(location dir)/t && ln -s t $(location dir)/l', -) -EOF - bazel build \ - --genrule_strategy=remote \ - --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ - //:make-link &> $TEST_log \ - && fail "should have failed" || true - expect_log "dir/l is a symbolic link" - - bazel build \ - --experimental_remote_spawn_cache \ - --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \ - //:make-link &> $TEST_log \ - && fail "should have failed" || true - expect_log "dir/l is a symbolic link" -} - function set_directory_artifact_testfixtures() { mkdir -p a cat > a/BUILD <<'EOF' diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 2f5c6ebc241c98..277b882c6dad5f 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -522,23 +522,6 @@ EOF expect_log "Remote connection/protocol failed" } -function test_refuse_symlink_output() { - cat > BUILD <<'EOF' -genrule( - name = 'make-link', - outs = ['l', 't'], - cmd = 'touch $(location t) && ln -s t $(location l)', -) -EOF - - bazel build \ - --genrule_strategy=remote \ - --remote_executor=localhost:${worker_port} \ - //:make-link >& TEST_log \ - && fail "should have failed"# || true - expect_log "/l is a symbolic link" -} - # TODO(alpha): Add a test that fails remote execution when remote worker # supports sandbox. diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index 980822c5f21c60..098b4adf4be831 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -24,7 +24,6 @@ import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.ThreadFactoryBuilder; -import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.remote.CacheNotFoundException; import com.google.devtools.build.lib.remote.ExecutionStatusException; import com.google.devtools.build.lib.remote.SimpleBlobStoreActionCache; @@ -253,7 +252,6 @@ private ActionResult execute(Action action, Path execRoot) (cmdResult != null && cmdResult.getTerminationStatus().timedOut()) || wasTimeout(timeoutMillis, System.currentTimeMillis() - startTime); final int exitCode; - Status errStatus = null; ExecuteResponse.Builder resp = ExecuteResponse.newBuilder(); if (wasTimeout) { final String errMessage = @@ -261,11 +259,11 @@ private ActionResult execute(Action action, Path execRoot) "Command:\n%s\nexceeded deadline of %f seconds.", Arrays.toString(command.getArgumentsList().toArray()), timeoutMillis / 1000.0); logger.warning(errMessage); - errStatus = + resp.setStatus( Status.newBuilder() .setCode(Code.DEADLINE_EXCEEDED.getNumber()) .setMessage(errMessage) - .build(); + .build()); exitCode = LOCAL_EXEC_ERROR; } else if (cmdResult == null) { exitCode = LOCAL_EXEC_ERROR; @@ -274,29 +272,19 @@ private ActionResult execute(Action action, Path execRoot) } ActionResult.Builder result = ActionResult.newBuilder(); - try { - cache.upload(result, execRoot, outputs); - } catch (ExecException e) { - if (errStatus == null) { - errStatus = - Status.newBuilder() - .setCode(Code.FAILED_PRECONDITION.getNumber()) - .setMessage(e.getMessage()) - .build(); - } - } + cache.upload(result, execRoot, outputs); byte[] stdout = cmdResult.getStdout(); byte[] stderr = cmdResult.getStderr(); cache.uploadOutErr(result, stdout, stderr); ActionResult finalResult = result.setExitCode(exitCode).build(); - resp.setResult(finalResult); - if (errStatus != null) { - resp.setStatus(errStatus); - throw new ExecutionStatusException(errStatus, resp.build()); - } else if (exitCode == 0 && !action.getDoNotCache()) { + if (exitCode == 0 && !action.getDoNotCache()) { ActionKey actionKey = digestUtil.computeActionKey(action); cache.setCachedActionResult(actionKey, finalResult); } + resp.setResult(finalResult); + if (wasTimeout) { + throw new ExecutionStatusException(resp.getStatus(), resp.build()); + } return finalResult; }