From 4e29042fd77eae565a711655df6150500cb6e915 Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 3 Aug 2021 17:56:19 -0700 Subject: [PATCH] Save remote output metadata to ActionCache and load them before checking action cache This PR extends `ActionCache.Entry` to store output metadata by having a map of . This map is updated after action execution when we update action cache so that metadata of all outputs of the action are saved. Before checking the action cache (when executing actions), we will load the output metadata into output store if it is remote and the correspondingly local one is missing. With this change, remote output metadata is saved to disk so build without bytes can use them among server restarts. We can also download outputs after action execution since remote output metadata can be accessed outside. Part of https://github.com/bazelbuild/bazel/issues/12665. Fixes https://github.com/bazelbuild/bazel/issues/8248. Closes #13604. PiperOrigin-RevId: 388586691 --- .../build/lib/actions/ActionCacheChecker.java | 276 ++++++- .../build/lib/actions/cache/ActionCache.java | 165 +++- .../cache/CompactPersistentActionCache.java | 295 +++++-- .../lib/buildtool/BuildRequestOptions.java | 11 + .../build/lib/buildtool/ExecutionTool.java | 4 +- .../lib/remote/RemoteExecutionService.java | 6 +- .../lib/skyframe/ActionMetadataHandler.java | 2 - .../build/lib/skyframe/TreeArtifactValue.java | 15 +- .../devtools/build/lib/util/VarInt.java | 7 + .../lib/actions/ActionCacheCheckerTest.java | 765 +++++++++++++++++- .../CompactPersistentActionCacheTest.java | 168 +++- .../lib/actions/util/ActionsTestUtil.java | 2 +- .../SequencedSkyframeExecutorTest.java | 3 +- .../skyframe/TimestampBuilderTestCase.java | 7 +- .../bazel/remote/remote_execution_test.sh | 46 ++ 15 files changed, 1587 insertions(+), 185 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index 8a62a01c14b9dc..4317f7731a1cc6 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -13,14 +13,21 @@ // limitations under the License. package com.google.devtools.build.lib.actions; +import static com.google.common.base.Preconditions.checkState; + import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; +import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.cache.ActionCache; +import com.google.devtools.build.lib.actions.cache.ActionCache.Entry.SerializableTreeArtifactValue; import com.google.devtools.build.lib.actions.cache.MetadataDigestUtils; import com.google.devtools.build.lib.actions.cache.MetadataHandler; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics.MissReason; @@ -30,14 +37,18 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.events.EventKind; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import java.io.FileNotFoundException; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import javax.annotation.Nullable; /** @@ -54,10 +65,13 @@ * otherwise lightweight, and should be constructed anew and discarded for each build request. */ public class ActionCacheChecker { + private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); + private final ActionKeyContext actionKeyContext; private final Predicate executionFilter; private final ArtifactResolver artifactResolver; private final CacheConfig cacheConfig; + private final PathFragment derivedPathPrefix; @Nullable private final ActionCache actionCache; // Null when not enabled. @@ -68,6 +82,8 @@ public abstract static class CacheConfig { // True iff --verbose_explanations flag is set. abstract boolean verboseExplanations(); + abstract boolean storeOutputMetadata(); + public static Builder builder() { return new AutoValue_ActionCacheChecker_CacheConfig.Builder(); } @@ -79,6 +95,8 @@ public abstract static class Builder { public abstract Builder setEnabled(boolean value); + public abstract Builder setStoreOutputMetadata(boolean value); + public abstract CacheConfig build(); } } @@ -88,19 +106,25 @@ public ActionCacheChecker( ArtifactResolver artifactResolver, ActionKeyContext actionKeyContext, Predicate executionFilter, - @Nullable CacheConfig cacheConfig) { + @Nullable CacheConfig cacheConfig, + PathFragment derivedPathPrefix) { this.executionFilter = executionFilter; this.actionKeyContext = actionKeyContext; this.artifactResolver = artifactResolver; this.cacheConfig = cacheConfig != null ? cacheConfig - : CacheConfig.builder().setEnabled(true).setVerboseExplanations(false).build(); + : CacheConfig.builder() + .setEnabled(true) + .setVerboseExplanations(false) + .setStoreOutputMetadata(false) + .build(); if (this.cacheConfig.enabled()) { this.actionCache = Preconditions.checkNotNull(actionCache); } else { this.actionCache = null; } + this.derivedPathPrefix = derivedPathPrefix; } public boolean isActionExecutionProhibited(Action action) { @@ -113,8 +137,8 @@ public boolean enabled() { } /** - * Checks whether one of existing output paths is already used as a key. - * If yes, returns it - otherwise uses first output file as a key + * Checks whether one of existing output paths is already used as a key. If yes, returns it - + * otherwise uses first output file as a key */ private ActionCache.Entry getCacheEntry(Action action) { if (!cacheConfig.enabled()) { @@ -135,6 +159,26 @@ private void removeCacheEntry(Action action) { } } + @Nullable + private static FileArtifactValue getCachedMetadata( + @Nullable CachedOutputMetadata cachedOutputMetadata, Artifact artifact) { + if (cachedOutputMetadata == null) { + return null; + } + + if (artifact.isTreeArtifact()) { + TreeArtifactValue value = + cachedOutputMetadata.mergedTreeMetadata.get((SpecialArtifact) artifact); + if (value == null) { + return null; + } + + return value.getMetadata(); + } else { + return cachedOutputMetadata.remoteFileMetadata.get(artifact); + } + } + /** * Validate metadata state for action input or output artifacts. * @@ -144,6 +188,8 @@ private void removeCacheEntry(Action action) { * but if this action doesn't yet know its inputs, we check the inputs from the cache. * @param metadataHandler provider of metadata for the artifacts this action interacts with. * @param checkOutput true to validate output artifacts, Otherwise, just validate inputs. + * @param cachedOutputMetadata a set of cached metadata that should be used instead of loading + * from {@code metadataHandler}. * @return true if at least one artifact has changed, false - otherwise. */ private static boolean validateArtifacts( @@ -151,11 +197,17 @@ private static boolean validateArtifacts( Action action, NestedSet actionInputs, MetadataHandler metadataHandler, - boolean checkOutput) { + boolean checkOutput, + @Nullable CachedOutputMetadata cachedOutputMetadata) { Map mdMap = new HashMap<>(); if (checkOutput) { for (Artifact artifact : action.getOutputs()) { - mdMap.put(artifact.getExecPathString(), getMetadataMaybe(metadataHandler, artifact)); + FileArtifactValue metadata = getCachedMetadata(cachedOutputMetadata, artifact); + if (metadata == null) { + metadata = getMetadataMaybe(metadataHandler, artifact); + } + + mdMap.put(artifact.getExecPathString(), metadata); } } for (Artifact artifact : actionInputs.toList()) { @@ -168,11 +220,16 @@ private void reportCommand(EventHandler handler, Action action) { if (handler != null) { if (cacheConfig.verboseExplanations()) { String keyDescription = action.describeKey(); - reportRebuild(handler, action, keyDescription == null - ? "action command has changed" - : "action command has changed.\nNew action: " + keyDescription); + reportRebuild( + handler, + action, + keyDescription == null + ? "action command has changed" + : "action command has changed.\nNew action: " + keyDescription); } else { - reportRebuild(handler, action, + reportRebuild( + handler, + action, "action command has changed (try --verbose_explanations for more info)"); } } @@ -184,7 +241,11 @@ private void reportClientEnv(EventHandler handler, Action action, Map entry : used.entrySet()) { - message.append(" ").append(entry.getKey()).append("=").append(entry.getValue()) + message + .append(" ") + .append(entry.getKey()) + .append("=") + .append(entry.getValue()) .append("\n"); } reportRebuild(handler, action, message.toString()); @@ -234,6 +295,113 @@ private static Map computeUsedEnv( return usedEnvironment; } + private static class CachedOutputMetadata { + private final ImmutableMap remoteFileMetadata; + // TreeArtifactValues merged from local files and remote metadata pulled from the action cache. + private final ImmutableMap mergedTreeMetadata; + + private CachedOutputMetadata( + ImmutableMap remoteFileMetadata, + ImmutableMap mergedTreeMetadata) { + this.remoteFileMetadata = remoteFileMetadata; + this.mergedTreeMetadata = mergedTreeMetadata; + } + } + + private static CachedOutputMetadata loadCachedOutputMetadata( + Action action, + ActionCache.Entry entry, + MetadataHandler metadataHandler, + PathFragment derivedPathPrefix) { + ImmutableMap.Builder remoteFileMetadata = + ImmutableMap.builder(); + ImmutableMap.Builder mergedTreeMetadata = + ImmutableMap.builder(); + + for (Artifact artifact : action.getOutputs()) { + if (artifact.isTreeArtifact()) { + SpecialArtifact parent = (SpecialArtifact) artifact; + SerializableTreeArtifactValue cachedTreeMetadata = entry.getOutputTree(parent); + if (cachedTreeMetadata == null) { + continue; + } + + TreeArtifactValue localTreeMetadata; + try { + localTreeMetadata = metadataHandler.getTreeArtifactValue(parent); + } catch (IOException e) { + // Ignore the cached metadata if we encountered an error when loading corresponding + // local one. + logger.atWarning().withCause(e).log("Failed to load metadata for %s", parent); + continue; + } + + boolean localTreeMetadataExists = + localTreeMetadata != null + && !localTreeMetadata.equals(TreeArtifactValue.MISSING_TREE_ARTIFACT); + + Map childValues = new HashMap<>(); + // Load remote child file metadata from cache. + cachedTreeMetadata + .childValues() + .forEach( + (key, value) -> + childValues.put(TreeFileArtifact.createTreeOutput(parent, key), value)); + // Or add local one. + if (localTreeMetadataExists) { + localTreeMetadata.getChildValues().forEach(childValues::put); + } + + Optional archivedRepresentation; + if (localTreeMetadataExists && localTreeMetadata.getArchivedRepresentation().isPresent()) { + archivedRepresentation = localTreeMetadata.getArchivedRepresentation(); + } else { + archivedRepresentation = + cachedTreeMetadata + .archivedFileValue() + .map( + fileArtifactValue -> { + Artifact.ArchivedTreeArtifact archivedTreeArtifact = + Artifact.ArchivedTreeArtifact.create(parent, derivedPathPrefix); + return ArchivedRepresentation.create( + archivedTreeArtifact, fileArtifactValue); + }); + } + + TreeArtifactValue.Builder merged = TreeArtifactValue.newBuilder(parent); + childValues.forEach(merged::putChild); + archivedRepresentation.ifPresent(merged::setArchivedRepresentation); + + // Always inject merged tree if we have a tree from cache + mergedTreeMetadata.put(parent, merged.build()); + } else { + RemoteFileArtifactValue cachedMetadata = entry.getOutputFile(artifact); + if (cachedMetadata == null) { + continue; + } + + FileArtifactValue localMetadata; + try { + localMetadata = getMetadataOrConstant(metadataHandler, artifact); + } catch (FileNotFoundException ignored) { + localMetadata = null; + } catch (IOException e) { + // Ignore the cached metadata if we encountered an error when loading the corresponding + // local one. + logger.atWarning().withCause(e).log("Failed to load metadata for %s", artifact); + continue; + } + + // Only inject remote metadata if the corresponding local one is missing. + if (localMetadata == null) { + remoteFileMetadata.put(artifact, cachedMetadata); + } + } + } + + return new CachedOutputMetadata(remoteFileMetadata.build(), mergedTreeMetadata.build()); + } + /** * Checks whether {@code action} needs to be executed and returns a non-null Token if so. * @@ -278,8 +446,10 @@ public Token getTokenIfNeedToExecute( boolean inputsDiscovered = action.inputsDiscovered(); if (!inputsDiscovered && resolvedCacheArtifacts != null) { // The action doesn't know its inputs, but the caller has a good idea of what they are. - Preconditions.checkState(action.discoversInputs(), - "Actions that don't know their inputs must discover them: %s", action); + checkState( + action.discoversInputs(), + "Actions that don't know their inputs must discover them: %s", + action); if (action instanceof ActionCacheAwareAction && ((ActionCacheAwareAction) action).storeInputsExecPathsInActionCache()) { actionInputs = NestedSetBuilder.wrap(Order.STABLE_ORDER, resolvedCacheArtifacts); @@ -292,6 +462,13 @@ public Token getTokenIfNeedToExecute( } } ActionCache.Entry entry = getCacheEntry(action); + CachedOutputMetadata cachedOutputMetadata = null; + // load remote metadata from action cache + if (entry != null && !entry.isCorrupted() && cacheConfig.storeOutputMetadata()) { + cachedOutputMetadata = + loadCachedOutputMetadata(action, entry, metadataHandler, derivedPathPrefix); + } + if (mustExecute( action, entry, @@ -300,7 +477,8 @@ public Token getTokenIfNeedToExecute( artifactExpander, actionInputs, clientEnv, - remoteDefaultPlatformProperties)) { + remoteDefaultPlatformProperties, + cachedOutputMetadata)) { if (entry != null) { removeCacheEntry(action); } @@ -310,6 +488,13 @@ public Token getTokenIfNeedToExecute( if (!inputsDiscovered) { action.updateInputs(actionInputs); } + + // Inject cached output metadata if we have an action cache hit + if (cachedOutputMetadata != null) { + cachedOutputMetadata.remoteFileMetadata.forEach(metadataHandler::injectFile); + cachedOutputMetadata.mergedTreeMetadata.forEach(metadataHandler::injectTree); + } + return null; } @@ -321,11 +506,12 @@ private boolean mustExecute( ArtifactExpander artifactExpander, NestedSet actionInputs, Map clientEnv, - Map remoteDefaultPlatformProperties) + Map remoteDefaultPlatformProperties, + @Nullable CachedOutputMetadata cachedOutputMetadata) throws InterruptedException { // Unconditional execution can be applied only for actions that are allowed to be executed. if (unconditionalExecution(action)) { - Preconditions.checkState(action.isVolatile()); + checkState(action.isVolatile()); reportUnconditionalExecution(handler, action); actionCache.accountMiss(MissReason.UNCONDITIONAL_EXECUTION); return true; @@ -340,7 +526,8 @@ private boolean mustExecute( reportCorruptedCacheEntry(handler, action); actionCache.accountMiss(MissReason.CORRUPTED_CACHE_ENTRY); return true; - } else if (validateArtifacts(entry, action, actionInputs, metadataHandler, true)) { + } else if (validateArtifacts( + entry, action, actionInputs, metadataHandler, true, cachedOutputMetadata)) { reportChanged(handler, action); actionCache.accountMiss(MissReason.DIFFERENT_FILES); return true; @@ -392,8 +579,7 @@ public void updateActionCache( Map clientEnv, Map remoteDefaultPlatformProperties) throws IOException, InterruptedException { - Preconditions.checkState( - cacheConfig.enabled(), "cache unexpectedly disabled, action: %s", action); + checkState(cacheConfig.enabled(), "cache unexpectedly disabled, action: %s", action); Preconditions.checkArgument(token != null, "token unexpectedly null, action: %s", action); String key = token.cacheKey; if (actionCache.get(key) != null) { @@ -414,13 +600,19 @@ public void updateActionCache( actionCache.remove(execPath); } if (!metadataHandler.artifactOmitted(output)) { - // Output files *must* exist and be accessible after successful action execution. We use the - // 'constant' metadata for the volatile workspace status output. The volatile output - // contains information such as timestamps, and even when --stamp is enabled, we don't want - // to rebuild everything if only that file changes. - FileArtifactValue metadata = getMetadataOrConstant(metadataHandler, output); - Preconditions.checkState(metadata != null); - entry.addFile(output.getExecPath(), metadata, /* saveExecPath= */ false); + if (output.isTreeArtifact()) { + SpecialArtifact parent = (SpecialArtifact) output; + TreeArtifactValue metadata = metadataHandler.getTreeArtifactValue(parent); + entry.addOutputTree(parent, metadata, cacheConfig.storeOutputMetadata()); + } else { + // Output files *must* exist and be accessible after successful action execution. We use + // the 'constant' metadata for the volatile workspace status output. The volatile output + // contains information such as timestamps, and even when --stamp is enabled, we don't + // want to rebuild everything if only that file changes. + FileArtifactValue metadata = getMetadataOrConstant(metadataHandler, output); + checkState(metadata != null); + entry.addOutputFile(output, metadata, cacheConfig.storeOutputMetadata()); + } } } @@ -433,7 +625,7 @@ public void updateActionCache( : ImmutableSet.of(); for (Artifact input : action.getInputs().toList()) { - entry.addFile( + entry.addInputFile( input.getExecPath(), getMetadataMaybe(metadataHandler, input), /* saveExecPath= */ !excludePathsFromActionCache.contains(input)); @@ -531,7 +723,13 @@ private void checkMiddlemanAction( reportCorruptedCacheEntry(handler, action); actionCache.accountMiss(MissReason.CORRUPTED_CACHE_ENTRY); changed = true; - } else if (validateArtifacts(entry, action, action.getInputs(), metadataHandler, false)) { + } else if (validateArtifacts( + entry, + action, + action.getInputs(), + metadataHandler, + false, + /*cachedOutputMetadata=*/ null)) { reportChanged(handler, action); actionCache.accountMiss(MissReason.DIFFERENT_FILES); changed = true; @@ -547,7 +745,8 @@ private void checkMiddlemanAction( // it in the cache entry and just use empty string instead. entry = new ActionCache.Entry("", ImmutableMap.of(), false); for (Artifact input : action.getInputs().toList()) { - entry.addFile(input.getExecPath(), getMetadataMaybe(metadataHandler, input)); + entry.addInputFile( + input.getExecPath(), getMetadataMaybe(metadataHandler, input), /*saveExecPath=*/ true); } } @@ -559,25 +758,24 @@ private void checkMiddlemanAction( } } - /** - * Returns an action key. It is always set to the first output exec path string. - */ + /** Returns an action key. It is always set to the first output exec path string. */ private static String getKeyString(Action action) { - Preconditions.checkState(!action.getOutputs().isEmpty()); + checkState(!action.getOutputs().isEmpty()); return action.getOutputs().iterator().next().getExecPathString(); } - /** - * In most cases, this method should not be called directly - reportXXX() methods - * should be used instead. This is done to avoid cost associated with building - * the message. + * In most cases, this method should not be called directly - reportXXX() methods should be used + * instead. This is done to avoid cost associated with building the message. */ private static void reportRebuild(@Nullable EventHandler handler, Action action, String message) { // For MiddlemanAction, do not report rebuild. if (handler != null && !action.getActionType().isMiddleman()) { - handler.handle(Event.of( - EventKind.DEPCHECKER, null, "Executing " + action.prettyPrint() + ": " + message + ".")); + handler.handle( + Event.of( + EventKind.DEPCHECKER, + null, + "Executing " + action.prettyPrint() + ": " + message + ".")); } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java index f7b7b0a8d513f6..94bc9fbf2407a8 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java @@ -14,15 +14,23 @@ package com.google.devtools.build.lib.actions.cache; -import com.google.common.base.Preconditions; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableMap.toImmutableMap; + +import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.io.BaseEncoding; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics.MissReason; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; import java.io.PrintStream; @@ -32,6 +40,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import javax.annotation.Nullable; /** @@ -84,31 +93,148 @@ final class Entry { private Map mdMap; private byte[] digest; private final byte[] usedClientEnvDigest; + private final Map outputFileMetadata; + private final Map outputTreeMetadata; + + /** + * The metadata for output tree that can be serialized. + * + *

