ActionMetadataHandler: proper metadata even for the volatile workspace status
This is part of cleaning up the ActionInputFileCache / MetadataHandler split.
Both classes generally store and return identical information, except the
MetadataHandler lies about constant metadata artifacts. Instead of lying here,
update the ActionCacheChecker, which is the only place where we actually want
constant metadata.
Additionally, remove getMetadataMaybe; again, the only caller that needs
special casing is the ActionCacheChecker, so we move the special casing there
instead of having it in the MetadataHandler.
PiperOrigin-RevId: 159665345
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 a7f0b82..57abd35 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
@@ -131,14 +131,15 @@
*
* @return true if at least one artifact has changed, false - otherwise.
*/
- private boolean validateArtifacts(Entry entry, Action action,
- Iterable<Artifact> actionInputs, MetadataHandler metadataHandler, boolean checkOutput) {
+ private boolean validateArtifacts(
+ Entry entry, Action action, Iterable<Artifact> actionInputs, MetadataHandler metadataHandler,
+ boolean checkOutput) {
Iterable<Artifact> artifacts = checkOutput
? Iterables.concat(action.getOutputs(), actionInputs)
: actionInputs;
Map<String, Metadata> mdMap = new HashMap<>();
for (Artifact artifact : artifacts) {
- mdMap.put(artifact.getExecPathString(), metadataHandler.getMetadataMaybe(artifact));
+ mdMap.put(artifact.getExecPathString(), getMetadataMaybe(metadataHandler, artifact));
}
return !DigestUtils.fromMetadata(mdMap).equals(entry.getFileDigest());
}
@@ -290,6 +291,27 @@
return false; // cache hit
}
+ private static Metadata getMetadataOrConstant(MetadataHandler metadataHandler, Artifact artifact)
+ throws IOException {
+ if (artifact.isConstantMetadata()) {
+ return Metadata.CONSTANT_METADATA;
+ } else {
+ return metadataHandler.getMetadata(artifact);
+ }
+ }
+
+ // TODO(ulfjack): It's unclear to me why we're ignoring all IOExceptions. In some cases, we want
+ // to trigger a re-execution, so we should catch the IOException explicitly there. In others, we
+ // should propagate the exception, because it is unexpected (e.g., bad file system state).
+ @Nullable
+ private static Metadata getMetadataMaybe(MetadataHandler metadataHandler, Artifact artifact) {
+ try {
+ return getMetadataOrConstant(metadataHandler, artifact);
+ } catch (IOException e) {
+ return null;
+ }
+ }
+
public void afterExecution(
Action action, Token token, MetadataHandler metadataHandler, Map<String, String> clientEnv)
throws IOException {
@@ -313,14 +335,17 @@
actionCache.remove(execPath);
}
if (!metadataHandler.artifactOmitted(output)) {
- // Output files *must* exist and be accessible after successful action execution.
- Metadata metadata = metadataHandler.getMetadata(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.
+ Metadata metadata = getMetadataOrConstant(metadataHandler, output);
Preconditions.checkState(metadata != null);
entry.addFile(output.getExecPath(), metadata);
}
}
for (Artifact input : action.getInputs()) {
- entry.addFile(input.getExecPath(), metadataHandler.getMetadataMaybe(input));
+ entry.addFile(input.getExecPath(), getMetadataMaybe(metadataHandler, input));
}
entry.getFileDigest();
actionCache.put(key, entry);
@@ -392,18 +417,16 @@
}
/**
- * Special handling for the MiddlemanAction. Since MiddlemanAction output
- * artifacts are purely fictional and used only to stay within dependency
- * graph model limitations (action has to depend on artifacts, not on other
- * actions), we do not need to validate metadata for the outputs - only for
- * inputs. We also do not need to validate MiddlemanAction key, since action
- * cache entry key already incorporates that information for the middlemen
- * and we will experience a cache miss when it is different. Whenever it
- * encounters middleman artifacts as input artifacts for other actions, it
- * consults with the aggregated middleman digest computed here.
+ * Special handling for the MiddlemanAction. Since MiddlemanAction output artifacts are purely
+ * fictional and used only to stay within dependency graph model limitations (action has to depend
+ * on artifacts, not on other actions), we do not need to validate metadata for the outputs - only
+ * for inputs. We also do not need to validate MiddlemanAction key, since action cache entry key
+ * already incorporates that information for the middlemen and we will experience a cache miss
+ * when it is different. Whenever it encounters middleman artifacts as input artifacts for other
+ * actions, it consults with the aggregated middleman digest computed here.
*/
- protected void checkMiddlemanAction(Action action, EventHandler handler,
- MetadataHandler metadataHandler) {
+ protected void checkMiddlemanAction(
+ Action action, EventHandler handler, MetadataHandler metadataHandler) {
if (!cacheConfig.enabled()) {
// Action cache is disabled, don't generate digests.
return;
@@ -430,7 +453,7 @@
// it in the cache entry and just use empty string instead.
entry = new ActionCache.Entry("", ImmutableMap.<String, String>of(), false);
for (Artifact input : action.getInputs()) {
- entry.addFile(input.getExecPath(), metadataHandler.getMetadataMaybe(input));
+ entry.addFile(input.getExecPath(), getMetadataMaybe(metadataHandler, input));
}
}
