diff options
Diffstat (limited to 'src/main/java/com/google')
8 files changed, 97 insertions, 92 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 { |