We can't serialize {@link TreeArtifactValue} directly as it contains some objects that we + * don't want to serialize, e.g. {@link SpecialArtifact}. + */ + @AutoValue + public abstract static class SerializableTreeArtifactValue { + public static SerializableTreeArtifactValue create( + ImmutableMap childValues, + Optional archivedFileValue) { + return new AutoValue_ActionCache_Entry_SerializableTreeArtifactValue( + childValues, archivedFileValue); + } + + /** + * Creates {@link SerializableTreeArtifactValue} from {@link TreeArtifactValue} by collecting + * children and archived artifact which are remote. + * + *

If no remote value, {@link Optional#empty} is returned. + */ + public static Optional createSerializable( + TreeArtifactValue treeMetadata) { + ImmutableMap childValues = + treeMetadata.getChildValues().entrySet().stream() + // Only save remote tree file + .filter(e -> e.getValue().isRemote()) + .collect( + toImmutableMap( + e -> e.getKey().getTreeRelativePathString(), + e -> (RemoteFileArtifactValue) e.getValue())); + + // Only save remote archived artifact + Optional archivedFileValue = + treeMetadata + .getArchivedRepresentation() + .filter(ar -> ar.archivedFileValue().isRemote()) + .map(ar -> (RemoteFileArtifactValue) ar.archivedFileValue()); + + if (childValues.isEmpty() && !archivedFileValue.isPresent()) { + return Optional.empty(); + } + + return Optional.of(SerializableTreeArtifactValue.create(childValues, archivedFileValue)); + } + + // A map from parentRelativePath to the file metadata + public abstract ImmutableMap childValues(); + + public abstract Optional archivedFileValue(); + } public Entry(String key, Map usedClientEnv, boolean discoversInputs) { actionKey = key; this.usedClientEnvDigest = MetadataDigestUtils.fromEnv(usedClientEnv); files = discoversInputs ? new ArrayList() : null; mdMap = new HashMap<>(); + outputFileMetadata = new HashMap<>(); + outputTreeMetadata = new HashMap<>(); } public Entry( - String key, byte[] usedClientEnvDigest, @Nullable List files, byte[] digest) { + String key, + byte[] usedClientEnvDigest, + @Nullable List files, + byte[] digest, + Map outputFileMetadata, + Map outputTreeMetadata) { actionKey = key; this.usedClientEnvDigest = usedClientEnvDigest; this.files = files; this.digest = digest; mdMap = null; + this.outputFileMetadata = outputFileMetadata; + this.outputTreeMetadata = outputTreeMetadata; } - /** - * Adds the artifact, specified by the executable relative path and its metadata into the cache - * entry. - */ - public void addFile(PathFragment relativePath, FileArtifactValue md, boolean saveExecPath) { - Preconditions.checkState(mdMap != null); - Preconditions.checkState(!isCorrupted()); - Preconditions.checkState(digest == null); + /** Adds metadata of an output file */ + public void addOutputFile(Artifact output, FileArtifactValue value, boolean saveFileMetadata) { + checkArgument( + !output.isTreeArtifact() && !output.isChildOfDeclaredDirectory(), + "Must use addOutputTree to save tree artifacts and their children: %s", + output); + checkState(mdMap != null); + checkState(!isCorrupted()); + checkState(digest == null); + + String execPath = output.getExecPathString(); + // Only save remote file metadata + if (saveFileMetadata && value.isRemote()) { + outputFileMetadata.put(execPath, (RemoteFileArtifactValue) value); + } + mdMap.put(execPath, value); + } + + /** Gets metadata of an output file */ + @Nullable + public RemoteFileArtifactValue getOutputFile(Artifact output) { + checkState(!isCorrupted()); + return outputFileMetadata.get(output.getExecPathString()); + } + + Map getOutputFiles() { + return outputFileMetadata; + } + + /** Adds metadata of an output tree */ + public void addOutputTree( + SpecialArtifact output, TreeArtifactValue metadata, boolean saveTreeMetadata) { + checkArgument(output.isTreeArtifact(), "artifact must be a tree artifact: %s", output); + checkState(mdMap != null); + checkState(!isCorrupted()); + checkState(digest == null); + + String execPath = output.getExecPathString(); + if (saveTreeMetadata) { + SerializableTreeArtifactValue.createSerializable(metadata) + .ifPresent(value -> outputTreeMetadata.put(execPath, value)); + } + mdMap.put(execPath, metadata.getMetadata()); + } + + /** Gets metadata of an output tree */ + @Nullable + public SerializableTreeArtifactValue getOutputTree(SpecialArtifact output) { + checkState(!isCorrupted()); + return outputTreeMetadata.get(output.getExecPathString()); + } + + Map getOutputTrees() { + return outputTreeMetadata; + } + + /** Adds metadata of an input file */ + public void addInputFile( + PathFragment relativePath, FileArtifactValue md, boolean saveExecPath) { + checkState(mdMap != null); + checkState(!isCorrupted()); + checkState(digest == null); String execPath = relativePath.getPathString(); if (discoversInputs() && saveExecPath) { @@ -117,8 +243,8 @@ public void addFile(PathFragment relativePath, FileArtifactValue md, boolean sav mdMap.put(execPath, md); } - public void addFile(PathFragment relativePath, FileArtifactValue md) { - addFile(relativePath, md, /* saveExecPath= */ true); + public void addInputFile(PathFragment relativePath, FileArtifactValue md) { + addInputFile(relativePath, md, /*saveExecPath=*/ true); } /** @@ -197,6 +323,21 @@ public String toString() { builder.append(" ").append(info).append("\n"); } } + + for (Map.Entry entry : outputFileMetadata.entrySet()) { + builder + .append(" ") + .append(entry.getKey()) + .append(" = ") + .append(entry.getValue()) + .append("\n"); + } + + for (Map.Entry entry : outputTreeMetadata.entrySet()) { + SerializableTreeArtifactValue metadata = entry.getValue(); + builder.append(" ").append(entry.getKey()).append(" = ").append(metadata).append("\n"); + } + return builder.toString(); } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java index 1d7a0bba14b1ea..d9c71867f38793 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java @@ -20,6 +20,8 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import com.google.common.flogger.GoogleLogger; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; +import com.google.devtools.build.lib.actions.cache.ActionCache.Entry.SerializableTreeArtifactValue; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics.MissReason; import com.google.devtools.build.lib.clock.Clock; @@ -46,6 +48,7 @@ import java.util.EnumMap; import java.util.HashMap; import java.util.Map; +import java.util.Optional; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -69,7 +72,7 @@ public class CompactPersistentActionCache implements ActionCache { private static final int NO_INPUT_DISCOVERY_COUNT = -1; - private static final int VERSION = 12; + private static final int VERSION = 13; private static final class ActionMap extends PersistentMap { private final Clock clock; @@ -131,8 +134,7 @@ protected Integer readKey(DataInputStream in) throws IOException { } @Override - protected byte[] readValue(DataInputStream in) - throws IOException { + protected byte[] readValue(DataInputStream in) throws IOException { int size = in.readInt(); if (size < 0) { throw new IOException("found negative array size: " + size); @@ -143,8 +145,7 @@ protected byte[] readValue(DataInputStream in) } @Override - protected void writeKey(Integer key, DataOutputStream out) - throws IOException { + protected void writeKey(Integer key, DataOutputStream out) throws IOException { out.writeInt(key); } @@ -155,8 +156,7 @@ protected void writeKey(Integer key, DataOutputStream out) // need to change VERSION to different number every time we touch those // methods. Especially when we'll start to add stuff like statistics for // each action. - protected void writeValue(byte[] value, DataOutputStream out) - throws IOException { + protected void writeValue(byte[] value, DataOutputStream out) throws IOException { out.writeInt(value.length); out.write(value); } @@ -274,17 +274,17 @@ private static CompactPersistentActionCache logAndThrowOrRecurse( } /** - * Rename corrupted files so they could be analyzed later. This would also ensure - * that next initialization attempt will create empty cache. + * Rename corrupted files so they could be analyzed later. This would also ensure that next + * initialization attempt will create empty cache. */ private static void renameCorruptedFiles(Path cacheRoot) { try { - for (Path path : UnixGlob.forPath(cacheRoot).addPattern("action_*_v" + VERSION + ".*") - .glob()) { + for (Path path : + UnixGlob.forPath(cacheRoot).addPattern("action_*_v" + VERSION + ".*").glob()) { path.renameTo(path.getParentDirectory().getChild(path.getBaseName() + ".bad")); } - for (Path path : UnixGlob.forPath(cacheRoot).addPattern("filename_*_v" + VERSION + ".*") - .glob()) { + for (Path path : + UnixGlob.forPath(cacheRoot).addPattern("filename_*_v" + VERSION + ".*").glob()) { path.renameTo(path.getParentDirectory().getChild(path.getBaseName() + ".bad")); } } catch (UnixGlob.BadPattern ex) { @@ -339,7 +339,7 @@ public ActionCache.Entry get(String key) { data = map.get(index); } try { - return data != null ? CompactPersistentActionCache.decode(indexer, data) : null; + return data != null ? decode(indexer, data) : null; } catch (IOException e) { // return entry marked as corrupted. return ActionCache.Entry.CORRUPTED; @@ -350,7 +350,13 @@ public ActionCache.Entry get(String key) { public void put(String key, ActionCache.Entry entry) { // Encode record. Note that both methods may create new mappings in the indexer. int index = indexer.getOrCreateIndex(key); - byte[] content = encode(indexer, entry); + byte[] content; + try { + content = encode(indexer, entry); + } catch (IOException e) { + logger.atWarning().withCause(e).log("Failed to save cache entry %s with key %s", entry, key); + return; + } // Update validation record. ByteBuffer buffer = ByteBuffer.allocate(4); // size of int in bytes @@ -393,16 +399,24 @@ public synchronized String toString() { builder.append("Action cache (" + (map.size() - 1) + " records):\n"); int size = map.size() > 1000 ? 10 : map.size(); int ct = 0; - for (Map.Entry entry: map.entrySet()) { - if (entry.getKey() == VALIDATION_KEY) { continue; } + for (Map.Entry entry : map.entrySet()) { + if (entry.getKey() == VALIDATION_KEY) { + continue; + } String content; try { content = decode(indexer, entry.getValue()).toString(); } catch (IOException e) { content = e + "\n"; } - builder.append("-> ").append(indexer.getStringForIndex(entry.getKey())).append("\n") - .append(content).append(" packed_len = ").append(entry.getValue().length).append("\n"); + builder + .append("-> ") + .append(indexer.getStringForIndex(entry.getKey())) + .append("\n") + .append(content) + .append(" packed_len = ") + .append(entry.getValue().length) + .append("\n"); if (++ct > size) { builder.append("..."); break; @@ -411,76 +425,172 @@ public synchronized String toString() { return builder.toString(); } - /** - * Dumps action cache content. - */ + /** Dumps action cache content. */ @Override public synchronized void dump(PrintStream out) { out.println("String indexer content:\n"); out.println(indexer); out.println("Action cache (" + map.size() + " records):\n"); - for (Map.Entry entry: map.entrySet()) { - if (entry.getKey() == VALIDATION_KEY) { continue; } + for (Map.Entry entry : map.entrySet()) { + if (entry.getKey() == VALIDATION_KEY) { + continue; + } String content; try { - content = CompactPersistentActionCache.decode(indexer, entry.getValue()).toString(); + content = decode(indexer, entry.getValue()).toString(); } catch (IOException e) { content = e + "\n"; } - out.println(entry.getKey() + ", " + indexer.getStringForIndex(entry.getKey()) + ":\n" - + content + "\n packed_len = " + entry.getValue().length + "\n"); + out.println( + entry.getKey() + + ", " + + indexer.getStringForIndex(entry.getKey()) + + ":\n" + + content + + "\n packed_len = " + + entry.getValue().length + + "\n"); } } - /** - * @return action data encoded as a byte[] array. - */ - private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) { + private static void encodeRemoteMetadata( + RemoteFileArtifactValue value, StringIndexer indexer, ByteArrayOutputStream sink) + throws IOException { + MetadataDigestUtils.write(value.getDigest(), sink); + + VarInt.putVarLong(value.getSize(), sink); + + VarInt.putVarInt(value.getLocationIndex(), sink); + + VarInt.putVarInt(indexer.getOrCreateIndex(value.getActionId()), sink); + } + + private static final int MAX_REMOTE_METADATA_SIZE = + DigestUtils.ESTIMATED_SIZE + + VarInt.MAX_VARLONG_SIZE + + VarInt.MAX_VARINT_SIZE + + VarInt.MAX_VARINT_SIZE; + + private static RemoteFileArtifactValue decodeRemoteMetadata( + StringIndexer indexer, ByteBuffer source) throws IOException { + byte[] digest = MetadataDigestUtils.read(source); + + long size = VarInt.getVarLong(source); + + int locationIndex = VarInt.getVarInt(source); + + String actionId = getStringForIndex(indexer, VarInt.getVarInt(source)); + + return new RemoteFileArtifactValue(digest, size, locationIndex, actionId); + } + + /** @return action data encoded as a byte[] array. */ + private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) throws IOException { Preconditions.checkState(!entry.isCorrupted()); - try { - byte[] actionKeyBytes = entry.getActionKey().getBytes(ISO_8859_1); - Collection files = entry.getPaths(); - - // Estimate the size of the buffer: - // 5 bytes max for the actionKey length - // + the actionKey itself - // + 32 bytes for the digest - // + 5 bytes max for the file list length - // + 5 bytes max for each file id - // + 32 bytes for the environment digest - int maxSize = - VarInt.MAX_VARINT_SIZE - + actionKeyBytes.length - + DigestUtils.ESTIMATED_SIZE - + VarInt.MAX_VARINT_SIZE - + files.size() * VarInt.MAX_VARINT_SIZE - + DigestUtils.ESTIMATED_SIZE; - ByteArrayOutputStream sink = new ByteArrayOutputStream(maxSize); - - VarInt.putVarInt(actionKeyBytes.length, sink); - sink.write(actionKeyBytes); - - MetadataDigestUtils.write(entry.getFileDigest(), sink); - - VarInt.putVarInt(entry.discoversInputs() ? files.size() : NO_INPUT_DISCOVERY_COUNT, sink); - for (String file : files) { - VarInt.putVarInt(indexer.getOrCreateIndex(file), sink); + byte[] actionKeyBytes = entry.getActionKey().getBytes(ISO_8859_1); + Collection files = entry.getPaths(); + + int maxOutputFilesSize = + VarInt.MAX_VARINT_SIZE // entry.getOutputFiles().size() + + (VarInt.MAX_VARINT_SIZE // execPath + + MAX_REMOTE_METADATA_SIZE) + * entry.getOutputFiles().size(); + + int maxOutputTreesSize = VarInt.MAX_VARINT_SIZE; // entry.getOutputTrees().size() + for (Map.Entry tree : + entry.getOutputTrees().entrySet()) { + maxOutputTreesSize += VarInt.MAX_VARINT_SIZE; // execPath + + SerializableTreeArtifactValue value = tree.getValue(); + + maxOutputTreesSize += VarInt.MAX_VARINT_SIZE; // value.childValues().size() + maxOutputTreesSize += + (VarInt.MAX_VARINT_SIZE // parentRelativePath + + MAX_REMOTE_METADATA_SIZE) + * value.childValues().size(); + + maxOutputTreesSize += VarInt.MAX_VARINT_SIZE; // value.archivedFileValue() optional + maxOutputTreesSize += + value.archivedFileValue().map(ignored -> MAX_REMOTE_METADATA_SIZE).orElse(0); + } + + // Estimate the size of the buffer: + // 5 bytes max for the actionKey length + // + the actionKey itself + // + 32 bytes for the digest + // + 5 bytes max for the file list length + // + 5 bytes max for each file id + // + 32 bytes for the environment digest + // + max bytes for output files + // + max bytes for output trees + int maxSize = + VarInt.MAX_VARINT_SIZE + + actionKeyBytes.length + + DigestUtils.ESTIMATED_SIZE + + VarInt.MAX_VARINT_SIZE + + files.size() * VarInt.MAX_VARINT_SIZE + + DigestUtils.ESTIMATED_SIZE + + maxOutputFilesSize + + maxOutputTreesSize; + ByteArrayOutputStream sink = new ByteArrayOutputStream(maxSize); + + VarInt.putVarInt(actionKeyBytes.length, sink); + sink.write(actionKeyBytes); + + MetadataDigestUtils.write(entry.getFileDigest(), sink); + + VarInt.putVarInt(entry.discoversInputs() ? files.size() : NO_INPUT_DISCOVERY_COUNT, sink); + for (String file : files) { + VarInt.putVarInt(indexer.getOrCreateIndex(file), sink); + } + + MetadataDigestUtils.write(entry.getUsedClientEnvDigest(), sink); + + VarInt.putVarInt(entry.getOutputFiles().size(), sink); + for (Map.Entry file : entry.getOutputFiles().entrySet()) { + VarInt.putVarInt(indexer.getOrCreateIndex(file.getKey()), sink); + encodeRemoteMetadata(file.getValue(), indexer, sink); + } + + VarInt.putVarInt(entry.getOutputTrees().size(), sink); + for (Map.Entry tree : + entry.getOutputTrees().entrySet()) { + VarInt.putVarInt(indexer.getOrCreateIndex(tree.getKey()), sink); + + SerializableTreeArtifactValue serializableTreeArtifactValue = tree.getValue(); + + VarInt.putVarInt(serializableTreeArtifactValue.childValues().size(), sink); + for (Map.Entry child : + serializableTreeArtifactValue.childValues().entrySet()) { + VarInt.putVarInt(indexer.getOrCreateIndex(child.getKey()), sink); + encodeRemoteMetadata(child.getValue(), indexer, sink); } - MetadataDigestUtils.write(entry.getUsedClientEnvDigest(), sink); + Optional archivedFileValue = + serializableTreeArtifactValue.archivedFileValue(); + if (archivedFileValue.isPresent()) { + VarInt.putVarInt(1, sink); + encodeRemoteMetadata(archivedFileValue.get(), indexer, sink); + } else { + VarInt.putVarInt(0, sink); + } + } - return sink.toByteArray(); - } catch (IOException e) { - // This Exception can never be thrown by ByteArrayOutputStream. - throw new AssertionError(e); + return sink.toByteArray(); + } + + private static String getStringForIndex(StringIndexer indexer, int index) throws IOException { + String path = (index >= 0 ? indexer.getStringForIndex(index) : null); + if (path == null) { + throw new IOException("Corrupted string index"); } + return path; } /** - * Creates new action cache entry using given compressed entry data. Data - * will stay in the compressed format until entry is actually used by the - * dependency checker. + * Creates new action cache entry using given compressed entry data. Data will stay in the + * compressed format until entry is actually used by the dependency checker. */ private static ActionCache.Entry decode(StringIndexer indexer, byte[] data) throws IOException { try { @@ -501,10 +611,7 @@ private static ActionCache.Entry decode(StringIndexer indexer, byte[] data) thro ImmutableList.Builder builder = ImmutableList.builderWithExpectedSize(count); for (int i = 0; i < count; i++) { int id = VarInt.getVarInt(source); - String filename = (id >= 0 ? indexer.getStringForIndex(id) : null); - if (filename == null) { - throw new IOException("Corrupted file index"); - } + String filename = getStringForIndex(indexer, id); builder.add(filename); } files = builder.build(); @@ -512,10 +619,48 @@ private static ActionCache.Entry decode(StringIndexer indexer, byte[] data) thro byte[] usedClientEnvDigest = MetadataDigestUtils.read(source); + int numOutputFiles = VarInt.getVarInt(source); + Map outputFiles = + Maps.newHashMapWithExpectedSize(numOutputFiles); + for (int i = 0; i < numOutputFiles; i++) { + String execPath = getStringForIndex(indexer, VarInt.getVarInt(source)); + RemoteFileArtifactValue value = decodeRemoteMetadata(indexer, source); + outputFiles.put(execPath, value); + } + + int numOutputTrees = VarInt.getVarInt(source); + Map outputTrees = + Maps.newHashMapWithExpectedSize(numOutputTrees); + for (int i = 0; i < numOutputTrees; i++) { + String treeKey = getStringForIndex(indexer, VarInt.getVarInt(source)); + + ImmutableMap.Builder childValues = ImmutableMap.builder(); + int numChildValues = VarInt.getVarInt(source); + for (int j = 0; j < numChildValues; ++j) { + String childKey = getStringForIndex(indexer, VarInt.getVarInt(source)); + RemoteFileArtifactValue value = decodeRemoteMetadata(indexer, source); + childValues.put(childKey, value); + } + + Optional archivedFileValue = Optional.empty(); + int numArchivedFileValue = VarInt.getVarInt(source); + if (numArchivedFileValue > 0) { + if (numArchivedFileValue != 1) { + throw new IOException("Invalid number of archived artifacts"); + } + archivedFileValue = Optional.of(decodeRemoteMetadata(indexer, source)); + } + + SerializableTreeArtifactValue value = + SerializableTreeArtifactValue.create(childValues.build(), archivedFileValue); + outputTrees.put(treeKey, value); + } + if (source.remaining() > 0) { throw new IOException("serialized entry data has not been fully decoded"); } - return new ActionCache.Entry(actionKey, usedClientEnvDigest, files, digest); + return new ActionCache.Entry( + actionKey, usedClientEnvDigest, files, digest, outputFiles, outputTrees); } catch (BufferUnderflowException e) { throw new IOException("encoded entry data is incomplete", e); } @@ -529,8 +674,10 @@ public void accountHit() { @Override public void accountMiss(MissReason reason) { AtomicInteger counter = misses.get(reason); - Preconditions.checkNotNull(counter, "Miss reason %s was not registered in the misses map " - + "during cache construction", reason); + Preconditions.checkNotNull( + counter, + "Miss reason %s was not registered in the misses map " + "during cache construction", + reason); counter.incrementAndGet(); } diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java index 23c4630c94d182..6b62bd1b32a624 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java @@ -387,6 +387,17 @@ public boolean useTopLevelTargetsForSymlinks() { help = "Whether to use the action cache") public boolean useActionCache; + @Option( + name = "experimental_action_cache_store_output_metadata", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = { + OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION, + OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS + }, + help = "Whether to store output metadata in the action cache") + public boolean actionCacheStoreOutputMetadata; + @Option( name = "discard_actions_after_execution", defaultValue = "true", diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index a4cffc16d3cd7d..0bbe732177c4c7 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -781,7 +781,9 @@ private Builder createBuilder( ActionCacheChecker.CacheConfig.builder() .setEnabled(options.useActionCache) .setVerboseExplanations(options.verboseExplanations) - .build()), + .setStoreOutputMetadata(options.actionCacheStoreOutputMetadata) + .build(), + PathFragment.create(env.getDirectories().getRelativeOutputPath())), env.getTopDownActionCache(), request.getPackageOptions().checkOutputFiles ? modifiedOutputFiles 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 8fe2efd4dd7b7b..5a94b980e062a4 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 @@ -77,7 +77,6 @@ import com.google.devtools.build.lib.remote.common.OperationObserver; import com.google.devtools.build.lib.remote.common.OutputDigestMismatchException; import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; -import com.google.devtools.build.lib.remote.common.RemoteActionFileArtifactValue; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; import com.google.devtools.build.lib.remote.common.RemotePathResolver; @@ -623,12 +622,11 @@ private void injectRemoteArtifact( } metadataInjector.injectFile( output, - new RemoteActionFileArtifactValue( + new RemoteFileArtifactValue( DigestUtil.toBinaryDigest(outputMetadata.digest()), outputMetadata.digest().getSizeBytes(), /*locationIndex=*/ 1, - context.getRequestMetadata().getActionId(), - outputMetadata.isExecutable())); + context.getRequestMetadata().getActionId())); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 1f3a6625d19a15..b7dd80bbe043c0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -406,7 +406,6 @@ public void injectFile(Artifact output, FileArtifactValue metadata) { !output.isTreeArtifact() && !output.isChildOfDeclaredDirectory(), "Tree artifacts and their children must be injected via injectTree: %s", output); - checkState(executionMode.get(), "Tried to inject %s outside of execution", output); store.putArtifactData(output, metadata); } @@ -415,7 +414,6 @@ public void injectFile(Artifact output, FileArtifactValue metadata) { public void injectTree(SpecialArtifact output, TreeArtifactValue tree) { checkArgument(isKnownOutput(output), "%s is not a declared output of this action", output); checkArgument(output.isTreeArtifact(), "Output must be a tree artifact: %s", output); - checkState(executionMode.get(), "Tried to inject %s outside of execution", output); checkArgument( archivedTreeArtifactsEnabled == tree.getArchivedRepresentation().isPresent(), "Archived representation presence mismatched for: %s with archivedTreeArtifactsEnabled: %s", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java index 9b282bb4017687..8590015816cb61 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java @@ -153,12 +153,12 @@ public static MultiBuilder newMultiBuilder() { * metadata for it. */ @AutoValue - abstract static class ArchivedRepresentation { - abstract ArchivedTreeArtifact archivedTreeFileArtifact(); + public abstract static class ArchivedRepresentation { + public abstract ArchivedTreeArtifact archivedTreeFileArtifact(); - abstract FileArtifactValue archivedFileValue(); + public abstract FileArtifactValue archivedFileValue(); - static ArchivedRepresentation create( + public static ArchivedRepresentation create( ArchivedTreeArtifact archivedTreeFileArtifact, FileArtifactValue fileArtifactValue) { return new AutoValue_TreeArtifactValue_ArchivedRepresentation( archivedTreeFileArtifact, fileArtifactValue); @@ -225,7 +225,7 @@ public long getTotalChildBytes() { } /** Return archived representation of the tree artifact (if present). */ - Optional getArchivedRepresentation() { + public Optional getArchivedRepresentation() { return Optional.ofNullable(archivedRepresentation); } @@ -304,7 +304,8 @@ public String toString() { * A TreeArtifactValue that represents a missing TreeArtifact. This is occasionally useful because * Java's concurrent collections disallow null members. */ - static final TreeArtifactValue MISSING_TREE_ARTIFACT = createMarker("MISSING_TREE_ARTIFACT"); + public static final TreeArtifactValue MISSING_TREE_ARTIFACT = + createMarker("MISSING_TREE_ARTIFACT"); private static TreeArtifactValue createMarker(String toStringRepresentation) { return new TreeArtifactValue( @@ -488,7 +489,7 @@ public Builder setArchivedRepresentation( ArchivedRepresentation.create(archivedTreeArtifact, metadata)); } - private Builder setArchivedRepresentation(ArchivedRepresentation archivedRepresentation) { + public Builder setArchivedRepresentation(ArchivedRepresentation archivedRepresentation) { checkState( this.archivedRepresentation == null, "Tried to add 2 archived representations for: %s", diff --git a/src/main/java/com/google/devtools/build/lib/util/VarInt.java b/src/main/java/com/google/devtools/build/lib/util/VarInt.java index 70f26daf101484..d5d77f9b1e68ef 100644 --- a/src/main/java/com/google/devtools/build/lib/util/VarInt.java +++ b/src/main/java/com/google/devtools/build/lib/util/VarInt.java @@ -283,4 +283,11 @@ public static void putVarLong(long v, ByteBuffer sink) { sink.put((byte) (bits | 0x80)); } } + + public static void putVarLong(long v, OutputStream outputStream) throws IOException { + byte[] bytes = new byte[varLongSize(v)]; + ByteBuffer sink = ByteBuffer.wrap(bytes); + putVarLong(v, sink); + outputStream.write(bytes); + } } diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java index 3cbf6341296b4f..ad132133020e58 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java @@ -15,8 +15,14 @@ package com.google.devtools.build.lib.actions; import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.NULL_ARTIFACT_OWNER; +import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.createArtifact; +import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.createTreeArtifactWithGeneratingAction; +import static com.google.devtools.build.lib.vfs.FileSystemUtils.writeIsoLatin1; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; -import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionCacheChecker.Token; @@ -24,7 +30,9 @@ import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.cache.ActionCache; +import com.google.devtools.build.lib.actions.cache.ActionCache.Entry.SerializableTreeArtifactValue; import com.google.devtools.build.lib.actions.cache.CompactPersistentActionCache; import com.google.devtools.build.lib.actions.cache.MetadataHandler; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics; @@ -40,10 +48,12 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.events.NullEventHandler; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; @@ -52,9 +62,12 @@ import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.IOException; import java.io.PrintStream; +import java.util.ArrayDeque; +import java.util.Deque; import java.util.HashMap; import java.util.HashSet; import java.util.Map; +import java.util.Optional; import java.util.Set; import javax.annotation.Nullable; import org.junit.After; @@ -68,17 +81,41 @@ public class ActionCacheCheckerTest { private CorruptibleActionCache cache; private ActionCacheChecker cacheChecker; private Set filesToDelete; + private DigestHashFunction digestHashFunction; + private FileSystem fileSystem; + private ArtifactRoot artifactRoot; + private PathFragment derivedPathPrefix; @Before public void setupCache() throws Exception { Scratch scratch = new Scratch(); Clock clock = new ManualClock(); - ArtifactResolver artifactResolver = new FakeArtifactResolverBase(); cache = new CorruptibleActionCache(scratch.resolve("/cache/test.dat"), clock); - cacheChecker = - new ActionCacheChecker( - cache, artifactResolver, new ActionKeyContext(), Predicates.alwaysTrue(), null); + derivedPathPrefix = PathFragment.create("bin"); + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ false); + digestHashFunction = DigestHashFunction.SHA256; + fileSystem = new InMemoryFileSystem(digestHashFunction); + Path execRoot = fileSystem.getPath("/output"); + artifactRoot = ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, "bin"); + } + + private byte[] digest(byte[] content) { + return digestHashFunction.getHashFunction().hashBytes(content).asBytes(); + } + + private ActionCacheChecker createActionCacheChecker(boolean storeOutputMetadata) { + return new ActionCacheChecker( + cache, + new FakeArtifactResolverBase(), + new ActionKeyContext(), + action -> true, + ActionCacheChecker.CacheConfig.builder() + .setEnabled(true) + .setVerboseExplanations(false) + .setStoreOutputMetadata(storeOutputMetadata) + .build(), + derivedPathPrefix); } @Before @@ -89,7 +126,11 @@ public void clearFilesToDeleteAfterTest() throws Exception { @After public void deleteFilesCreatedDuringTest() throws Exception { for (Path path : filesToDelete) { - path.delete(); + if (path.isDirectory()) { + path.deleteTree(); + } else { + path.delete(); + } } } @@ -98,6 +139,10 @@ private void runAction(Action action) throws Exception { runAction(action, new HashMap<>()); } + private void runAction(Action action, MetadataHandler metadataHandler) throws Exception { + runAction(action, new HashMap<>(), ImmutableMap.of(), metadataHandler); + } + /** * "Executes" the given action from the point of view of the cache's lifecycle with a custom * client environment. @@ -108,8 +153,15 @@ private void runAction(Action action, Map clientEnv) throws Exce private void runAction(Action action, Map clientEnv, Map platform) throws Exception { - MetadataHandler metadataHandler = new FakeMetadataHandler(); + runAction(action, clientEnv, platform, new FakeMetadataHandler(derivedPathPrefix)); + } + private void runAction( + Action action, + Map clientEnv, + Map platform, + MetadataHandler metadataHandler) + throws Exception { for (Artifact artifact : action.getOutputs()) { Path path = artifact.getPath(); @@ -119,8 +171,9 @@ private void runAction(Action action, Map clientEnv, Map clientEnv, Map getClientEnvironmentVariables() { return ImmutableList.of("used-var"); @@ -255,7 +312,7 @@ public ImmutableList getClientEnvironmentVariables() { @Test public void testDifferentRemoteDefaultPlatform() throws Exception { - Action action = new NullAction(); + Action action = new WriteEmptyOutputAction(); Map env = new HashMap<>(); env.put("unused-var", "1"); @@ -285,9 +342,9 @@ public void testDifferentRemoteDefaultPlatform() throws Exception { @Test public void testDifferentFiles() throws Exception { - Action action = new NullAction(); + Action action = new WriteEmptyOutputAction(); runAction(action); // Not cached. - FileSystemUtils.writeContentAsLatin1(action.getPrimaryOutput().getPath(), "modified"); + writeContentAsLatin1(action.getPrimaryOutput().getPath(), "modified"); runAction(action); // Cache miss because output files were modified. assertStatistics( @@ -300,17 +357,18 @@ public void testDifferentFiles() throws Exception { @Test public void testUnconditionalExecution() throws Exception { - Action action = new NullAction() { - @Override - public boolean executeUnconditionally() { - return true; - } + Action action = + new WriteEmptyOutputAction() { + @Override + public boolean executeUnconditionally() { + return true; + } - @Override - public boolean isVolatile() { - return true; - } - }; + @Override + public boolean isVolatile() { + return true; + } + }; int runs = 5; for (int i = 0; i < runs; i++) { @@ -350,9 +408,9 @@ public synchronized NestedSet getInputs() { } }; runAction(action); // Not cached so recorded as different deps. - FileSystemUtils.writeContentAsLatin1(action.getPrimaryInput().getPath(), "modified"); + writeContentAsLatin1(action.getPrimaryInput().getPath(), "modified"); runAction(action); // Cache miss because input files were modified. - FileSystemUtils.writeContentAsLatin1(action.getPrimaryOutput().getPath(), "modified"); + writeContentAsLatin1(action.getPrimaryOutput().getPath(), "modified"); runAction(action); // Outputs are not considered for middleman actions, so this is a cache hit. runAction(action); // Outputs are not considered for middleman actions, so this is a cache hit. @@ -368,29 +426,525 @@ public synchronized NestedSet getInputs() { public void testDeletedConstantMetadataOutputCausesReexecution() throws Exception { SpecialArtifact output = new Artifact.SpecialArtifact( - ArtifactRoot.asDerivedRoot( - new InMemoryFileSystem(DigestHashFunction.SHA256).getPath("/output"), - RootType.Output, - "bin"), + artifactRoot, PathFragment.create("bin/dummy"), - ActionsTestUtil.NULL_ARTIFACT_OWNER, + NULL_ARTIFACT_OWNER, SpecialArtifactType.CONSTANT_METADATA); output.getPath().getParentDirectory().createDirectoryAndParents(); - Action action = new NullAction(output); + Action action = new WriteEmptyOutputAction(output); runAction(action); output.getPath().delete(); assertThat( cacheChecker.getTokenIfNeedToExecute( action, - null, - ImmutableMap.of(), - null, - new FakeMetadataHandler(), - null, - ImmutableMap.of())) + /*resolvedCacheArtifacts=*/ null, + /*clientEnv=*/ ImmutableMap.of(), + /*handler=*/ null, + new FakeMetadataHandler(derivedPathPrefix), + /*artifactExpander=*/ null, + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of())) .isNotNull(); } + private RemoteFileArtifactValue createRemoteFileMetadata(String content) { + byte[] bytes = content.getBytes(UTF_8); + return new RemoteFileArtifactValue(digest(bytes), bytes.length, 1, "action-id"); + } + + private TreeArtifactValue createTreeMetadata( + SpecialArtifact parent, + ImmutableMap children, + Optional archivedArtifactValue) { + TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder(parent); + for (Map.Entry entry : children.entrySet()) { + builder.putChild( + Artifact.TreeFileArtifact.createTreeOutput(parent, entry.getKey()), entry.getValue()); + } + archivedArtifactValue.ifPresent( + metadata -> { + Artifact.ArchivedTreeArtifact artifact = + Artifact.ArchivedTreeArtifact.create(parent, derivedPathPrefix); + builder.setArchivedRepresentation( + TreeArtifactValue.ArchivedRepresentation.create(artifact, metadata)); + }); + return builder.build(); + } + + @Test + public void saveOutputMetadata_remoteFileMetadataSaved() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + Artifact output = createArtifact(artifactRoot, "bin/dummy"); + String content = "content"; + Action action = new InjectOutputFileMetadataAction(output, createRemoteFileMetadata(content)); + + // Not cached. + runAction(action); + + assertThat(output.getPath().exists()).isFalse(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputFile(output)).isEqualTo(createRemoteFileMetadata(content)); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + } + + @Test + public void saveOutputMetadata_localFileMetadataNotSaved() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + Artifact output = createArtifact(artifactRoot, "bin/dummy"); + Action action = new WriteEmptyOutputAction(output); + output.getPath().delete(); + + runAction(action); + + assertThat(output.getPath().exists()).isTrue(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputFile(output)).isNull(); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + } + + @Test + public void saveOutputMetadata_remoteMetadataInjectedAndLocalFilesStored() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + Artifact output = createArtifact(artifactRoot, "bin/dummy"); + Action action = + new WriteEmptyOutputAction(output) { + @Override + public ActionResult execute(ActionExecutionContext actionExecutionContext) { + actionExecutionContext + .getMetadataHandler() + .injectFile(output, createRemoteFileMetadata("")); + return super.execute(actionExecutionContext); + } + }; + output.getPath().delete(); + + runAction(action); + + assertThat(output.getPath().exists()).isTrue(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputFile(output)).isEqualTo(createRemoteFileMetadata("")); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + } + + @Test + public void saveOutputMetadata_notSavedIfDisabled() throws Exception { + Artifact output = createArtifact(artifactRoot, "bin/dummy"); + String content = "content"; + Action action = new InjectOutputFileMetadataAction(output, createRemoteFileMetadata(content)); + + runAction(action); + + assertThat(output.getPath().exists()).isFalse(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputFile(output)).isNull(); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + } + + @Test + public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + Artifact output = createArtifact(artifactRoot, "bin/dummy"); + String content = "content"; + Action action = new InjectOutputFileMetadataAction(output, createRemoteFileMetadata(content)); + MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + + runAction(action); + Token token = + cacheChecker.getTokenIfNeedToExecute( + action, + /*resolvedCacheArtifacts=*/ null, + /*clientEnv=*/ ImmutableMap.of(), + /*handler=*/ null, + metadataHandler, + /*artifactExpander=*/ null, + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()); + + assertThat(output.getPath().exists()).isFalse(); + assertThat(token).isNull(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputFile(output)).isEqualTo(createRemoteFileMetadata(content)); + assertThat(metadataHandler.getMetadata(output)).isEqualTo(createRemoteFileMetadata(content)); + } + + @Test + public void saveOutputMetadata_localMetadataIsSameAsRemoteMetadata_cached() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + Artifact output = createArtifact(artifactRoot, "bin/dummy"); + String content = "content"; + Action action = new InjectOutputFileMetadataAction(output, createRemoteFileMetadata(content)); + runAction(action); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + + writeContentAsLatin1(output.getPath(), content); + // Cached since local metadata is same as remote metadata + runAction(action); + + assertStatistics(1, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputFile(output)).isEqualTo(createRemoteFileMetadata(content)); + } + + @Test + public void saveOutputMetadata_localMetadataIsDifferentFromRemoteMetadata_notCached() + throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + Artifact output = createArtifact(artifactRoot, "bin/dummy"); + String content1 = "content1"; + String content2 = "content2"; + Action action = + new InjectOutputFileMetadataAction( + output, createRemoteFileMetadata(content1), createRemoteFileMetadata(content2)); + runAction(action); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + + writeContentAsLatin1(output.getPath(), content2); + // Not cached since local file changed + runAction(action); + + assertStatistics( + 0, + new MissDetailsBuilder() + .set(MissReason.NOT_CACHED, 1) + .set(MissReason.DIFFERENT_FILES, 1) + .build()); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputFile(output)).isEqualTo(createRemoteFileMetadata(content2)); + } + + @Test + public void saveOutputMetadata_treeMetadata_remoteFileMetadataSaved() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + ImmutableMap children = + ImmutableMap.of( + "file1", createRemoteFileMetadata("content1"), + "file2", createRemoteFileMetadata("content2")); + Action action = + new InjectOutputTreeMetadataAction( + output, createTreeMetadata(output, children, Optional.empty())); + + runAction(action); + + assertThat(output.getPath().exists()).isFalse(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputTree(output)) + .isEqualTo(SerializableTreeArtifactValue.create(children, Optional.empty())); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + } + + @Test + public void saveOutputMetadata_treeMetadata_remoteArchivedArtifactSaved() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + Action action = + new InjectOutputTreeMetadataAction( + output, + createTreeMetadata( + output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("content")))); + + runAction(action); + + assertThat(output.getPath().exists()).isFalse(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputTree(output)) + .isEqualTo( + SerializableTreeArtifactValue.create( + ImmutableMap.of(), Optional.of(createRemoteFileMetadata("content")))); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + } + + @Test + public void saveOutputMetadata_emptyTreeMetadata_notSaved() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + Action action = + new InjectOutputTreeMetadataAction( + output, createTreeMetadata(output, ImmutableMap.of(), Optional.empty())); + MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + + runAction(action); + Token token = + cacheChecker.getTokenIfNeedToExecute( + action, + /*resolvedCacheArtifacts=*/ null, + /*clientEnv=*/ ImmutableMap.of(), + /*handler=*/ null, + metadataHandler, + /*artifactExpander=*/ null, + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()); + + assertThat(token).isNull(); + assertThat(output.getPath().exists()).isFalse(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputTree(output)).isNull(); + assertStatistics(1, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + } + + @Test + public void saveOutputMetadata_treeMetadata_localFileMetadataNotSaved() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + writeIsoLatin1(fileSystem.getPath("/file2"), ""); + ImmutableMap children = + ImmutableMap.of( + "file1", createRemoteFileMetadata("content1"), + "file2", FileArtifactValue.createForTesting(fileSystem.getPath("/file2"))); + fileSystem.getPath("/file2").delete(); + Action action = + new InjectOutputTreeMetadataAction( + output, createTreeMetadata(output, children, Optional.empty())); + + runAction(action); + + assertThat(output.getPath().exists()).isFalse(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputTree(output)) + .isEqualTo( + SerializableTreeArtifactValue.create( + ImmutableMap.of("file1", createRemoteFileMetadata("content1")), Optional.empty())); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + } + + @Test + public void saveOutputMetadata_treeMetadata_localArchivedArtifactNotSaved() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + writeIsoLatin1(fileSystem.getPath("/archive"), ""); + Action action = + new InjectOutputTreeMetadataAction( + output, + createTreeMetadata( + output, + ImmutableMap.of(), + Optional.of(FileArtifactValue.createForTesting(fileSystem.getPath("/archive"))))); + fileSystem.getPath("/archive").delete(); + + runAction(action); + + assertThat(output.getPath().exists()).isFalse(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputTree(output)).isNull(); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + } + + @Test + public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + ImmutableMap children = + ImmutableMap.of( + "file1", createRemoteFileMetadata("content1"), + "file2", createRemoteFileMetadata("content2")); + Action action = + new InjectOutputTreeMetadataAction( + output, createTreeMetadata(output, children, Optional.empty())); + MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + + runAction(action); + Token token = + cacheChecker.getTokenIfNeedToExecute( + action, + /*resolvedCacheArtifacts=*/ null, + /*clientEnv=*/ ImmutableMap.of(), + /*handler=*/ null, + metadataHandler, + /*artifactExpander=*/ null, + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()); + + TreeArtifactValue expectedMetadata = createTreeMetadata(output, children, Optional.empty()); + assertThat(token).isNull(); + assertThat(output.getPath().exists()).isFalse(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputTree(output)) + .isEqualTo(SerializableTreeArtifactValue.createSerializable(expectedMetadata).get()); + assertThat(metadataHandler.getTreeArtifactValue(output)).isEqualTo(expectedMetadata); + } + + @Test + public void saveOutputMetadata_treeMetadata_localFileMetadataLoaded() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + ImmutableMap children1 = + ImmutableMap.of( + "file1", createRemoteFileMetadata("content1"), + "file2", createRemoteFileMetadata("content2")); + ImmutableMap children2 = + ImmutableMap.of( + "file1", createRemoteFileMetadata("content1"), + "file2", createRemoteFileMetadata("modified_remote")); + Action action = + new InjectOutputTreeMetadataAction( + output, + createTreeMetadata(output, children1, Optional.empty()), + createTreeMetadata(output, children2, Optional.empty())); + MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + + runAction(action); + writeIsoLatin1(output.getPath().getRelative("file2"), "modified_local"); + // Not cached since local file changed + runAction(action, metadataHandler); + + assertStatistics( + 0, + new MissDetailsBuilder() + .set(MissReason.NOT_CACHED, 1) + .set(MissReason.DIFFERENT_FILES, 1) + .build()); + assertThat(output.getPath().exists()).isTrue(); + TreeArtifactValue expectedMetadata = + createTreeMetadata( + output, + ImmutableMap.of( + "file1", createRemoteFileMetadata("content1"), + "file2", createRemoteFileMetadata("modified_remote")), + Optional.empty()); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputTree(output)) + .isEqualTo(SerializableTreeArtifactValue.createSerializable(expectedMetadata).get()); + assertThat(metadataHandler.getTreeArtifactValue(output)).isEqualTo(expectedMetadata); + } + + @Test + public void saveOutputMetadata_treeMetadata_localArchivedArtifactLoaded() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + Action action = + new InjectOutputTreeMetadataAction( + output, + createTreeMetadata( + output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("content"))), + createTreeMetadata( + output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("modified")))); + MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + + runAction(action); + writeIsoLatin1( + Artifact.ArchivedTreeArtifact.create(output, derivedPathPrefix).getPath(), "modified"); + // Not cached since local file changed + runAction(action, metadataHandler); + + assertStatistics( + 0, + new MissDetailsBuilder() + .set(MissReason.NOT_CACHED, 1) + .set(MissReason.DIFFERENT_FILES, 1) + .build()); + assertThat(output.getPath().exists()).isFalse(); + TreeArtifactValue expectedMetadata = + createTreeMetadata( + output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("modified"))); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputTree(output)) + .isEqualTo(SerializableTreeArtifactValue.createSerializable(expectedMetadata).get()); + assertThat(metadataHandler.getTreeArtifactValue(output)).isEqualTo(expectedMetadata); + } + + private void writeContentAsLatin1(Path path, String content) throws IOException { + Path parent = path.getParentDirectory(); + if (parent != null) { + parent.createDirectoryAndParents(); + } + FileSystemUtils.writeContentAsLatin1(path, content); + } + + @Test + public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + ImmutableMap children = + ImmutableMap.of( + "file1", createRemoteFileMetadata("content1"), + "file2", createRemoteFileMetadata("content2")); + Action action = + new InjectOutputTreeMetadataAction( + output, createTreeMetadata(output, children, Optional.empty())); + MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + + runAction(action); + writeContentAsLatin1(output.getPath().getRelative("file1"), "content1"); + // Cache hit + Token token = + cacheChecker.getTokenIfNeedToExecute( + action, + /*resolvedCacheArtifacts=*/ null, + /*clientEnv=*/ ImmutableMap.of(), + /*handler=*/ null, + metadataHandler, + /*artifactExpander=*/ null, + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()); + + assertThat(token).isNull(); + assertStatistics(1, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + assertThat(output.getPath().exists()).isTrue(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputTree(output)) + .isEqualTo(SerializableTreeArtifactValue.create(children, Optional.empty())); + assertThat(metadataHandler.getTreeArtifactValue(output)) + .isEqualTo( + createTreeMetadata( + output, + ImmutableMap.of( + "file1", + FileArtifactValue.createForTesting(output.getPath().getRelative("file1")), + "file2", createRemoteFileMetadata("content2")), + Optional.empty())); + } + + @Test + public void saveOutputMetadata_treeMetadataWithSameLocalArchivedArtifact_cached() + throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + Action action = + new InjectOutputTreeMetadataAction( + output, + createTreeMetadata( + output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("content")))); + MetadataHandler metadataHandler = new FakeMetadataHandler(derivedPathPrefix); + + runAction(action); + writeContentAsLatin1( + Artifact.ArchivedTreeArtifact.create(output, derivedPathPrefix).getPath(), "content"); + // Cache hit + runAction(action, metadataHandler); + + assertStatistics(1, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + assertThat(output.getPath().exists()).isFalse(); + TreeArtifactValue expectedMetadata = + createTreeMetadata( + output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("content"))); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputTree(output)) + .isEqualTo(SerializableTreeArtifactValue.createSerializable(expectedMetadata).get()); + assertThat(metadataHandler.getTreeArtifactValue(output)).isEqualTo(expectedMetadata); + } + /** An {@link ActionCache} that allows injecting corruption for testing. */ private static class CorruptibleActionCache implements ActionCache { private final CompactPersistentActionCache delegate; @@ -466,15 +1020,142 @@ public MiddlemanType getActionType() { /** A fake metadata handler that is able to obtain metadata from the file system. */ private static class FakeMetadataHandler extends FakeMetadataHandlerBase { + private final Map fileMetadata = new HashMap<>(); + private final Map treeMetadata = new HashMap<>(); + private final PathFragment derivedPathPrefix; + + FakeMetadataHandler(PathFragment derivedPathPrefix) { + this.derivedPathPrefix = derivedPathPrefix; + } + + @Override + public void injectFile(Artifact output, FileArtifactValue metadata) { + fileMetadata.put(output, metadata); + } + + @Override + public void injectTree(SpecialArtifact treeArtifact, TreeArtifactValue tree) { + treeMetadata.put(treeArtifact, tree); + } + @Override public FileArtifactValue getMetadata(ActionInput input) throws IOException { if (!(input instanceof Artifact)) { return null; } - return FileArtifactValue.createForTesting((Artifact) input); + Artifact output = (Artifact) input; + + if (output.isTreeArtifact()) { + TreeArtifactValue treeArtifactValue = getTreeArtifactValue((SpecialArtifact) output); + if (treeArtifactValue != null) { + return treeArtifactValue.getMetadata(); + } else { + return null; + } + } + + if (fileMetadata.containsKey(output)) { + return fileMetadata.get(output); + } + return FileArtifactValue.createForTesting(output); + } + + @Override + public TreeArtifactValue getTreeArtifactValue(SpecialArtifact output) throws IOException { + if (treeMetadata.containsKey(output)) { + return treeMetadata.get(output); + } + + TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(output); + + Path treeDir = output.getPath(); + if (treeDir.exists()) { + TreeArtifactValue.visitTree( + treeDir, + (parentRelativePath, type) -> { + if (type == Dirent.Type.DIRECTORY) { + return; + } + Artifact.TreeFileArtifact child = + Artifact.TreeFileArtifact.createTreeOutput(output, parentRelativePath); + FileArtifactValue metadata = + FileArtifactValue.createForTesting(treeDir.getRelative(parentRelativePath)); + tree.putChild(child, metadata); + }); + } + + Artifact.ArchivedTreeArtifact archivedTreeArtifact = + Artifact.ArchivedTreeArtifact.create(output, derivedPathPrefix); + if (archivedTreeArtifact.getPath().exists()) { + tree.setArchivedRepresentation( + archivedTreeArtifact, + FileArtifactValue.createForTesting(archivedTreeArtifact.getPath())); + } + + return tree.build(); } @Override public void setDigestForVirtualArtifact(Artifact artifact, byte[] digest) {} } + + private static class WriteEmptyOutputAction extends NullAction { + public WriteEmptyOutputAction() {} + + public WriteEmptyOutputAction(Artifact... outputs) { + super(outputs); + } + + @Override + public ActionResult execute(ActionExecutionContext actionExecutionContext) { + for (Artifact output : getOutputs()) { + Path path = output.getPath(); + if (!path.exists()) { + try { + FileSystemUtils.writeContentAsLatin1(path, ""); + } catch (IOException e) { + throw new IllegalStateException("Failed to create output", e); + } + } + } + + return super.execute(actionExecutionContext); + } + } + + private static class InjectOutputFileMetadataAction extends NullAction { + private final Artifact output; + private final Deque metadataDeque; + + public InjectOutputFileMetadataAction(Artifact output, FileArtifactValue... metadata) { + super(output); + + this.output = output; + this.metadataDeque = new ArrayDeque<>(ImmutableList.copyOf(metadata)); + } + + @Override + public ActionResult execute(ActionExecutionContext actionExecutionContext) { + actionExecutionContext.getMetadataHandler().injectFile(output, metadataDeque.pop()); + return super.execute(actionExecutionContext); + } + } + + private static class InjectOutputTreeMetadataAction extends NullAction { + private final SpecialArtifact output; + private final Deque metadataDeque; + + public InjectOutputTreeMetadataAction(SpecialArtifact output, TreeArtifactValue... metadata) { + super(output); + + this.output = output; + this.metadataDeque = new ArrayDeque<>(ImmutableList.copyOf(metadata)); + } + + @Override + public ActionResult execute(ActionExecutionContext actionExecutionContext) { + actionExecutionContext.getMetadataHandler().injectTree(output, metadataDeque.pop()); + return super.execute(actionExecutionContext); + } + } } diff --git a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java index e2969fc6231d26..b65f59dfd8b149 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java @@ -17,13 +17,24 @@ import static com.google.common.truth.Truth.assertWithMessage; import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; +import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; +import com.google.devtools.build.lib.actions.cache.ActionCache.Entry.SerializableTreeArtifactValue; +import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.events.NullEventHandler; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.testutil.Scratch; +import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import java.util.Optional; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -39,6 +50,8 @@ public class CompactPersistentActionCacheTest { private Path journalFile; private final ManualClock clock = new ManualClock(); private CompactPersistentActionCache cache; + private PathFragment derivedPathPrefix; + private ArtifactRoot artifactRoot; @Before public final void createFiles() throws Exception { @@ -46,6 +59,10 @@ public final void createFiles() throws Exception { cache = CompactPersistentActionCache.create(dataRoot, clock, NullEventHandler.INSTANCE); mapFile = CompactPersistentActionCache.cacheFile(dataRoot); journalFile = CompactPersistentActionCache.journalFile(dataRoot); + derivedPathPrefix = PathFragment.create("bin"); + artifactRoot = + ArtifactRoot.asDerivedRoot( + scratch.getFileSystem().getPath("/output"), ArtifactRoot.RootType.Output, "bin"); } @Test @@ -145,7 +162,7 @@ public void testIncrementalSave() throws IOException { public void testEntryToStringIsIdempotent() { ActionCache.Entry entry = new ActionCache.Entry("actionKey", ImmutableMap.of(), false); entry.toString(); - entry.addFile( + entry.addInputFile( PathFragment.create("foo/bar"), FileArtifactValue.createForDirectoryWithMtime(1234)); entry.toString(); entry.getFileDigest(); @@ -171,6 +188,155 @@ public void testToStringIsntTooBig() { assertToStringIsntTooBig(3000); } + private FileArtifactValue createLocalMetadata(Artifact artifact, String content) + throws IOException { + artifact.getPath().getParentDirectory().createDirectoryAndParents(); + FileSystemUtils.writeContentAsLatin1(artifact.getPath(), content); + return FileArtifactValue.createForTesting(artifact.getPath()); + } + + private RemoteFileArtifactValue createRemoteMetadata(Artifact artifact, String content) { + byte[] bytes = content.getBytes(StandardCharsets.UTF_8); + byte[] digest = + artifact + .getPath() + .getFileSystem() + .getDigestFunction() + .getHashFunction() + .hashBytes(bytes) + .asBytes(); + return new RemoteFileArtifactValue(digest, bytes.length, 1, "action-id"); + } + + private TreeArtifactValue createTreeMetadata( + SpecialArtifact parent, + ImmutableMap children, + Optional archivedArtifactValue) { + TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder(parent); + for (Map.Entry entry : children.entrySet()) { + builder.putChild( + Artifact.TreeFileArtifact.createTreeOutput(parent, entry.getKey()), entry.getValue()); + } + archivedArtifactValue.ifPresent( + metadata -> { + Artifact.ArchivedTreeArtifact artifact = + Artifact.ArchivedTreeArtifact.create(parent, derivedPathPrefix); + builder.setArchivedRepresentation( + TreeArtifactValue.ArchivedRepresentation.create(artifact, metadata)); + }); + return builder.build(); + } + + @Test + public void putAndGet_savesRemoteFileMetadata() { + String key = "key"; + ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + Artifact artifact = ActionsTestUtil.DUMMY_ARTIFACT; + RemoteFileArtifactValue metadata = createRemoteMetadata(artifact, "content"); + entry.addOutputFile(artifact, metadata, /*saveFileMetadata=*/ true); + + cache.put(key, entry); + entry = cache.get(key); + + assertThat(entry.getOutputFile(artifact)).isEqualTo(metadata); + } + + @Test + public void putAndGet_ignoresLocalFileMetadata() throws IOException { + String key = "key"; + ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + Artifact artifact = ActionsTestUtil.DUMMY_ARTIFACT; + FileArtifactValue metadata = createLocalMetadata(artifact, "content"); + entry.addOutputFile(artifact, metadata, /*saveFileMetadata=*/ true); + + cache.put(key, entry); + entry = cache.get(key); + + assertThat(entry.getOutputFile(artifact)).isNull(); + } + + @Test + public void putAndGet_treeMetadata_onlySavesRemoteFileMetadata() throws IOException { + String key = "key"; + ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + SpecialArtifact artifact = + ActionsTestUtil.createTreeArtifactWithGeneratingAction( + artifactRoot, PathFragment.create("bin/dummy")); + TreeArtifactValue metadata = + createTreeMetadata( + artifact, + ImmutableMap.of( + "file1", + createRemoteMetadata( + Artifact.TreeFileArtifact.createTreeOutput( + artifact, PathFragment.create("file1")), + "content1"), + "file2", + createLocalMetadata( + Artifact.TreeFileArtifact.createTreeOutput( + artifact, PathFragment.create("file2")), + "content2")), + Optional.empty()); + entry.addOutputTree(artifact, metadata, /* saveTreeMetadata= */ true); + + cache.put(key, entry); + entry = cache.get(key); + + assertThat(entry.getOutputTree(artifact)) + .isEqualTo( + SerializableTreeArtifactValue.create( + ImmutableMap.of( + "file1", + createRemoteMetadata( + Artifact.TreeFileArtifact.createTreeOutput( + artifact, PathFragment.create("file1")), + "content1")), + Optional.empty())); + } + + @Test + public void putAndGet_treeMetadata_savesRemoteArchivedArtifact() { + String key = "key"; + ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + SpecialArtifact artifact = + ActionsTestUtil.createTreeArtifactWithGeneratingAction( + artifactRoot, PathFragment.create("bin/dummy")); + TreeArtifactValue metadata = + createTreeMetadata( + artifact, ImmutableMap.of(), Optional.of(createRemoteMetadata(artifact, "content"))); + entry.addOutputTree(artifact, metadata, /* saveTreeMetadata= */ true); + + cache.put(key, entry); + entry = cache.get(key); + + assertThat(entry.getOutputTree(artifact)) + .isEqualTo( + SerializableTreeArtifactValue.create( + ImmutableMap.of(), Optional.of(createRemoteMetadata(artifact, "content")))); + } + + @Test + public void putAndGet_treeMetadata_ignoresLocalArchivedArtifact() throws IOException { + String key = "key"; + ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + SpecialArtifact artifact = + ActionsTestUtil.createTreeArtifactWithGeneratingAction( + artifactRoot, PathFragment.create("bin/dummy")); + TreeArtifactValue metadata = + createTreeMetadata( + artifact, + ImmutableMap.of(), + Optional.of( + createLocalMetadata( + ActionsTestUtil.createArtifact(artifactRoot, "bin/archive"), "content"))); + entry.addOutputTree(artifact, metadata, /* saveTreeMetadata= */ true); + + cache.put(key, entry); + entry = cache.get(key); + + assertThat(entry.getOutputTree(artifact)).isNull(); + } + private static void assertKeyEquals(ActionCache cache1, ActionCache cache2, String key) { Object entry = cache1.get(key); assertThat(entry).isNotNull(); diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java index 002464bca5dcbb..88a4b57a5b43c3 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java @@ -982,7 +982,7 @@ public ImmutableSet getTreeArtifactChildren(SpecialArtifact tr } @Override - public TreeArtifactValue getTreeArtifactValue(SpecialArtifact treeArtifact) { + public TreeArtifactValue getTreeArtifactValue(SpecialArtifact treeArtifact) throws IOException { throw new UnsupportedOperationException(); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java index f6477c670c15d3..8c7dad2f8331c0 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java @@ -734,7 +734,8 @@ public Path getPathFromSourceExecPath(Path execRoot, PathFragment execPath) { }, new ActionKeyContext(), Predicates.alwaysTrue(), - null); + null, + PathFragment.create("bazel-out")); private static final ProgressSupplier EMPTY_PROGRESS_SUPPLIER = new ProgressSupplier() { @Override diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java index 3ead4d201d3f89..931822b0756a8d 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java @@ -378,7 +378,12 @@ public void buildArtifacts( executor, options, new ActionCacheChecker( - actionCache, null, actionKeyContext, ALWAYS_EXECUTE_FILTER, null), + actionCache, + null, + actionKeyContext, + ALWAYS_EXECUTE_FILTER, + null, + PathFragment.create("bazel-out")), topDownActionCache, /*outputService=*/ null, /*incrementalAnalysis=*/ true); diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 95fd661bb13703..565aca2a0a85ee 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -1157,6 +1157,52 @@ EOF || fail "Expected bazel-bin/a/template.txt to be downloaded" } +function test_downloads_minimal_hit_action_cache() { + # Test that remote metadata is saved and action cache is hit across server restarts when using + # --remote_download_minimal + mkdir -p a + cat > a/BUILD <<'EOF' +genrule( + name = "foo", + srcs = [], + outs = ["foo.txt"], + cmd = "echo \"foo\" > \"$@\"", +) + +genrule( + name = "foobar", + srcs = [":foo"], + outs = ["foobar.txt"], + cmd = "cat $(location :foo) > \"$@\" && echo \"bar\" >> \"$@\"", +) +EOF + + bazel build \ + --experimental_ui_debug_all_events \ + --experimental_action_cache_store_output_metadata \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + //a:foobar >& $TEST_log || fail "Failed to build //a:foobar" + + expect_log "START.*: \[.*\] Executing genrule //a:foobar" + + (! [[ -e bazel-bin/a/foo.txt ]] && ! [[ -e bazel-bin/a/foobar.txt ]]) \ + || fail "Expected no files to have been downloaded" + + assert_equals "" "$(ls bazel-bin/a)" + + bazel shutdown + + bazel build \ + --experimental_ui_debug_all_events \ + --experimental_action_cache_store_output_metadata \ + --remote_executor=grpc://localhost:${worker_port} \ + --remote_download_minimal \ + //a:foobar >& $TEST_log || fail "Failed to build //a:foobar" + + expect_not_log "START.*: \[.*\] Executing genrule //a:foobar" +} + function test_downloads_toplevel() { # Test that when using --remote_download_outputs=toplevel only the output of the # toplevel target is being downloaded.