Refactor FileArtifactValue and ArtifactValue now that presence of mtime and digest are mutually exclusive.
--
MOS_MIGRATED_REVID=128843642
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
index b71321c..b68665b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
@@ -20,7 +20,7 @@
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.pkgcache.PackageProvider;
-import com.google.devtools.build.lib.skyframe.ArtifactValue.OwnedArtifact;
+import com.google.devtools.build.lib.skyframe.ArtifactSkyKey.OwnedArtifact;
import com.google.devtools.build.lib.skyframe.TargetCompletionValue.TargetCompletionKey;
import com.google.devtools.build.skyframe.CycleInfo;
import com.google.devtools.build.skyframe.SkyFunctionName;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index fdbd5be..8ec6f0e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -411,8 +411,7 @@
inputArtifactData.putAll(state.inputArtifactData);
for (Map.Entry<SkyKey, SkyValue> entry : metadataFoundDuringActionExecution.entrySet()) {
inputArtifactData.put(
- ArtifactValue.artifact(entry.getKey()),
- (FileArtifactValue) entry.getValue());
+ ArtifactSkyKey.artifact(entry.getKey()), (FileArtifactValue) entry.getValue());
}
state.inputArtifactData = inputArtifactData;
metadataHandler =
@@ -461,7 +460,7 @@
for (Artifact artifact : discoveredInputs) {
if (!inputData.containsKey(artifact)) {
// Note that if the artifact is derived, the mandatory flag is ignored.
- keys.add(ArtifactValue.key(artifact, /*mandatory=*/false));
+ keys.add(ArtifactSkyKey.key(artifact, /*mandatory=*/ false));
}
}
// We do not do a getValuesOrThrow() call for the following reasons:
@@ -474,7 +473,8 @@
Map<SkyKey, SkyValue> data = env.getValues(keys);
if (!env.valuesMissing()) {
for (Entry<SkyKey, SkyValue> entry : data.entrySet()) {
- inputData.put(ArtifactValue.artifact(entry.getKey()), (FileArtifactValue) entry.getValue());
+ inputData.put(
+ ArtifactSkyKey.artifact(entry.getKey()), (FileArtifactValue) entry.getValue());
}
}
}
@@ -508,17 +508,19 @@
if (mandatoryInputs == null) {
// This is a non inputs-discovering action, so no need to distinguish mandatory from regular
// inputs.
- return Iterables.transform(inputs, new Function<Artifact, SkyKey>() {
- @Override
- public SkyKey apply(Artifact artifact) {
- return ArtifactValue.key(artifact, true);
- }
- });
+ return Iterables.transform(
+ inputs,
+ new Function<Artifact, SkyKey>() {
+ @Override
+ public SkyKey apply(Artifact artifact) {
+ return ArtifactSkyKey.key(artifact, true);
+ }
+ });
} else {
Collection<SkyKey> discoveredArtifacts = new HashSet<>();
Set<Artifact> mandatory = Sets.newHashSet(mandatoryInputs);
for (Artifact artifact : inputs) {
- discoveredArtifacts.add(ArtifactValue.key(artifact, mandatory.contains(artifact)));
+ discoveredArtifacts.add(ArtifactSkyKey.key(artifact, mandatory.contains(artifact)));
}
return discoveredArtifacts;
}
@@ -549,9 +551,9 @@
ActionExecutionException firstActionExecutionException = null;
for (Map.Entry<SkyKey, ValueOrException2<MissingInputFileException,
ActionExecutionException>> depsEntry : inputDeps.entrySet()) {
- Artifact input = ArtifactValue.artifact(depsEntry.getKey());
+ Artifact input = ArtifactSkyKey.artifact(depsEntry.getKey());
try {
- ArtifactValue value = (ArtifactValue) depsEntry.getValue().get();
+ SkyValue value = depsEntry.getValue().get();
if (populateInputData) {
if (value instanceof AggregatingArtifactValue) {
AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value;
@@ -574,8 +576,8 @@
expandedArtifacts.put(input, expandedTreeArtifacts);
// Again, we cache the "digest" of the value for cache checking.
inputArtifactData.put(input, setValue.getSelfData());
- } else if (value instanceof FileArtifactValue) {
- // TODO(bazel-team): Make sure middleman "virtual" artifact data is properly processed.
+ } else {
+ Preconditions.checkState(value instanceof FileArtifactValue, depsEntry);
inputArtifactData.put(input, (FileArtifactValue) value);
}
}
@@ -760,7 +762,7 @@
new Function<SkyKey, Artifact>() {
@Override
public Artifact apply(SkyKey key) {
- return ArtifactValue.artifact(key);
+ return ArtifactSkyKey.artifact(key);
}
});
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 681ed81..dd93e04 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
@@ -57,14 +57,13 @@
* <p>As well, this cache collects data about the action's output files, which is used in three
* ways. First, it is served as requested during action execution, primarily by the {@code
* ActionCacheChecker} when determining if the action must be rerun, and then after the action is
- * run, to gather information about the outputs. Second, it is accessed by {@link
- * ArtifactFunction}s in order to construct {@link ArtifactValue}, and by this class itself to
- * generate {@link TreeArtifactValue}s. Third, the {@link
- * FilesystemValueChecker} uses it to determine the set of output files to check for inter-build
- * modifications. Because all these use cases are slightly different, we must occasionally store two
- * versions of the data for a value. See {@link #getAdditionalOutputData} for elaboration on
- * the difference between these cases, and see the javadoc for the various internal maps to see
- * what is stored where.
+ * run, to gather information about the outputs. Second, it is accessed by {@link ArtifactFunction}s
+ * in order to construct {@link FileArtifactValue}s, and by this class itself to generate {@link
+ * TreeArtifactValue}s. Third, the {@link FilesystemValueChecker} uses it to determine the set of
+ * output files to check for inter-build modifications. Because all these use cases are slightly
+ * different, we must occasionally store two versions of the data for a value. See {@link
+ * #getAdditionalOutputData} for elaboration on the difference between these cases, and see the
+ * javadoc for the various internal maps to see what is stored where.
*/
@VisibleForTesting
public class ActionMetadataHandler implements MetadataHandler {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java
index 26d254b..59572f1 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java
@@ -23,10 +23,8 @@
import com.google.devtools.build.lib.skyframe.ActionTemplateExpansionValue.ActionTemplateExpansionKey;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
-import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-
import javax.annotation.Nullable;
/**
@@ -45,7 +43,7 @@
SpawnActionTemplate actionTemplate = key.getActionTemplate();
// Requests the TreeArtifactValue object for the input TreeArtifact.
- SkyKey artifactValueKey = ArtifactValue.key(actionTemplate.getInputTreeArtifact(), true);
+ SkyKey artifactValueKey = ArtifactSkyKey.key(actionTemplate.getInputTreeArtifact(), true);
TreeArtifactValue treeArtifactValue = (TreeArtifactValue) env.getValue(artifactValueKey);
// Input TreeArtifact is not ready yet.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AggregatingArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AggregatingArtifactValue.java
index 030bf13..2fb2199 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AggregatingArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AggregatingArtifactValue.java
@@ -16,11 +16,11 @@
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.util.Pair;
-
+import com.google.devtools.build.skyframe.SkyValue;
import java.util.Collection;
/** Value for aggregating artifacts, which must be expanded to a set of other artifacts. */
-class AggregatingArtifactValue extends ArtifactValue {
+class AggregatingArtifactValue implements SkyValue {
private final FileArtifactValue selfData;
private final ImmutableList<Pair<Artifact, FileArtifactValue>> inputs;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
index 5393d19..f7007b7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -30,7 +30,7 @@
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.skyframe.ActionLookupValue.ActionLookupKey;
-import com.google.devtools.build.lib.skyframe.ArtifactValue.OwnedArtifact;
+import com.google.devtools.build.lib.skyframe.ArtifactSkyKey.OwnedArtifact;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -41,13 +41,10 @@
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-
import java.io.IOException;
import java.util.Map;
-/**
- * A builder for {@link ArtifactValue}s.
- */
+/** A builder of values for {@link ArtifactSkyKey} keys. */
class ArtifactFunction implements SkyFunction {
private final Predicate<PathFragment> allowedMissingInputs;
@@ -169,7 +166,7 @@
return TreeArtifactValue.create(map.build());
}
- private ArtifactValue createSourceValue(Artifact artifact, boolean mandatory, Environment env)
+ private FileArtifactValue createSourceValue(Artifact artifact, boolean mandatory, Environment env)
throws MissingInputFileException {
SkyKey fileSkyKey = FileValue.key(RootedPath.toRootedPath(artifact.getRoot().getPath(),
artifact.getPath()));
@@ -204,8 +201,9 @@
return allowedMissingInputs.apply(((RootedPath) fileSkyKey.argument()).getRelativePath());
}
- private static ArtifactValue missingInputFile(Artifact artifact, boolean mandatory,
- Exception failure, EventHandler reporter) throws MissingInputFileException {
+ private static FileArtifactValue missingInputFile(
+ Artifact artifact, boolean mandatory, Exception failure, EventHandler reporter)
+ throws MissingInputFileException {
if (!mandatory) {
return FileArtifactValue.MISSING_FILE_MARKER;
}
@@ -251,14 +249,17 @@
}
}
- private AggregatingArtifactValue createAggregatingValue(Artifact artifact,
- ActionAnalysisMetadata action, FileArtifactValue value, SkyFunction.Environment env) {
+ private static AggregatingArtifactValue createAggregatingValue(
+ Artifact artifact,
+ ActionAnalysisMetadata action,
+ FileArtifactValue value,
+ SkyFunction.Environment env) {
// This artifact aggregates other artifacts. Keep track of them so callers can find them.
ImmutableList.Builder<Pair<Artifact, FileArtifactValue>> inputs = ImmutableList.builder();
for (Map.Entry<SkyKey, SkyValue> entry :
- env.getValues(ArtifactValue.mandatoryKeys(action.getInputs())).entrySet()) {
- Artifact input = ArtifactValue.artifact(entry.getKey());
- ArtifactValue inputValue = (ArtifactValue) entry.getValue();
+ env.getValues(ArtifactSkyKey.mandatoryKeys(action.getInputs())).entrySet()) {
+ Artifact input = ArtifactSkyKey.artifact(entry.getKey());
+ SkyValue inputValue = entry.getValue();
Preconditions.checkNotNull(inputValue, "%s has null dep %s", artifact, input);
if (!(inputValue instanceof FileArtifactValue)) {
// We do not recurse in aggregating middleman artifacts.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java
similarity index 81%
rename from src/main/java/com/google/devtools/build/lib/skyframe/ArtifactValue.java
rename to src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java
index d9a5150..4fa5d95 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactSkyKey.java
@@ -21,23 +21,22 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.skyframe.SkyKey;
-import com.google.devtools.build.skyframe.SkyValue;
-
import java.util.Collection;
/**
- * A value representing an artifact. Source artifacts are checked for existence, while output
- * artifacts imply creation of the output file.
+ * A utility class for {@link SkyKey}s coming from {@link Artifact}s. Source artifacts are checked
+ * for existence, while output artifacts imply creation of the output file.
*
- * <p>There are effectively three kinds of output artifact values. The first corresponds to an
- * ordinary artifact {@link FileArtifactValue}. It stores the relevant data for the artifact --
- * digest/mtime and size. The second corresponds to either an "aggregating" artifact -- the output
- * of an aggregating middleman action -- or a TreeArtifact. It stores the relevant data of all its
- * inputs, as well as a combined digest for itself.
+ * <p>There are effectively three kinds of output artifact values corresponding to these keys. The
+ * first corresponds to an ordinary artifact {@link FileArtifactValue}. It stores the relevant data
+ * for the artifact -- digest/mtime and size. The second corresponds to either an "aggregating"
+ * artifact -- the output of an aggregating middleman action -- or a TreeArtifact. It stores the
+ * relevant data of all its inputs, as well as a combined digest for itself.
*/
@Immutable
@ThreadSafe
-public abstract class ArtifactValue implements SkyValue {
+public final class ArtifactSkyKey {
+ private ArtifactSkyKey() {}
@ThreadSafe
public static SkyKey key(Artifact artifact, boolean isMandatory) {
@@ -63,11 +62,11 @@
private static final Function<OwnedArtifact, Artifact> TO_ARTIFACT =
new Function<OwnedArtifact, Artifact>() {
- @Override
- public Artifact apply(OwnedArtifact key) {
- return key.getArtifact();
- }
- };
+ @Override
+ public Artifact apply(OwnedArtifact key) {
+ return key.getArtifact();
+ }
+ };
public static Collection<Artifact> artifacts(Collection<? extends OwnedArtifact> keys) {
return Collections2.transform(keys, TO_ARTIFACT);
@@ -78,8 +77,7 @@
}
public static boolean equalWithOwner(Artifact first, Artifact second) {
- return first.equals(second)
- && first.getArtifactOwner().equals(second.getArtifactOwner());
+ return first.equals(second) && first.getArtifactOwner().equals(second.getArtifactOwner());
}
/**
@@ -113,11 +111,11 @@
}
/**
- * Constructs an OwnedArtifact wrapper for a derived artifact. The mandatory attribute is
- * not needed because a derived artifact must be a mandatory input for some action in order to
- * ensure that it is built in the first place. If it fails to build, then that fact is cached
- * in the node, so any action that has it as a non-mandatory input can retrieve that
- * information from the node.
+ * Constructs an OwnedArtifact wrapper for a derived artifact. The mandatory attribute is not
+ * needed because a derived artifact must be a mandatory input for some action in order to
+ * ensure that it is built in the first place. If it fails to build, then that fact is cached in
+ * the node, so any action that has it as a non-mandatory input can retrieve that information
+ * from the node.
*/
private OwnedArtifact(Artifact derivedArtifact) {
this.artifact = Preconditions.checkNotNull(derivedArtifact);
@@ -127,7 +125,7 @@
@Override
public int hashCode() {
- int initialHash = artifact.hashCode() + artifact.getArtifactOwner().hashCode();
+ int initialHash = artifact.hashCode() + artifact.getArtifactOwner().hashCode();
return isMandatory ? initialHash : 47 * initialHash + 1;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
index 1f48c84..db9ab97 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
@@ -36,10 +36,8 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.ValueOrException2;
-
import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;
-
import javax.annotation.Nullable;
/**
@@ -248,7 +246,7 @@
Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps =
env.getValuesOrThrow(
- ArtifactValue.mandatoryKeys(
+ ArtifactSkyKey.mandatoryKeys(
completor.getAllArtifactsToBuild(value, topLevelContext).getAllArtifacts()),
MissingInputFileException.class,
ActionExecutionException.class);
@@ -259,7 +257,7 @@
NestedSetBuilder<Label> rootCausesBuilder = NestedSetBuilder.stableOrder();
for (Map.Entry<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>>
depsEntry : inputDeps.entrySet()) {
- Artifact input = ArtifactValue.artifact(depsEntry.getKey());
+ Artifact input = ArtifactSkyKey.artifact(depsEntry.getKey());
try {
depsEntry.getValue().get();
} catch (MissingInputFileException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java
index 1e7a4c7..dcbe1bc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java
@@ -20,68 +20,194 @@
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
import java.util.Arrays;
import javax.annotation.Nullable;
/**
* Stores the actual metadata data of a file. We have the following cases:
- * <ul><li>
- * an ordinary file, in which case we would expect to see a digest and size;
- * </li><li>
- * a directory, in which case we would expect to see an mtime;
- * </li><li>
- * an intentionally omitted file which the build system is aware of but doesn't actually exist,
- * where all access methods are unsupported;
- * </li><li>
- * The "self data" of a middleman artifact or TreeArtifact, where we would expect to see a digest
- * representing the artifact's contents, and a size of 1.
- * </li></ul>
+ *
+ * <ul>
+ * <li> an ordinary file, in which case we would expect to see a digest and size;
+ * <li> a directory, in which case we would expect to see an mtime;
+ * <li> an intentionally omitted file which the build system is aware of but doesn't actually exist,
+ * where all access methods are unsupported;
+ * <li> a "middleman marker" object, which has a null digest, 0 size, and mtime of 0.
+ * <li> The "self data" of a TreeArtifact, where we would expect to see a digest representing the
+ * artifact's contents, and a size of 0.
+ * </ul>
*/
-public class FileArtifactValue extends ArtifactValue {
- /** Data for Middleman artifacts that did not have data specified. */
- static final FileArtifactValue DEFAULT_MIDDLEMAN = new FileArtifactValue(null, 0, 0);
- /** Data that marks that a file is not present on the filesystem. */
- @VisibleForTesting
- public static final FileArtifactValue MISSING_FILE_MARKER = new FileArtifactValue(null, 1, 0) {
+// TODO(janakr): make this an interface once JDK8 allows us to have static methods on interfaces.
+public abstract class FileArtifactValue implements SkyValue {
+ private static final class SingletonMarkerValue extends FileArtifactValue {
+ @Nullable
@Override
- public boolean exists() {
+ public byte[] getDigest() {
+ return null;
+ }
+
+ @Override
+ boolean isFile() {
return false;
}
- };
- /**
- * Represents an omitted file -- we are aware of it but it doesn't exist. All access methods
- * are unsupported.
- */
- static final FileArtifactValue OMITTED_FILE_MARKER = new FileArtifactValue(null, 2, 0) {
- @Override public byte[] getDigest() { throw new UnsupportedOperationException(); }
- @Override public boolean isFile() { throw new UnsupportedOperationException(); }
- @Override public long getSize() { throw new UnsupportedOperationException(); }
- @Override public long getModifiedTime() { throw new UnsupportedOperationException(); }
- @Override public boolean equals(Object o) { return this == o; }
- @Override public int hashCode() { return System.identityHashCode(this); }
- @Override public String toString() { return "OMITTED_FILE_MARKER"; }
- };
+ @Override
+ public long getSize() {
+ return 0;
+ }
- @Nullable private final byte[] digest;
- private final long mtime;
- private final long size;
+ @Override
+ public long getModifiedTime() {
+ return 0;
+ }
- private FileArtifactValue(byte[] digest, long size) {
- this.digest = Preconditions.checkNotNull(digest, size);
- this.size = size;
- this.mtime = -1;
+ @Override
+ public String toString() {
+ return "singleton marker artifact value (" + hashCode() + ")";
+ }
}
- // Only used by directories (null digest).
- private FileArtifactValue(byte[] digest, long mtime, long size) {
- Preconditions.checkState(mtime >= 0, "mtime must be non-negative: %s %s", mtime, size);
- Preconditions.checkState(size == 0, "size must be zero: %s %s", mtime, size);
- Preconditions.checkState(digest == null, "digest must be null:");
- this.digest = digest;
- this.size = size;
- this.mtime = mtime;
+ static final FileArtifactValue DEFAULT_MIDDLEMAN = new SingletonMarkerValue();
+ /** Data that marks that a file is not present on the filesystem. */
+ @VisibleForTesting
+ public static final FileArtifactValue MISSING_FILE_MARKER = new SingletonMarkerValue();
+
+ /**
+ * Represents an omitted file -- we are aware of it but it doesn't exist. All access methods are
+ * unsupported.
+ */
+ static final FileArtifactValue OMITTED_FILE_MARKER =
+ new FileArtifactValue() {
+ @Override
+ public byte[] getDigest() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ boolean isFile() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public long getSize() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public long getModifiedTime() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public String toString() {
+ return "OMITTED_FILE_MARKER";
+ }
+ };
+
+ private static final class DirectoryArtifactValue extends FileArtifactValue {
+ private final long mtime;
+
+ private DirectoryArtifactValue(long mtime) {
+ this.mtime = mtime;
+ }
+
+ @Nullable
+ @Override
+ public byte[] getDigest() {
+ return null;
+ }
+
+ @Override
+ public long getModifiedTime() {
+ return mtime;
+ }
+
+ @Override
+ public long getSize() {
+ return 0;
+ }
+
+ @Override
+ public boolean isFile() {
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return (int) mtime;
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ return (this == other)
+ || ((other instanceof DirectoryArtifactValue)
+ && this.mtime == ((DirectoryArtifactValue) other).mtime);
+ }
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this).add("mtime", mtime).toString();
+ }
+ }
+
+ private static final class RegularFileArtifactValue extends FileArtifactValue {
+ private final byte[] digest;
+ private final long size;
+
+ private RegularFileArtifactValue(byte[] digest, long size) {
+ this.digest = Preconditions.checkNotNull(digest);
+ this.size = size;
+ }
+
+ @Override
+ public byte[] getDigest() {
+ return digest;
+ }
+
+ @Override
+ boolean isFile() {
+ return true;
+ }
+
+ @Override
+ public long getSize() {
+ return size;
+ }
+
+ @Override
+ public long getModifiedTime() {
+ throw new UnsupportedOperationException(
+ "regular file's mtime should never be called. (" + this + ")");
+ }
+
+ @Override
+ public int hashCode() {
+ // Hash digest by content, not reference.
+ return 37 * (int) size + Arrays.hashCode(digest);
+ }
+
+ /**
+ * Two RegularFileArtifactValues will only compare equal if they have the same content. This
+ * differs from the {@code Metadata#equivalence} method, which allows for comparison using mtime
+ * if one object does not have a digest available.
+ */
+ @Override
+ public boolean equals(Object other) {
+ if (this == other) {
+ return true;
+ }
+ if (!(other instanceof RegularFileArtifactValue)) {
+ return false;
+ }
+ RegularFileArtifactValue that = (RegularFileArtifactValue) other;
+ return this.size == that.size && Arrays.equals(this.digest, that.digest);
+ }
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this).add("digest", digest).add("size", size).toString();
+ }
}
@VisibleForTesting
@@ -107,10 +233,10 @@
// In this case, we need to store the mtime because the action cache uses mtime for
// directories to determine if this artifact has changed. We want this code path to go away
// somehow (maybe by implementing FileSet in Skyframe).
- return new FileArtifactValue(digest, artifact.getPath().getLastModifiedTime(), size);
+ return new DirectoryArtifactValue(artifact.getPath().getLastModifiedTime());
}
Preconditions.checkState(digest != null, artifact);
- return new FileArtifactValue(digest, size);
+ return new RegularFileArtifactValue(digest, size);
}
/**
@@ -119,65 +245,19 @@
*/
static FileArtifactValue createProxy(byte[] digest) {
Preconditions.checkNotNull(digest);
- // The Middleman artifact values have size 1 because we want their digests to be used. This hack
- // can be removed once empty files are digested.
- return new FileArtifactValue(digest, /*size=*/1);
+ return new RegularFileArtifactValue(digest, /*size=*/ 0);
}
+ /** Returns the digest of this value. Null for non-files, non-null for files. */
@Nullable
- public byte[] getDigest() {
- return digest;
- }
+ public abstract byte[] getDigest();
/** @return true if this is a file or a symlink to an existing file */
- boolean isFile() {
- return digest != null;
- }
+ abstract boolean isFile();
- /** Gets the size of the file. Directories have size 0. */
- public long getSize() {
- return size;
- }
+ /** Gets the size of the file. Non-files (including directories) have size 0. */
+ public abstract long getSize();
- /** Gets last modified time of file. Should only be called if this is a directory. */
- long getModifiedTime() {
- Preconditions.checkState(size == 0 && digest == null, "%s %s %s", digest, mtime, size);
- return mtime;
- }
-
- public boolean exists() {
- return true;
- }
-
- @Override
- public int hashCode() {
- // Hash digest by content, not reference. Note that digest is the only array in this array.
- return Arrays.deepHashCode(new Object[] {size, mtime, digest});
- }
-
- /**
- * Two FileArtifactValues will only compare equal if they have the same content. This differs
- * from the {@code Metadata#equivalence} method, which allows for comparison using mtime if
- * one object does not have a digest available.
- */
- @Override
- public boolean equals(Object other) {
- if (this == other) {
- return true;
- }
- if (!(other instanceof FileArtifactValue)) {
- return false;
- }
- FileArtifactValue that = (FileArtifactValue) other;
- return this.mtime == that.mtime && this.size == that.size
- && Arrays.equals(this.digest, that.digest);
- }
-
- @Override
- public String toString() {
- return MoreObjects.toStringHelper(FileArtifactValue.class)
- .add("digest", digest)
- .add("mtime", mtime)
- .add("size", size).toString();
- }
+ /** Gets last modified time of file. Should only be called if this is not a file. */
+ abstract long getModifiedTime();
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileValue.java
index d4ce6ca..6b2bfa7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileValue.java
@@ -21,30 +21,26 @@
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-
import java.util.Objects;
-
import javax.annotation.Nullable;
/**
* A value that corresponds to a file (or directory or symlink or non-existent file), fully
* accounting for symlinks (e.g. proper dependencies on ancestor symlinks so as to be incrementally
* correct). Anything in Skyframe that cares about the fully resolved path of a file (e.g. anything
- * that cares about the contents of a file) should have a dependency on the corresponding
- * {@link FileValue}.
+ * that cares about the contents of a file) should have a dependency on the corresponding {@link
+ * FileValue}.
*
- * <p>
- * Note that the existence of a file value does not imply that the file exists on the filesystem.
+ * <p>Note that the existence of a file value does not imply that the file exists on the filesystem.
* File values for missing files will be created on purpose in order to facilitate incremental
* builds in the case those files have reappeared.
*
- * <p>
- * This class contains the relevant metadata for a file, although not the contents. Note that
+ * <p>This class contains the relevant metadata for a file, although not the contents. Note that
* since a FileValue doesn't store its corresponding SkyKey, it's possible for the FileValues for
* two different paths to be the same.
*
- * <p>
- * This should not be used for build outputs; use {@link ArtifactValue} for those.
+ * <p>This should not be used for build outputs; use {@link ArtifactSkyKey} to create keys for
+ * those.
*/
@Immutable
@ThreadSafe
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index f9426d1..a735261 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -137,7 +137,6 @@
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.WalkableGraph.WalkableGraphFactory;
import com.google.devtools.common.options.OptionsClassProvider;
-
import java.io.IOException;
import java.io.PrintStream;
import java.util.ArrayList;
@@ -155,7 +154,6 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;
import java.util.logging.Logger;
-
import javax.annotation.Nullable;
/**
@@ -1089,7 +1087,7 @@
resourceManager.resetResourceUsage();
try {
progressReceiver.executionProgressReceiver = executionProgressReceiver;
- Iterable<SkyKey> artifactKeys = ArtifactValue.mandatoryKeys(artifactsToBuild);
+ Iterable<SkyKey> artifactKeys = ArtifactSkyKey.mandatoryKeys(artifactsToBuild);
Iterable<SkyKey> targetKeys =
TargetCompletionValue.keys(targetsToBuild, topLevelArtifactContext);
Iterable<SkyKey> aspectKeys = AspectCompletionValue.keys(aspects, topLevelArtifactContext);
@@ -1743,7 +1741,7 @@
+ "Please remove '" + directories.getInstallBase() + "' and try again.";
throw new AbruptExitException(message, ExitCode.LOCAL_ENVIRONMENTAL_ERROR, e);
}
- values.put(ArtifactValue.key(artifact, /*isMandatory=*/true), fileArtifactValue);
+ values.put(ArtifactSkyKey.key(artifact, /*isMandatory=*/ true), fileArtifactValue);
}
injectable().inject(values);
needToInjectEmbeddedArtifacts = false;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
index cdb22dd..d277b19 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
@@ -53,12 +53,12 @@
if (key.exclusiveTesting()) {
// Request test artifacts iteratively if testing exclusively.
for (Artifact testArtifact : TestProvider.getTestStatusArtifacts(ct)) {
- if (env.getValue(ArtifactValue.key(testArtifact, /*isMandatory=*/true)) == null) {
+ if (env.getValue(ArtifactSkyKey.key(testArtifact, /*isMandatory=*/ true)) == null) {
return null;
}
}
} else {
- env.getValues(ArtifactValue.mandatoryKeys(TestProvider.getTestStatusArtifacts(ct)));
+ env.getValues(ArtifactSkyKey.mandatoryKeys(TestProvider.getTestStatusArtifacts(ct)));
if (env.valuesMissing()) {
return null;
}
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 a0f2241..bddd592 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
@@ -26,20 +26,18 @@
import com.google.devtools.build.lib.actions.cache.Metadata;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
-
+import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
-
import java.util.Arrays;
import java.util.Map;
import java.util.Set;
-
import javax.annotation.Nullable;
/**
- * Value for TreeArtifacts, which contains a digest and the {@link FileArtifactValue}s
- * of its child {@link TreeFileArtifact}s.
+ * Value for TreeArtifacts, which contains a digest and the {@link FileArtifactValue}s of its child
+ * {@link TreeFileArtifact}s.
*/
-public class TreeArtifactValue extends ArtifactValue {
+public class TreeArtifactValue implements SkyValue {
private static final Function<Artifact, PathFragment> PARENT_RELATIVE_PATHS =
new Function<Artifact, PathFragment>() {
@Override
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
index 790429a..1b725b3 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
@@ -35,7 +35,7 @@
import com.google.devtools.build.lib.analysis.actions.SpawnActionTemplate.OutputPathMapper;
import com.google.devtools.build.lib.events.NullEventHandler;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
-import com.google.devtools.build.lib.skyframe.ArtifactValue.OwnedArtifact;
+import com.google.devtools.build.lib.skyframe.ArtifactSkyKey.OwnedArtifact;
import com.google.devtools.build.lib.testutil.FoundationTestCase;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.Path;
@@ -49,22 +49,20 @@
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.concurrent.atomic.AtomicReference;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
/** Tests for {@link ActionTemplateExpansionFunction}. */
@RunWith(JUnit4.class)
public final class ActionTemplateExpansionFunctionTest extends FoundationTestCase {
- private Map<Artifact, ArtifactValue> artifactValueMap;
+ private Map<Artifact, TreeArtifactValue> artifactValueMap;
private SequentialBuildDriver driver;
@Before
@@ -225,9 +223,9 @@
/** Dummy ArtifactFunction that just returns injected values */
private static class DummyArtifactFunction implements SkyFunction {
- private final Map<Artifact, ArtifactValue> artifactValueMap;
+ private final Map<Artifact, TreeArtifactValue> artifactValueMap;
- DummyArtifactFunction(Map<Artifact, ArtifactValue> artifactValueMap) {
+ DummyArtifactFunction(Map<Artifact, TreeArtifactValue> artifactValueMap) {
this.artifactValueMap = artifactValueMap;
}
@Override
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 1ba51c8..dcd7a33 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -49,12 +49,6 @@
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
@@ -62,6 +56,10 @@
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
/**
* Tests for {@link ArtifactFunction}.
@@ -185,8 +183,8 @@
file(input1.getPath(), "source contents");
evaluate(
Iterables.toArray(
- ArtifactValue.mandatoryKeys(ImmutableSet.of(input2, input1, input2)), SkyKey.class));
- ArtifactValue value = evaluateArtifactValue(output);
+ ArtifactSkyKey.mandatoryKeys(ImmutableSet.of(input2, input1, input2)), SkyKey.class));
+ SkyValue value = evaluateArtifactValue(output);
assertThat(((AggregatingArtifactValue) value).getInputs())
.containsExactly(Pair.of(input1, create(input1)), Pair.of(input2, create(input2)));
}
@@ -247,7 +245,7 @@
try {
value.getModifiedTime();
fail("mtime for non-empty file should not be stored.");
- } catch (IllegalStateException e) {
+ } catch (UnsupportedOperationException e) {
// Expected.
}
}
@@ -451,14 +449,13 @@
return ((FileArtifactValue) evaluateArtifactValue(artifact));
}
- private ArtifactValue evaluateArtifactValue(Artifact artifact) throws Throwable {
+ private SkyValue evaluateArtifactValue(Artifact artifact) throws Throwable {
return evaluateArtifactValue(artifact, /*isMandatory=*/ true);
}
- private ArtifactValue evaluateArtifactValue(Artifact artifact, boolean mandatory)
- throws Throwable {
- SkyKey key = ArtifactValue.key(artifact, mandatory);
- EvaluationResult<ArtifactValue> result = evaluate(ImmutableList.of(key).toArray(new SkyKey[0]));
+ private SkyValue evaluateArtifactValue(Artifact artifact, boolean mandatory) throws Throwable {
+ SkyKey key = ArtifactSkyKey.key(artifact, mandatory);
+ EvaluationResult<SkyValue> result = evaluate(ImmutableList.of(key).toArray(new SkyKey[0]));
if (result.hasError()) {
throw result.getError().getException();
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java
index 17a273e..de98941 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java
@@ -40,12 +40,6 @@
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
@@ -53,8 +47,11 @@
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.atomic.AtomicInteger;
-
import javax.annotation.Nullable;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
/** Tests for {@link SkyframeAwareAction}. */
@RunWith(JUnit4.class)
@@ -740,7 +737,7 @@
// ArtifactFunction(genfiles/gen0) | return FileValue(genfiles/foo:non-existent)
// CONFIGURED_TARGET://foo:gen0 |
// ACTION_EXECUTION:gen0_action | MockFunction()
- // return ArtifactValue(genfiles/gen0) | FILE:genfiles/foo
+ // return ArtifactSkyKey(genfiles/gen0) | FILE:genfiles/foo
// | FILE:genfiles/foo/bar/gen1
// ActionExecutionFunction(gen1_action) | env.valuesMissing():yes ==> return
// ARTIFACT:genfiles/gen0 |
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 de3984d..037a9e8 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
@@ -73,9 +73,6 @@
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
-
-import org.junit.Before;
-
import java.io.IOException;
import java.io.PrintStream;
import java.util.ArrayList;
@@ -88,8 +85,8 @@
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.atomic.AtomicReference;
-
import javax.annotation.Nullable;
+import org.junit.Before;
/**
* The common code that's shared between various builder tests.
@@ -203,9 +200,8 @@
return new Builder() {
private void setGeneratingActions() {
if (evaluator.getExistingValueForTesting(OWNER_KEY) == null) {
- differencer.inject(ImmutableMap.of(
- OWNER_KEY,
- new ActionLookupValue(ImmutableList.copyOf(actions))));
+ differencer.inject(
+ ImmutableMap.of(OWNER_KEY, new ActionLookupValue(ImmutableList.copyOf(actions))));
}
}
@@ -229,11 +225,12 @@
executor,
keepGoing, /*explain=*/
false,
- new ActionCacheChecker(actionCache, null, ALWAYS_EXECUTE_FILTER, false), null);
+ new ActionCacheChecker(actionCache, null, ALWAYS_EXECUTE_FILTER, false),
+ null);
List<SkyKey> keys = new ArrayList<>();
for (Artifact artifact : artifacts) {
- keys.add(ArtifactValue.key(artifact, true));
+ keys.add(ArtifactSkyKey.key(artifact, true));
}
setGeneratingActions();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
index 3fe2c2b..8d8b511 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
@@ -188,10 +188,9 @@
return output;
}
- private ArtifactValue evaluateArtifactValue(Artifact artifact, boolean mandatory)
- throws Exception {
- SkyKey key = ArtifactValue.key(artifact, mandatory);
- EvaluationResult<ArtifactValue> result = evaluate(key);
+ private SkyValue evaluateArtifactValue(Artifact artifact, boolean mandatory) throws Exception {
+ SkyKey key = ArtifactSkyKey.key(artifact, mandatory);
+ EvaluationResult<SkyValue> result = evaluate(key);
if (result.hasError()) {
throw result.getError().getException();
}
