diff options
Diffstat (limited to 'src')
14 files changed, 267 insertions, 116 deletions
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 0a0a2a811b..dd4cdea7f4 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,7 +41,6 @@ import com.google.devtools.build.lib.vfs.Path; 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; @@ -95,8 +94,7 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte // Actual execution. spawnResult = spawnRunner.exec(spawn, policy); if (cacheHandle.willStore()) { - cacheHandle.store( - spawnResult, listExistingOutputFiles(spawn, actionExecutionContext.getExecRoot())); + cacheHandle.store(spawnResult); } } } @@ -120,19 +118,6 @@ public abstract class AbstractSpawnStrategy implements SandboxedSpawnActionConte return ImmutableList.of(spawnResult); } - private List<Path> listExistingOutputFiles(Spawn spawn, Path execRoot) { - ArrayList<Path> 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 1374b47fd8..206aa58bfc 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,10 +19,8 @@ import com.google.devtools.build.lib.actions.ExecutionStrategy; 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; /** @@ -33,32 +31,31 @@ import java.util.NoSuchElementException; */ 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, Collection<Path> files) - 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) throws InterruptedException, IOException { + // Do nothing. + } + + @Override + public void close() {} + }; /** * Helper method to create a {@link CacheHandle} from a successful {@link SpawnResult} instance. @@ -81,14 +78,12 @@ public interface SpawnCache extends ActionContext { } @Override - public void store(SpawnResult result, Collection<Path> files) - throws InterruptedException, IOException { + public void store(SpawnResult result) throws InterruptedException, IOException { throw new IllegalStateException(); } @Override - public void close() { - } + public void close() {} }; } @@ -148,8 +143,7 @@ public interface SpawnCache extends ActionContext { * <p>If the current thread is interrupted, then this method should return as quickly as * possible with an {@link InterruptedException}. */ - void store(SpawnResult result, Collection<Path> files) - throws InterruptedException, IOException; + void store(SpawnResult result) throws ExecException, 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 be81ad6cfe..3bd935ca63 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,13 +15,16 @@ package com.google.devtools.build.lib.remote; 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; @@ -93,7 +96,7 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { Collection<Path> files, FileOutErr outErr, boolean uploadAction) - throws IOException, InterruptedException; + throws ExecException, IOException, InterruptedException; /** * Download a remote blob to a local destination. @@ -253,10 +256,9 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { } } - /** - * The UploadManifest is used to mutualize upload between the RemoteActionCache implementations. - */ - public class UploadManifest { + /** UploadManifest adds output metadata to a {@link ActionResult}. */ + static class UploadManifest { + private final DigestUtil digestUtil; private final ActionResult.Builder result; private final Path execRoot; private final Map<Digest, Path> digestToFile; @@ -266,7 +268,8 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { * 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(ActionResult.Builder result, Path execRoot) { + public UploadManifest(DigestUtil digestUtil, ActionResult.Builder result, Path execRoot) { + this.digestUtil = digestUtil; this.result = result; this.execRoot = execRoot; @@ -275,24 +278,31 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { } /** - * Add a collection of files (and directories) to the UploadManifest. Adding a directory has the + * Add a collection of files or 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. + * + * <p>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<Path> files) throws IOException, InterruptedException { + public void addFiles(Collection<Path> files) + throws ExecException, 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. - if (!file.exists()) { + FileStatus stat = file.statIfFound(Symlinks.NOFOLLOW); + if (stat == null) { // We ignore requested results that have not been generated by the action. continue; } - if (file.isDirectory()) { + if (stat.isDirectory()) { addDirectory(file); - } else { - Digest digest = digestUtil.compute(file); + } else if (stat.isFile()) { + Digest digest = digestUtil.compute(file, stat.getSize()); addFile(digest, file); + } else { + illegalOutput(file); } } } @@ -321,7 +331,7 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { digestToFile.put(digest, file); } - private void addDirectory(Path dir) throws IOException { + private void addDirectory(Path dir) throws ExecException, IOException { Tree.Builder tree = Tree.newBuilder(); Directory root = computeDirectory(dir, tree); tree.setRoot(root); @@ -340,7 +350,8 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { digestToChunkers.put(chunker.digest(), chunker); } - private Directory computeDirectory(Path path, Tree.Builder tree) throws IOException { + private Directory computeDirectory(Path path, Tree.Builder tree) + throws ExecException, IOException { Directory.Builder b = Directory.newBuilder(); List<Dirent> sortedDirent = new ArrayList<>(path.readdir(TreeNodeRepository.SYMLINK_POLICY)); @@ -353,15 +364,27 @@ public abstract class AbstractRemoteActionCache implements AutoCloseable { Directory dir = computeDirectory(child, tree); b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir)); tree.addChildren(dir); - } else { + } else if (dirent.getType() == Dirent.Type.FILE) { 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 3f5a48176b..a29d7d23c4 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,6 +25,7 @@ import com.google.common.collect.Iterables; 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; @@ -240,7 +241,7 @@ public class GrpcRemoteCache extends AbstractRemoteActionCache { Collection<Path> files, FileOutErr outErr, boolean uploadAction) - throws IOException, InterruptedException { + throws ExecException, IOException, InterruptedException { ActionResult.Builder result = ActionResult.newBuilder(); upload(execRoot, files, outErr, result); if (!uploadAction) { @@ -266,8 +267,8 @@ public class GrpcRemoteCache extends AbstractRemoteActionCache { } void upload(Path execRoot, Collection<Path> files, FileOutErr outErr, ActionResult.Builder result) - throws IOException, InterruptedException { - UploadManifest manifest = new UploadManifest(result, execRoot); + throws ExecException, IOException, InterruptedException { + UploadManifest manifest = new UploadManifest(digestUtil, result, execRoot); manifest.addFiles(files); List<Chunker> 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 6d766923ab..9b515d16a1 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 @@ final class RemoteSpawnCache implements SpawnCache { } @Override - public void store(SpawnResult result, Collection<Path> files) - throws InterruptedException, IOException { + public void store(SpawnResult result) + throws ExecException, InterruptedException, IOException { if (options.experimentalGuardAgainstConcurrentChanges) { try { checkForConcurrentModifications(); @@ -175,6 +175,8 @@ final class RemoteSpawnCache implements SpawnCache { && Status.SUCCESS.equals(result.status()) && result.exitCode() == 0; Context previous = withMetadata.attach(); + Collection<Path> 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 fb9724bac9..bb858a924a 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,6 +16,7 @@ package com.google.devtools.build.lib.remote; 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 +443,12 @@ class RemoteSpawnRunner implements SpawnRunner { return result; } } - List<Path> outputFiles = listExistingOutputFiles(execRoot, spawn); + boolean uploadAction = + Spawns.mayBeCached(spawn) + && Status.SUCCESS.equals(result.status()) + && result.exitCode() == 0; + Collection<Path> outputFiles = resolveActionInputs(execRoot, spawn.getOutputFiles()); 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,16 +472,12 @@ class RemoteSpawnRunner implements SpawnRunner { } } - static List<Path> listExistingOutputFiles(Path execRoot, Spawn spawn) { - ArrayList<Path> 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; + /** Resolve a collection of {@link com.google.build.lib.actions.ActionInput}s to {@link Path}s. */ + static Collection<Path> resolveActionInputs( + Path execRoot, Collection<? extends ActionInput> actionInputs) { + return actionInputs + .stream() + .map((inp) -> execRoot.getRelative(inp.getExecPath())) + .collect(ImmutableList.toImmutableList()); } } 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 2d4f4e9bbc..845cf227b8 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,6 +15,7 @@ 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; @@ -114,7 +115,7 @@ public final class SimpleBlobStoreActionCache extends AbstractRemoteActionCache Collection<Path> files, FileOutErr outErr, boolean uploadAction) - throws IOException, InterruptedException { + throws ExecException, IOException, InterruptedException { ActionResult.Builder result = ActionResult.newBuilder(); upload(result, execRoot, files); if (outErr.getErrorPath().exists()) { @@ -131,8 +132,8 @@ public final class SimpleBlobStoreActionCache extends AbstractRemoteActionCache } public void upload(ActionResult.Builder result, Path execRoot, Collection<Path> files) - throws IOException, InterruptedException { - UploadManifest manifest = new UploadManifest(result, execRoot); + throws ExecException, IOException, InterruptedException { + UploadManifest manifest = new UploadManifest(digestUtil, result, execRoot); manifest.addFiles(files); for (Map.Entry<Digest, Path> 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 afd31ccdeb..e4dc92a46f 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,9 +60,11 @@ public class DigestUtil { } public Digest compute(Path file) throws IOException { - long fileSize = file.getFileSize(); - byte[] digest = DigestUtils.getDigestOrFail(file, fileSize); - return buildDigest(digest, fileSize); + return compute(file, file.getFileSize()); + } + + public Digest compute(Path file, long fileSize) throws IOException { + return buildDigest(DigestUtils.getDigestOrFail(file, fileSize), 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 cd8c2c3909..c4d3b5bdbb 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,7 +34,6 @@ import com.google.devtools.build.lib.testutil.TestSpec; 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; @@ -140,7 +139,7 @@ public class AbstractSpawnStrategyTest { // Must only be called exactly once. verify(spawnRunner).exec(any(Spawn.class), any(SpawnExecutionPolicy.class)); - verify(entry).store(eq(spawnResult), any(Collection.class)); + verify(entry).store(eq(spawnResult)); } @SuppressWarnings("unchecked") @@ -167,6 +166,6 @@ public class AbstractSpawnStrategyTest { } // Must only be called exactly once. verify(spawnRunner).exec(any(Spawn.class), any(SpawnExecutionPolicy.class)); - verify(entry).store(eq(result), any(Collection.class)); + verify(entry).store(eq(result)); } } 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 new file mode 100644 index 0000000000..b4092a043e --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java @@ -0,0 +1,71 @@ +// 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 4acdca8e3d..9faf8e4d5c 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 class RemoteSpawnCacheTest { ImmutableMap.of("VARIABLE", "value"), /*executionInfo=*/ ImmutableMap.<String, String>of(), /*inputs=*/ ImmutableList.of(ActionInputHelper.fromPath("input")), - /*outputs=*/ ImmutableList.<ActionInput>of(), + /*outputs=*/ ImmutableList.of(ActionInputHelper.fromPath("/random/file")), ResourceSet.ZERO); Path stdout = fs.getPath("/tmp/stdout"); @@ -262,7 +262,7 @@ public class RemoteSpawnCacheTest { }) .when(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true)); - entry.store(result, outputFiles); + entry.store(result); verify(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true)); } @@ -272,21 +272,22 @@ public class RemoteSpawnCacheTest { // 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.<ActionInput>of(), - 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(ActionInputHelper.fromPath("/random/file")), + ResourceSet.ZERO); CacheHandle entry = cache.lookup(uncacheableSpawn, simplePolicy); verify(remoteCache, never()) .getCachedActionResult(any(ActionKey.class)); assertThat(entry.hasResult()).isFalse(); SpawnResult result = new SpawnResult.Builder().setExitCode(0).setStatus(Status.SUCCESS).build(); + entry.store(result); ImmutableList<Path> 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)); } @@ -301,7 +302,7 @@ public class RemoteSpawnCacheTest { SpawnResult result = new SpawnResult.Builder().setExitCode(1).setStatus(Status.NON_ZERO_EXIT).build(); ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file")); - entry.store(result, outputFiles); + entry.store(result); verify(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false)); } @@ -317,7 +318,7 @@ public class RemoteSpawnCacheTest { .when(remoteCache) .upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true)); - entry.store(result, outputFiles); + entry.store(result); 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 fa21adab53..4bd27555f5 100755 --- a/src/test/shell/bazel/remote/remote_execution_http_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_http_test.sh @@ -127,6 +127,52 @@ 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 277b882c6d..2f5c6ebc24 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -522,6 +522,23 @@ 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 098b4adf4b..980822c5f2 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,6 +24,7 @@ import com.google.common.util.concurrent.ListenableFuture; 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; @@ -252,6 +253,7 @@ final class ExecutionServer extends ExecutionImplBase { (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 = @@ -259,11 +261,11 @@ final class ExecutionServer extends ExecutionImplBase { "Command:\n%s\nexceeded deadline of %f seconds.", Arrays.toString(command.getArgumentsList().toArray()), timeoutMillis / 1000.0); logger.warning(errMessage); - resp.setStatus( + errStatus = Status.newBuilder() .setCode(Code.DEADLINE_EXCEEDED.getNumber()) .setMessage(errMessage) - .build()); + .build(); exitCode = LOCAL_EXEC_ERROR; } else if (cmdResult == null) { exitCode = LOCAL_EXEC_ERROR; @@ -272,19 +274,29 @@ final class ExecutionServer extends ExecutionImplBase { } ActionResult.Builder result = ActionResult.newBuilder(); - cache.upload(result, execRoot, outputs); + try { + cache.upload(result, execRoot, outputs); + } catch (ExecException e) { + if (errStatus == null) { + errStatus = + Status.newBuilder() + .setCode(Code.FAILED_PRECONDITION.getNumber()) + .setMessage(e.getMessage()) + .build(); + } + } byte[] stdout = cmdResult.getStdout(); byte[] stderr = cmdResult.getStderr(); cache.uploadOutErr(result, stdout, stderr); ActionResult finalResult = result.setExitCode(exitCode).build(); - if (exitCode == 0 && !action.getDoNotCache()) { + resp.setResult(finalResult); + if (errStatus != null) { + resp.setStatus(errStatus); + throw new ExecutionStatusException(errStatus, resp.build()); + } else 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; } |