Remove ArtifactFileMetadata and replace it with FileArtifactValue.
A harmful side effect is the loss of the "digest must be present" assertion on RegularFileArtifactValue. I'm not a fan of this, however, in practice:
- There are other kinds of FileArtifactValues and it's not clear whether they are supposed to have digests or not
- The important invariant, that ActionExecutionValue supplies a digest in all sane cases, is not upheld at HEAD because ArtifactFileMetadata is not guaranteed to contain one.
The game plan is to re-instate that check when ActionExecutionValue is constructed (somehow) after getting rid of the PLACEHOLDER silliness, if possible.
In exchange, however, we get a saner treatment of output directories: before this change, we created ArtifactFileMetadata instances for them even though they were not, well, files.
RELNOTES: None.
PiperOrigin-RevId: 260909718
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 4abf668..5c204bd 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
@@ -553,6 +553,11 @@
}
@Override
+ public FileContentsProxy getContentsProxy() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
public long getSize() {
return 0;
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFileMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFileMetadata.java
deleted file mode 100644
index 7756ae2..0000000
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFileMetadata.java
+++ /dev/null
@@ -1,202 +0,0 @@
-// Copyright 2014 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.actions;
-
-import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
-import com.google.devtools.build.lib.util.BigIntegerFingerprint;
-import com.google.devtools.build.lib.vfs.PathFragment;
-import java.math.BigInteger;
-import java.util.Objects;
-import javax.annotation.Nullable;
-
-/**
- * A value that represents a file for the purposes of up-to-dateness checks of actions.
- *
- * <p>It always stands for an actual file. In particular, tree artifacts and middlemen do not have a
- * corresponding {@link ArtifactFileMetadata}. However, the file is not necessarily present in the
- * file system; this happens when intermediate build outputs are not downloaded (and maybe when an
- * input artifact of an action is missing?)
- *
- * <p>It makes its main appearance in {@code ActionExecutionValue.artifactData}. It has two main
- * uses:
- *
- * <ul>
- * <li>This is how dependent actions get hold of the output metadata of their generated inputs. In
- * this case, it will be transformed into a {@link FileArtifactValue} by {@code
- * ArtifactFunction}.
- * <li>This is how {@code FileSystemValueChecker} figures out which actions need to be invalidated
- * (just propagating the invalidation up from leaf nodes is not enough, because the output
- * tree may have been changed while Blaze was not looking)
- * </ul>
- *
- * <p>It would probably be possible to unify this and {@link FileArtifactValue} since they contain
- * much the same data. However, {@link FileArtifactValue} has a few other uses that are do not map
- * easily to {@link ArtifactFileMetadata}, mostly relating to ActionFS.
- */
-@Immutable
-@ThreadSafe
-public abstract class ArtifactFileMetadata {
- /**
- * Used as as placeholder in {@code OutputStore.artifactData} for artifacts that have entries in
- * {@code OutputStore.additionalOutputData}.
- */
- @AutoCodec public static final ArtifactFileMetadata PLACEHOLDER = new PlaceholderFileValue();
-
- // No implementations outside this class.
- private ArtifactFileMetadata() {}
-
- public boolean exists() {
- return realFileStateValue().getType() != FileStateType.NONEXISTENT;
- }
-
- /**
- * Returns true if this value corresponds to a file or symlink to an existing regular or special
- * file. If so, its parent directory is guaranteed to exist.
- */
- public boolean isFile() {
- return realFileStateValue().getType() == FileStateType.REGULAR_FILE;
- }
-
- /**
- * Returns true if the file is a directory or a symlink to an existing directory. If so, its
- * parent directory is guaranteed to exist.
- */
- public boolean isDirectory() {
- return realFileStateValue().getType() == FileStateType.DIRECTORY;
- }
-
- protected abstract FileStateValue realFileStateValue();
-
- public FileContentsProxy getContentsProxy() {
- return realFileStateValue().getContentsProxy();
- }
-
- public long getSize() {
- Preconditions.checkState(isFile(), this);
- return realFileStateValue().getSize();
- }
-
- @Nullable
- public byte[] getDigest() {
- Preconditions.checkState(isFile(), this);
- return realFileStateValue().getDigest();
- }
-
- /** Returns a quick fingerprint via a BigInteger */
- public BigInteger getFingerprint() {
- BigIntegerFingerprint fp = new BigIntegerFingerprint();
-
- // TODO(lberki): This could be replaced with addLong(getType().ordinal())
- // at the cost of making the order of elements in the enum affecting the fingerprint.
- fp.addBoolean(realFileStateValue().getType() == FileStateType.NONEXISTENT);
- fp.addBoolean(realFileStateValue().getType() == FileStateType.SPECIAL_FILE);
- fp.addBoolean(realFileStateValue().getType() == FileStateType.DIRECTORY);
- fp.addBoolean(realFileStateValue().getType() == FileStateType.REGULAR_FILE);
-
- if (isFile()) {
- fp.addLong(getSize());
- fp.addDigestedBytes(getDigest());
- }
- return fp.getFingerprint();
- }
-
- public static ArtifactFileMetadata forRegularFile(
- PathFragment pathFragment, FileStateValue fileStateValue) {
- return new Regular(pathFragment, fileStateValue);
- }
-
- /** Non-stub implementation of {@link ArtifactFileMetadata}. */
- private static final class Regular extends ArtifactFileMetadata {
- private final PathFragment realPath;
- private final FileStateValue realFileStateValue;
-
- Regular(PathFragment realPath, FileStateValue realFileStateValue) {
- this.realPath = Preconditions.checkNotNull(realPath);
- this.realFileStateValue = Preconditions.checkNotNull(realFileStateValue);
- }
-
- @Override
- public FileStateValue realFileStateValue() {
- return realFileStateValue;
- }
-
- @Override
- public FileContentsProxy getContentsProxy() {
- return realFileStateValue.getContentsProxy();
- }
-
- @Override
- public boolean equals(Object obj) {
- if (obj == null) {
- return false;
- }
- if (obj.getClass() != Regular.class) {
- return false;
- }
- Regular other = (Regular) obj;
- return realPath.equals(other.realPath) && realFileStateValue.equals(other.realFileStateValue);
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(realPath, realFileStateValue);
- }
-
- @Override
- public String toString() {
- return realPath + ", " + realFileStateValue;
- }
-
- @Override
- public BigInteger getFingerprint() {
- BigInteger original = super.getFingerprint();
- BigIntegerFingerprint fp = new BigIntegerFingerprint();
- fp.addBigIntegerOrdered(original);
- fp.addString(getClass().getCanonicalName());
- fp.addPath(realPath);
- fp.addBigIntegerOrdered(realFileStateValue.getValueFingerprint());
- return fp.getFingerprint();
- }
- }
-
- private static final class PlaceholderFileValue extends ArtifactFileMetadata {
- private static final BigInteger FINGERPRINT =
- new BigIntegerFingerprint().addString("PlaceholderFileValue").getFingerprint();
-
- private PlaceholderFileValue() {}
-
- @Override
- public FileStateValue realFileStateValue() {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public FileContentsProxy getContentsProxy() {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public BigInteger getFingerprint() {
- return FINGERPRINT;
- }
-
- @Override
- public String toString() {
- return "PlaceholderFileValue:Singleton";
- }
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
index 6a78998..d7cbc28 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
@@ -36,44 +36,32 @@
import javax.annotation.Nullable;
/**
- * State of a file system object for the execution phase.
+ * A value that represents a file for the purposes of up-to-dateness checks of actions.
*
- * <p>This is not used by Skyframe for invalidation, it is primarily used by the action cache and
- * the various {@link com.google.devtools.build.lib.exec.SpawnRunner} implementations.
+ * <p>It always stands for an actual file. In particular, tree artifacts and middlemen do not have a
+ * corresponding {@link FileArtifactValue}. However, the file is not necessarily present in the file
+ * system; this happens when intermediate build outputs are not downloaded (and maybe when an input
+ * artifact of an action is missing?)
*
- * <p>We have the following cases:
+ * <p>It makes its main appearance in {@code ActionExecutionValue.artifactData}. It has two main
+ * uses:
*
* <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.
- * <li>a file that's only stored by a remote caching/execution system, in which case we would
- * expect to see a digest and size.
+ * <li>This is how dependent actions get hold of the output metadata of their generated inputs.
+ * <li>This is how {@code FileSystemValueChecker} figures out which actions need to be invalidated
+ * (just propagating the invalidation up from leaf nodes is not enough, because the output
+ * tree may have been changed while Blaze was not looking)
* </ul>
*/
@Immutable
@ThreadSafe
public abstract class FileArtifactValue implements SkyValue, HasDigest {
- @AutoCodec public static final FileArtifactValue DEFAULT_MIDDLEMAN = new SingletonMarkerValue();
- /** Data that marks that a file is not present on the filesystem. */
- @AutoCodec 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.
+ * Used as as placeholder in {@code OutputStore.artifactData} for artifacts that have entries in
+ * {@code OutputStore.additionalOutputData}.
*/
- @AutoCodec public static final FileArtifactValue OMITTED_FILE_MARKER = new OmittedFileValue();
-
- /**
- * Marker interface for singleton implementations of this class.
- *
- * <p>Needed for a correct implementation of {@code equals}.
- */
- public interface Singleton {}
+ @AutoCodec public static final FileArtifactValue PLACEHOLDER = new PlaceholderFileValue();
/**
* The type of the underlying file system object. If it is a regular file, then it is guaranteed
@@ -92,7 +80,7 @@
*
* <p>The return value is owned by this object and must not be modified.
*/
- @Nullable
+ @Override
public abstract byte[] getDigest();
/** Returns the file's size, or 0 if the underlying file system object is not a file. */
@@ -116,6 +104,7 @@
return null;
}
+
/**
* Index used to resolve remote files.
*
@@ -127,7 +116,7 @@
}
/** Returns {@code true} if this is a special marker as opposed to a representing a real file. */
- public final boolean isMarkerValue() {
+ public boolean isMarkerValue() {
return this instanceof Singleton;
}
@@ -180,6 +169,24 @@
}
}
+ public abstract FileContentsProxy getContentsProxy();
+
+ /**
+ * Marker interface for singleton implementations of this class.
+ *
+ * <p>Needed for a correct implementation of {@code equals}.
+ */
+ interface Singleton {}
+
+ @AutoCodec public static final FileArtifactValue DEFAULT_MIDDLEMAN = new SingletonMarkerValue();
+ /** Data that marks that a file is not present on the filesystem. */
+ @AutoCodec 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.
+ */
+ @AutoCodec public static final FileArtifactValue OMITTED_FILE_MARKER = new OmittedFileValue();
+
public static FileArtifactValue createForSourceArtifact(Artifact artifact, FileValue fileValue)
throws IOException {
Preconditions.checkState(artifact.isSourceArtifact());
@@ -195,20 +202,21 @@
}
@VisibleForTesting
- public static FileArtifactValue createForTesting(Artifact artifact, ArtifactFileMetadata metadata)
- throws IOException {
- boolean isFile = metadata.isFile();
+ public static FileArtifactValue injectDigestForTesting(
+ Artifact artifact, FileArtifactValue source) throws IOException {
+ boolean isFile = source.getType() == FileStateType.REGULAR_FILE;
+ FileContentsProxy proxy = source.getContentsProxy();
return create(
artifact.getPath(),
isFile,
- isFile ? metadata.getSize() : 0,
- isFile ? metadata.getContentsProxy() : null,
- isFile ? metadata.getDigest() : null,
+ isFile ? source.getSize() : 0,
+ proxy,
+ isFile ? source.getDigest() : null,
!artifact.isConstantMetadata());
}
public static FileArtifactValue createFromInjectedDigest(
- ArtifactFileMetadata metadata, @Nullable byte[] digest, boolean isShareable) {
+ FileArtifactValue metadata, @Nullable byte[] digest, boolean isShareable) {
return createForNormalFile(
digest, metadata.getContentsProxy(), metadata.getSize(), isShareable);
}
@@ -269,15 +277,6 @@
: new UnshareableRegularFileArtifactValue(digest, proxy, size);
}
- public static FileArtifactValue createFromMetadata(
- ArtifactFileMetadata artifactMetadata, boolean isShareable) {
- return createForNormalFile(
- artifactMetadata.getDigest(),
- artifactMetadata.getContentsProxy(),
- artifactMetadata.getSize(),
- isShareable);
- }
-
public static FileArtifactValue createForDirectoryWithHash(byte[] digest) {
return new HashedDirectoryArtifactValue(digest);
}
@@ -288,13 +287,17 @@
/**
* Creates a FileArtifactValue used as a 'proxy' input for other ArtifactValues. These are used in
- * {@link com.google.devtools.build.lib.actions.ActionCacheChecker}.
+ * {@link ActionCacheChecker}.
*/
public static FileArtifactValue createProxy(byte[] digest) {
Preconditions.checkNotNull(digest);
return createForNormalFile(digest, /*proxy=*/ null, /*size=*/ 0, /*isShareable=*/ true);
}
+ private static String bytesToString(byte[] bytes) {
+ return "0x" + BaseEncoding.base16().omitPadding().encode(bytes);
+ }
+
private static final class DirectoryArtifactValue extends FileArtifactValue {
private final long mtime;
@@ -314,6 +317,11 @@
}
@Override
+ public FileContentsProxy getContentsProxy() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
public BigInteger getValueFingerprint() {
BigIntegerFingerprint fp = new BigIntegerFingerprint();
fp.addString(getClass().getCanonicalName());
@@ -361,6 +369,11 @@
}
@Override
+ public FileContentsProxy getContentsProxy() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
public long getModifiedTime() {
return 0;
}
@@ -406,8 +419,9 @@
@Nullable private final FileContentsProxy proxy;
private final long size;
- private RegularFileArtifactValue(byte[] digest, @Nullable FileContentsProxy proxy, long size) {
- this.digest = Preconditions.checkNotNull(digest);
+ private RegularFileArtifactValue(
+ @Nullable byte[] digest, @Nullable FileContentsProxy proxy, long size) {
+ this.digest = digest;
this.proxy = proxy;
this.size = size;
}
@@ -423,6 +437,11 @@
}
@Override
+ public FileContentsProxy getContentsProxy() {
+ return proxy;
+ }
+
+ @Override
public long getSize() {
return size;
}
@@ -447,7 +466,8 @@
return MoreObjects.toStringHelper(this)
.add("digest", BaseEncoding.base16().lowerCase().encode(digest))
.add("size", size)
- .add("proxy", proxy).toString();
+ .add("proxy", proxy)
+ .toString();
}
@Override
@@ -468,7 +488,8 @@
@Override
public int hashCode() {
return (proxy != null ? 127 * proxy.hashCode() : 0)
- + 37 * Long.hashCode(getSize()) + Arrays.hashCode(getDigest());
+ + 37 * Long.hashCode(getSize())
+ + Arrays.hashCode(getDigest());
}
}
@@ -507,6 +528,11 @@
}
@Override
+ public FileContentsProxy getContentsProxy() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
public long getSize() {
return size;
}
@@ -542,10 +568,6 @@
}
}
- private static String bytesToString(byte[] bytes) {
- return "0x" + BaseEncoding.base16().omitPadding().encode(bytes);
- }
-
/** File stored inline in metadata. */
public static class InlineFileArtifactValue extends FileArtifactValue {
private final byte[] data;
@@ -557,7 +579,8 @@
}
private InlineFileArtifactValue(byte[] bytes) {
- this(bytes,
+ this(
+ bytes,
DigestHashFunction.getDefaultUnchecked().getHashFunction().hashBytes(bytes).asBytes());
}
@@ -582,6 +605,11 @@
}
@Override
+ public FileContentsProxy getContentsProxy() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
public long getSize() {
return data.length;
}
@@ -621,8 +649,7 @@
private final byte[] digest;
private final long size;
- public SourceFileArtifactValue(
- PathFragment execPath, byte[] digest, long size) {
+ public SourceFileArtifactValue(PathFragment execPath, byte[] digest, long size) {
this.execPath = Preconditions.checkNotNull(execPath);
this.digest = Preconditions.checkNotNull(digest);
this.size = size;
@@ -643,6 +670,11 @@
}
@Override
+ public FileContentsProxy getContentsProxy() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
public long getSize() {
return size;
}
@@ -671,6 +703,11 @@
}
@Override
+ public FileContentsProxy getContentsProxy() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
public long getSize() {
return 0;
}
@@ -709,6 +746,11 @@
}
@Override
+ public FileContentsProxy getContentsProxy() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
public long getSize() {
throw new UnsupportedOperationException();
}
@@ -728,4 +770,61 @@
return "OMITTED_FILE_MARKER";
}
}
+
+ private static final class PlaceholderFileValue extends FileArtifactValue implements Singleton {
+ private static final BigInteger FINGERPRINT =
+ new BigIntegerFingerprint().addString("PlaceholderFileValue").getFingerprint();
+
+ private PlaceholderFileValue() {}
+
+ @Override
+ public long getSize() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public byte[] getDigest() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public FileContentsProxy getContentsProxy() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public BigInteger getValueFingerprint() {
+ return FINGERPRINT;
+ }
+
+ @Override
+ public String toString() {
+ return "PlaceholderFileValue:Singleton";
+ }
+
+ @Override
+ public FileStateType getType() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public long getModifiedTime() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public boolean wasModifiedSinceDigest(Path path) {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public boolean isRemote() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public boolean isMarkerValue() {
+ throw new UnsupportedOperationException();
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
index 76fddb8..ffc04336 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
@@ -23,17 +23,14 @@
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper;
-import com.google.devtools.build.lib.actions.ArtifactFileMetadata;
import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.FileArtifactValue;
-import com.google.devtools.build.lib.actions.FileStateValue;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.util.BigIntegerFingerprint;
-import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyValue;
import java.math.BigInteger;
import java.util.Comparator;
@@ -64,9 +61,9 @@
* checked quickly against the actual filesystem on incremental builds.
*
* <p>If additional metadata is not needed here over what is in {@link #additionalOutputData}, the
- * value will be {@link ArtifactFileMetadata#PLACEHOLDER}.
+ * value will be {@link FileArtifactValue#PLACEHOLDER}.
*/
- private final ImmutableMap<Artifact, ArtifactFileMetadata> artifactData;
+ private final ImmutableMap<Artifact, FileArtifactValue> artifactData;
/** The TreeArtifactValue of all TreeArtifacts output by this Action. */
private final ImmutableMap<Artifact, TreeArtifactValue> treeArtifactData;
@@ -87,7 +84,7 @@
@Nullable private transient BigInteger valueFingerprint;
/**
- * @param artifactData Map from Artifacts to corresponding {@link ArtifactFileMetadata}.
+ * @param artifactData Map from Artifacts to corresponding {@link FileArtifactValue}.
* @param treeArtifactData All tree artifact data.
* @param additionalOutputData Map from Artifacts to values if the FileArtifactValue for this
* artifact cannot be derived from the corresponding FileValue (see {@link
@@ -97,7 +94,7 @@
* @param discoveredModules cpp modules discovered
*/
private ActionExecutionValue(
- Map<Artifact, ArtifactFileMetadata> artifactData,
+ Map<Artifact, FileArtifactValue> artifactData,
Map<Artifact, TreeArtifactValue> treeArtifactData,
Map<Artifact, FileArtifactValue> additionalOutputData,
@Nullable ImmutableList<FilesetOutputSymlink> outputSymlinks,
@@ -124,7 +121,7 @@
}
static ActionExecutionValue create(
- Map<Artifact, ArtifactFileMetadata> artifactData,
+ Map<Artifact, FileArtifactValue> artifactData,
Map<Artifact, TreeArtifactValue> treeArtifactData,
Map<Artifact, FileArtifactValue> additionalOutputData,
@Nullable ImmutableList<FilesetOutputSymlink> outputSymlinks,
@@ -155,7 +152,7 @@
* @return The data for each non-middleman output of this action, in the form of the {@link
* FileValue} that would be created for the file if it were to be read from disk.
*/
- ArtifactFileMetadata getData(Artifact artifact) {
+ FileArtifactValue getData(Artifact artifact) {
Preconditions.checkState(!additionalOutputData.containsKey(artifact),
"Should not be requesting data for already-constructed FileArtifactValue: %s", artifact);
return artifactData.get(artifact);
@@ -171,7 +168,7 @@
* returned by {@link #getData}. Primarily needed by {@link FilesystemValueChecker}, also
* called by {@link ArtifactFunction} when aggregating a {@link TreeArtifactValue}.
*/
- Map<Artifact, ArtifactFileMetadata> getAllFileValues() {
+ Map<Artifact, FileArtifactValue> getAllFileValues() {
return Maps.transformEntries(artifactData, this::transformIfPlaceholder);
}
@@ -202,7 +199,7 @@
.forEach(
(entry) -> {
fp.addPath(entry.getKey().getExecPath());
- fp.addBigIntegerOrdered(entry.getValue().getFingerprint());
+ fp.addBigIntegerOrdered(entry.getValue().getValueFingerprint());
});
sortMapByArtifactExecPathAndStream(treeArtifactData)
.forEach(
@@ -265,12 +262,11 @@
}
/** Transforms PLACEHOLDER values into normal instances. */
- private ArtifactFileMetadata transformIfPlaceholder(
- Artifact artifact, ArtifactFileMetadata value) {
+ private FileArtifactValue transformIfPlaceholder(Artifact artifact, FileArtifactValue value) {
Preconditions.checkState(!artifact.isTreeArtifact());
Preconditions.checkState(!artifact.isMiddlemanArtifact());
- if (value != ArtifactFileMetadata.PLACEHOLDER) {
+ if (value != FileArtifactValue.PLACEHOLDER) {
return value;
}
@@ -279,11 +275,8 @@
additionalOutputData.get(artifact),
"Placeholder without corresponding FileArtifactValue for: %s",
artifact);
- FileStateValue.RegularFileStateValue fileStateValue =
- new FileStateValue.RegularFileStateValue(
- metadata.getSize(), metadata.getDigest(), /*contentsProxy=*/ null);
- PathFragment pathFragment = artifact.getPath().asFragment();
- return ArtifactFileMetadata.forRegularFile(pathFragment, fileStateValue);
+
+ return metadata;
}
/**
@@ -293,7 +286,7 @@
private static final class CrossServerUnshareableActionExecutionValue
extends ActionExecutionValue {
CrossServerUnshareableActionExecutionValue(
- Map<Artifact, ArtifactFileMetadata> artifactData,
+ Map<Artifact, FileArtifactValue> artifactData,
Map<Artifact, TreeArtifactValue> treeArtifactData,
Map<Artifact, FileArtifactValue> additionalOutputData,
@Nullable ImmutableList<FilesetOutputSymlink> outputSymlinks,
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 4363a68..97ce7c6 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
@@ -27,10 +27,10 @@
import com.google.devtools.build.lib.actions.Artifact;
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.ArtifactFileMetadata;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
+import com.google.devtools.build.lib.actions.FileStateType;
import com.google.devtools.build.lib.actions.FileStateValue;
import com.google.devtools.build.lib.actions.cache.DigestUtils;
import com.google.devtools.build.lib.actions.cache.Md5Digest;
@@ -218,7 +218,7 @@
// Check for existing metadata. It may have been injected. In either case, this method is called
// from SkyframeActionExecutor to make sure that we have metadata for all action outputs, as the
// results are then stored in Skyframe (and the action cache).
- ArtifactFileMetadata fileMetadata = store.getArtifactData(artifact);
+ FileArtifactValue fileMetadata = store.getArtifactData(artifact);
if (fileMetadata != null) {
// Non-middleman artifacts should only have additionalOutputData if they have
// outputArtifactData. We don't assert this because of concurrency possibilities, but at least
@@ -229,10 +229,10 @@
if (value != null) {
return metadataFromValue(value);
}
- if (!fileMetadata.exists()) {
+ if (fileMetadata.getType() == FileStateType.NONEXISTENT) {
throw new FileNotFoundException(artifact.prettyPrint() + " does not exist");
}
- return FileArtifactValue.createFromMetadata(fileMetadata, !artifact.isConstantMetadata());
+ return fileMetadata;
}
// No existing metadata; this can happen if the output metadata is not injected after a spawn
@@ -247,7 +247,7 @@
//
// We only cache nonexistence here, not file system errors. It is unlikely that the file will be
// requested from this cache too many times.
- fileMetadata = constructArtifactFileMetadata(artifact, /*statNoFollow=*/ null);
+ fileMetadata = constructFileArtifactValue(artifact, /*statNoFollow=*/ null);
return maybeStoreAdditionalData(artifact, fileMetadata, null);
}
@@ -261,22 +261,22 @@
* additional data, even for normal (non-middleman) artifacts.
*/
private FileArtifactValue maybeStoreAdditionalData(
- Artifact artifact, ArtifactFileMetadata data, @Nullable byte[] injectedDigest)
+ Artifact artifact, FileArtifactValue data, @Nullable byte[] injectedDigest)
throws IOException {
- if (!data.exists()) {
+ if (data.getType() == FileStateType.NONEXISTENT) {
// Nonexistent files should only occur before executing an action.
throw new FileNotFoundException(artifact.prettyPrint() + " does not exist");
}
- boolean isFile = data.isFile();
+ boolean isFile = data.getType() == FileStateType.REGULAR_FILE;
if (isFile && !artifact.hasParent() && data.getDigest() != null) {
// We do not need to store the FileArtifactValue separately -- the digest is in the file value
// and that is all that is needed for this file's metadata.
- return FileArtifactValue.createFromMetadata(data, !artifact.isConstantMetadata());
+ return data;
}
final FileArtifactValue value;
- if (data.isDirectory()) {
+ if (data.getType() == FileStateType.DIRECTORY) {
// This branch is taken when the output of an action is a directory:
// - A Fileset (in this case, Blaze is correct)
// - A directory someone created in a local action (in this case, changes under the
@@ -382,14 +382,14 @@
for (TreeFileArtifact treeFileArtifact : contents) {
FileArtifactValue cachedValue = store.getAdditionalOutputData(treeFileArtifact);
if (cachedValue == null) {
- ArtifactFileMetadata fileMetadata = store.getArtifactData(treeFileArtifact);
+ FileArtifactValue fileMetadata = store.getArtifactData(treeFileArtifact);
// This is similar to what's present in getRealMetadataForArtifact, except
// we get back the ArtifactFileMetadata, not the metadata.
// We do not cache exceptions besides nonexistence here, because it is unlikely that the
// file will be requested from this cache too many times.
if (fileMetadata == null) {
try {
- fileMetadata = constructArtifactFileMetadata(treeFileArtifact, /*statNoFollow=*/ null);
+ fileMetadata = constructFileArtifactValue(treeFileArtifact, /*statNoFollow=*/ null);
} catch (FileNotFoundException e) {
String errorMessage = String.format(
"Failed to resolve relative path %s inside TreeArtifact %s. "
@@ -450,10 +450,10 @@
// Assumption: any non-Artifact output is 'virtual' and should be ignored here.
if (output instanceof Artifact) {
final Artifact artifact = (Artifact) output;
- // We have to add the artifact to injectedFiles before calling constructArtifactFileMetadata
+ // We have to add the artifact to injectedFiles before calling constructFileArtifactValue
// to avoid duplicate chmod calls.
store.injectedFiles().add(artifact);
- ArtifactFileMetadata fileMetadata;
+ FileArtifactValue fileMetadata;
try {
// This call may do an unnecessary call to Path#getFastDigest to see if the digest is
// readily available. We cannot pass the digest in, though, because if it is not available
@@ -461,8 +461,7 @@
// created for the
// same file, because the other one will be missing its digest.
fileMetadata =
- constructArtifactFileMetadata(
- artifact, FileStatusWithDigestAdapter.adapt(statNoFollow));
+ constructFileArtifactValue(artifact, FileStatusWithDigestAdapter.adapt(statNoFollow));
// Ensure the digest supplied matches the actual digest if it exists.
byte[] fileDigest = fileMetadata.getDigest();
if (fileDigest != null && !Arrays.equals(digest, fileDigest)) {
@@ -574,10 +573,10 @@
}
/**
- * Constructs a new {@link ArtifactFileMetadata}, saves it, and checks inconsistent data. This
- * calls chmod on the file if we're in executionMode.
+ * Constructs a new {@link FileArtifactValue}, saves it, and checks inconsistent data. This calls
+ * chmod on the file if we're in executionMode.
*/
- private ArtifactFileMetadata constructArtifactFileMetadata(
+ private FileArtifactValue constructFileArtifactValue(
Artifact artifact, @Nullable FileStatusWithDigest statNoFollow) throws IOException {
// We first chmod the output files before we construct the FileContentsProxy. The proxy may use
// ctime, which is affected by chmod.
@@ -586,23 +585,47 @@
setPathReadOnlyAndExecutable(artifact);
}
- ArtifactFileMetadata value =
- fileMetadataFromArtifact(
+ FileArtifactValue value =
+ fileArtifactValueFromArtifact(
artifact, artifactPathResolver, statNoFollow, getTimestampGranularityMonitor(artifact));
store.putArtifactData(artifact, value);
return value;
}
+ private static FileArtifactValue fileArtifactValueFromStat(
+ RootedPath rootedPath,
+ FileStatusWithDigest stat,
+ boolean isConstantMetadata,
+ TimestampGranularityMonitor tsgm)
+ throws IOException {
+ if (stat == null) {
+ return FileArtifactValue.MISSING_FILE_MARKER;
+ }
+
+ FileStateValue fileStateValue = FileStateValue.createWithStatNoFollow(rootedPath, stat, tsgm);
+
+ if (stat.isDirectory()) {
+ return FileArtifactValue.createForDirectoryWithMtime(stat.getLastModifiedTime());
+ } else {
+ return FileArtifactValue.createForNormalFile(
+ fileStateValue.getDigest(),
+ fileStateValue.getContentsProxy(),
+ stat.getSize(),
+ !isConstantMetadata);
+ }
+ }
+
@VisibleForTesting
- static ArtifactFileMetadata fileMetadataFromArtifact(
+ static FileArtifactValue fileArtifactValueFromArtifact(
Artifact artifact,
@Nullable FileStatusWithDigest statNoFollow,
@Nullable TimestampGranularityMonitor tsgm)
throws IOException {
- return fileMetadataFromArtifact(artifact, ArtifactPathResolver.IDENTITY, statNoFollow, tsgm);
+ return fileArtifactValueFromArtifact(
+ artifact, ArtifactPathResolver.IDENTITY, statNoFollow, tsgm);
}
- private static ArtifactFileMetadata fileMetadataFromArtifact(
+ private static FileArtifactValue fileArtifactValueFromArtifact(
Artifact artifact,
ArtifactPathResolver artifactPathResolver,
@Nullable FileStatusWithDigest statNoFollow,
@@ -611,31 +634,25 @@
Preconditions.checkState(!artifact.isTreeArtifact());
Preconditions.checkState(!artifact.isMiddlemanArtifact());
- Path path = artifactPathResolver.toPath(artifact);
- PathFragment pathFragment = path.asFragment();
- RootedPath rootedPath =
+ Path pathNoFollow = artifactPathResolver.toPath(artifact);
+ RootedPath rootedPathNoFollow =
RootedPath.toRootedPath(
artifactPathResolver.transformRoot(artifact.getRoot().getRoot()),
artifact.getRootRelativePath());
if (statNoFollow == null) {
- statNoFollow = FileStatusWithDigestAdapter.adapt(path.statIfFound(Symlinks.NOFOLLOW));
- if (statNoFollow == null) {
- return ArtifactFileMetadata.forRegularFile(
- pathFragment, FileStateValue.NONEXISTENT_FILE_STATE_NODE);
- }
+ statNoFollow = FileStatusWithDigestAdapter.adapt(pathNoFollow.statIfFound(Symlinks.NOFOLLOW));
}
- FileStateValue fileStateValue =
- FileStateValue.createWithStatNoFollow(rootedPath, statNoFollow, tsgm);
- if (!statNoFollow.isSymbolicLink()) {
- return ArtifactFileMetadata.forRegularFile(path.asFragment(), fileStateValue);
+ if (statNoFollow == null || !statNoFollow.isSymbolicLink()) {
+ return fileArtifactValueFromStat(
+ rootedPathNoFollow, statNoFollow, artifact.isConstantMetadata(), tsgm);
}
// We use FileStatus#isSymbolicLink over Path#isSymbolicLink to avoid the unnecessary stat
// done by the latter. We need to protect against symlink cycles since
// ArtifactFileMetadata#value assumes it's dealing with a file that's not in a symlink cycle.
- Path realPath = path.resolveSymbolicLinks();
- if (realPath.equals(path)) {
+ Path realPath = pathNoFollow.resolveSymbolicLinks();
+ if (realPath.equals(pathNoFollow)) {
throw new IOException("symlink cycle");
}
@@ -646,8 +663,10 @@
// TODO(bazel-team): consider avoiding a 'stat' here when the symlink target hasn't changed
// and is a source file (since changes to those are checked separately).
- FileStateValue realFileStateValue = FileStateValue.create(realRootedPath, tsgm);
- return ArtifactFileMetadata.forRegularFile(realPath.asFragment(), realFileStateValue);
+ FileStatus realStat = realRootedPath.asPath().statIfFound(Symlinks.NOFOLLOW);
+ FileStatusWithDigest realStatWithDigest = FileStatusWithDigestAdapter.adapt(realStat);
+ return fileArtifactValueFromStat(
+ realRootedPath, realStatWithDigest, artifact.isConstantMetadata(), tsgm);
}
private void setPathReadOnlyAndExecutable(Artifact artifact) throws IOException {
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 d66c150..0e32d6f 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
@@ -27,9 +27,9 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
-import com.google.devtools.build.lib.actions.ArtifactFileMetadata;
import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.actions.FileStateType;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.actions.FilesetTraversalParams.DirectTraversalRoot;
import com.google.devtools.build.lib.actions.FilesetTraversalParams.PackageBoundaryMode;
@@ -294,14 +294,18 @@
if (value != null) {
return value;
}
- ArtifactFileMetadata data =
+ FileArtifactValue data =
Preconditions.checkNotNull(actionValue.getData(artifact), "%s %s", artifact, actionValue);
Preconditions.checkNotNull(
data.getDigest(), "Digest should already have been calculated for %s (%s)", artifact, data);
// Directories are special-cased because their mtimes are used, so should have been constructed
// during execution of the action (in ActionMetadataHandler#maybeStoreAdditionalData).
- Preconditions.checkState(data.isFile(), "Unexpected not file %s (%s)", artifact, data);
- return FileArtifactValue.createFromMetadata(data, !artifact.isConstantMetadata());
+ Preconditions.checkState(
+ data.getType() == FileStateType.REGULAR_FILE,
+ "Unexpected not file %s (%s)",
+ artifact,
+ data);
+ return data;
}
@Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
index d880c64..40bf9fd 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
@@ -25,8 +25,8 @@
import com.google.common.collect.Sets;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.ArtifactFileMetadata;
import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.actions.FileStateType;
import com.google.devtools.build.lib.concurrent.ExecutorUtil;
import com.google.devtools.build.lib.concurrent.Sharder;
import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrapper;
@@ -304,10 +304,10 @@
Pair<SkyKey, ActionExecutionValue> keyAndValue = fileToKeyAndValue.get(artifact);
ActionExecutionValue actionValue = keyAndValue.getSecond();
SkyKey key = keyAndValue.getFirst();
- ArtifactFileMetadata lastKnownData = actionValue.getAllFileValues().get(artifact);
+ FileArtifactValue lastKnownData = actionValue.getAllFileValues().get(artifact);
try {
- ArtifactFileMetadata newData =
- ActionMetadataHandler.fileMetadataFromArtifact(artifact, stat, tsgm);
+ FileArtifactValue newData =
+ ActionMetadataHandler.fileArtifactValueFromArtifact(artifact, stat, tsgm);
if (!newData.equals(lastKnownData)) {
updateIntraBuildModifiedCounter(stat != null ? stat.getLastChangeTime() : -1);
modifiedOutputFilesCounter.getAndIncrement();
@@ -403,20 +403,22 @@
ImmutableSet<PathFragment> knownModifiedOutputFiles,
Supplier<NavigableSet<PathFragment>> sortedKnownModifiedOutputFiles) {
boolean isDirty = false;
- for (Map.Entry<Artifact, ArtifactFileMetadata> entry :
- actionValue.getAllFileValues().entrySet()) {
+ for (Map.Entry<Artifact, FileArtifactValue> entry : actionValue.getAllFileValues().entrySet()) {
Artifact file = entry.getKey();
- ArtifactFileMetadata lastKnownData = entry.getValue();
+ FileArtifactValue lastKnownData = entry.getValue();
if (shouldCheckFile(knownModifiedOutputFiles, file)) {
try {
- ArtifactFileMetadata fileMetadata =
- ActionMetadataHandler.fileMetadataFromArtifact(file, null, tsgm);
+ FileArtifactValue fileMetadata =
+ ActionMetadataHandler.fileArtifactValueFromArtifact(file, null, tsgm);
FileArtifactValue fileValue = actionValue.getArtifactValue(file);
boolean lastSeenRemotely = (fileValue != null) && fileValue.isRemote();
- boolean trustRemoteValue = !fileMetadata.exists() && lastSeenRemotely;
+ boolean trustRemoteValue =
+ fileMetadata.getType() == FileStateType.NONEXISTENT && lastSeenRemotely;
if (!trustRemoteValue && !fileMetadata.equals(lastKnownData)) {
updateIntraBuildModifiedCounter(
- fileMetadata.exists() ? file.getPath().getLastModifiedTime(Symlinks.FOLLOW) : -1);
+ fileMetadata.getType() != FileStateType.NONEXISTENT
+ ? file.getPath().getLastModifiedTime(Symlinks.FOLLOW)
+ : -1);
modifiedOutputFilesCounter.getAndIncrement();
isDirty = true;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java
index 7f07c14..0c9b9ca 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java
@@ -15,7 +15,6 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
-import com.google.devtools.build.lib.actions.ArtifactFileMetadata;
import com.google.devtools.build.lib.actions.FileArtifactValue;
/**
@@ -35,7 +34,7 @@
}
@Override
- void putArtifactData(Artifact artifact, ArtifactFileMetadata value) {}
+ void putArtifactData(Artifact artifact, FileArtifactValue value) {}
@Override
void addTreeArtifactContents(Artifact artifact, TreeFileArtifact contents) {}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java
index 60a0ef2..ed034bd 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java
@@ -18,7 +18,6 @@
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
-import com.google.devtools.build.lib.actions.ArtifactFileMetadata;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import java.util.Set;
@@ -40,8 +39,7 @@
@ThreadSafe
class OutputStore {
- private final ConcurrentMap<Artifact, ArtifactFileMetadata> artifactData =
- new ConcurrentHashMap<>();
+ private final ConcurrentMap<Artifact, FileArtifactValue> artifactData = new ConcurrentHashMap<>();
private final ConcurrentMap<Artifact, TreeArtifactValue> treeArtifactData =
new ConcurrentHashMap<>();
@@ -55,21 +53,21 @@
private final Set<Artifact> injectedFiles = Sets.newConcurrentHashSet();
@Nullable
- final ArtifactFileMetadata getArtifactData(Artifact artifact) {
+ final FileArtifactValue getArtifactData(Artifact artifact) {
return artifactData.get(artifact);
}
- void putArtifactData(Artifact artifact, ArtifactFileMetadata value) {
+ void putArtifactData(Artifact artifact, FileArtifactValue value) {
artifactData.put(artifact, value);
}
/**
* Returns data for output files that was computed during execution.
*
- * <p>The value is {@link ArtifactFileMetadata#PLACEHOLDER} if the artifact's metadata is not
- * fully captured in {@link #additionalOutputData}.
+ * <p>The value is {@link FileArtifactValue#PLACEHOLDER} if the artifact's metadata is not fully
+ * captured in {@link #additionalOutputData}.
*/
- final ImmutableMap<Artifact, ArtifactFileMetadata> getAllArtifactData() {
+ final ImmutableMap<Artifact, FileArtifactValue> getAllArtifactData() {
return ImmutableMap.copyOf(artifactData);
}
@@ -111,7 +109,7 @@
* <li>If the filesystem does not possess fast digests, then we will have additional output data
* for practically every artifact, since we will need to store their digests.
* <li>If we have a remote execution service injecting metadata, then we will just store that
- * metadata here, and put {@link ArtifactFileMetadata#PLACEHOLDER} objects into {@link
+ * metadata here, and put {@link FileArtifactValue#PLACEHOLDER} objects into {@link
* #outputArtifactData} if the filesystem supports fast digests, and the actual metadata if
* the filesystem does not support fast digests.
* <li>If the filesystem has fast digests <i>but</i> there is no remote execution injecting
@@ -123,13 +121,13 @@
* populated. Locally executed actions are the exception to this rule inside Google.
*
* <p>Moreover, there are some artifacts that are always stored here. First, middleman artifacts
- * have no corresponding {@link ArtifactFileMetadata}. Second, directories' metadata contain their
- * mtimes, which the {@link ArtifactFileMetadata} does not expose, so that has to be stored
+ * have no corresponding {@link FileArtifactValue}. Second, directories' metadata contain their
+ * mtimes, which the {@link FileArtifactValue} does not expose, so that has to be stored
* separately.
*
* <p>Note that for files that need digests, we can't easily inject the digest in the {@link
- * ArtifactFileMetadata} because it would complicate equality-checking on subsequent builds -- if
- * our filesystem doesn't do fast digests, the comparison value would not have a digest.
+ * FileArtifactValue} because it would complicate equality-checking on subsequent builds -- if our
+ * filesystem doesn't do fast digests, the comparison value would not have a digest.
*/
final ImmutableMap<Artifact, FileArtifactValue> getAllAdditionalOutputData() {
return ImmutableMap.copyOf(additionalOutputData);
@@ -167,7 +165,7 @@
// While `artifactValue` carries the important information, consumers also require an entry in
// `artifactData` so a `PLACEHOLDER` is added to `artifactData`.
- artifactData.put(output, ArtifactFileMetadata.PLACEHOLDER);
+ artifactData.put(output, FileArtifactValue.PLACEHOLDER);
additionalOutputData.put(output, artifactValue);
}
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
index 5402175..c67e7a1 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
@@ -193,6 +193,21 @@
}
@Override
+ public boolean isRemote() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public boolean isMarkerValue() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public FileContentsProxy getContentsProxy() {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
@SuppressWarnings("EqualsHashCode")
public boolean equals(Object o) {
if (!(o instanceof TestMetadata)) {
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 07a020b..9b1e112 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
@@ -31,7 +31,6 @@
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.Artifact.TreeFileArtifact;
-import com.google.devtools.build.lib.actions.ArtifactFileMetadata;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.BasicActionLookupValue;
import com.google.devtools.build.lib.actions.FileArtifactValue;
@@ -263,8 +262,8 @@
ActionLookupData dummyData = ActionLookupData.create(ALL_OWNER, 0);
Artifact.DerivedArtifact artifact1 = createDerivedArtifact("one");
Artifact.DerivedArtifact artifact2 = createDerivedArtifact("two");
- ArtifactFileMetadata metadata1 =
- ActionMetadataHandler.fileMetadataFromArtifact(artifact1, null, null);
+ FileArtifactValue metadata1 =
+ ActionMetadataHandler.fileArtifactValueFromArtifact(artifact1, null, null);
SpecialArtifact treeArtifact = createDerivedTreeArtifactOnly("tree");
treeArtifact.setGeneratingActionKey(dummyData);
TreeFileArtifact treeFileArtifact = ActionInputHelper.treeFileArtifact(treeArtifact, "subpath");
@@ -281,7 +280,7 @@
PathFragment.EMPTY_FRAGMENT, PathFragment.EMPTY_FRAGMENT, PathFragment.EMPTY_FRAGMENT);
ActionExecutionValue actionExecutionValue =
ActionExecutionValue.create(
- ImmutableMap.of(artifact1, metadata1, artifact2, ArtifactFileMetadata.PLACEHOLDER),
+ ImmutableMap.of(artifact1, metadata1, artifact2, FileArtifactValue.PLACEHOLDER),
ImmutableMap.of(treeArtifact, treeArtifactValue),
ImmutableMap.of(artifact3, FileArtifactValue.DEFAULT_MIDDLEMAN),
ImmutableList.of(filesetOutputSymlink),
@@ -289,7 +288,7 @@
true);
ActionExecutionValue valueWithFingerprint =
ActionExecutionValue.create(
- ImmutableMap.of(artifact1, metadata1, artifact2, ArtifactFileMetadata.PLACEHOLDER),
+ ImmutableMap.of(artifact1, metadata1, artifact2, FileArtifactValue.PLACEHOLDER),
ImmutableMap.of(treeArtifact, treeArtifactValue),
ImmutableMap.of(artifact3, FileArtifactValue.DEFAULT_MIDDLEMAN),
ImmutableList.of(filesetOutputSymlink),
@@ -417,7 +416,7 @@
private static class SimpleActionExecutionFunction implements SkyFunction {
@Override
public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
- Map<Artifact, ArtifactFileMetadata> artifactData = new HashMap<>();
+ Map<Artifact, FileArtifactValue> artifactData = new HashMap<>();
Map<Artifact, TreeArtifactValue> treeArtifactData = new HashMap<>();
Map<Artifact, FileArtifactValue> additionalOutputData = new HashMap<>();
ActionLookupData actionLookupData = (ActionLookupData) skyKey.argument();
@@ -439,10 +438,11 @@
treeFileArtifact2, FileArtifactValue.createForTesting(treeFileArtifact2)));
treeArtifactData.put(output, treeArtifactValue);
} else if (action.getActionType() == MiddlemanType.NORMAL) {
- ArtifactFileMetadata fileValue =
- ActionMetadataHandler.fileMetadataFromArtifact(output, null, null);
+ FileArtifactValue fileValue =
+ ActionMetadataHandler.fileArtifactValueFromArtifact(output, null, null);
artifactData.put(output, fileValue);
- additionalOutputData.put(output, FileArtifactValue.createForTesting(output, fileValue));
+ additionalOutputData.put(
+ output, FileArtifactValue.injectDigestForTesting(output, fileValue));
} else {
additionalOutputData.put(output, FileArtifactValue.DEFAULT_MIDDLEMAN);
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
index 55e4524..4d9e29a 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
@@ -30,7 +30,6 @@
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.Artifact.TreeFileArtifact;
-import com.google.devtools.build.lib.actions.ArtifactFileMetadata;
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;
@@ -729,14 +728,14 @@
// Presently these appear to be untested.
private ActionExecutionValue actionValue(Action action, boolean forceDigest) {
- Map<Artifact, ArtifactFileMetadata> artifactData = new HashMap<>();
+ Map<Artifact, FileArtifactValue> artifactData = new HashMap<>();
for (Artifact output : action.getOutputs()) {
try {
Path path = output.getPath();
FileStatusWithDigest stat =
forceDigest ? statWithDigest(path, path.statIfFound(Symlinks.NOFOLLOW)) : null;
artifactData.put(
- output, ActionMetadataHandler.fileMetadataFromArtifact(output, stat, null));
+ output, ActionMetadataHandler.fileArtifactValueFromArtifact(output, stat, null));
} catch (IOException e) {
throw new IllegalStateException(e);
}
@@ -764,7 +763,7 @@
}
private ActionExecutionValue actionValueWithTreeArtifacts(List<TreeFileArtifact> contents) {
- Map<Artifact, ArtifactFileMetadata> fileData = new HashMap<>();
+ Map<Artifact, FileArtifactValue> fileData = new HashMap<>();
Map<Artifact, Map<TreeFileArtifact, FileArtifactValue>> directoryData = new HashMap<>();
for (TreeFileArtifact output : contents) {
@@ -775,9 +774,9 @@
dirDatum = new HashMap<>();
directoryData.put(output.getParent(), dirDatum);
}
- ArtifactFileMetadata fileValue =
- ActionMetadataHandler.fileMetadataFromArtifact(output, null, null);
- dirDatum.put(output, FileArtifactValue.createForTesting(output, fileValue));
+ FileArtifactValue fileValue =
+ ActionMetadataHandler.fileArtifactValueFromArtifact(output, null, null);
+ dirDatum.put(output, FileArtifactValue.injectDigestForTesting(output, fileValue));
fileData.put(output, fileValue);
} catch (IOException e) {
throw new IllegalStateException(e);
@@ -822,7 +821,7 @@
private ActionExecutionValue actionValueWithRemoteArtifact(
Artifact output, RemoteFileArtifactValue value) {
return ActionExecutionValue.create(
- Collections.singletonMap(output, ArtifactFileMetadata.PLACEHOLDER),
+ Collections.singletonMap(output, FileArtifactValue.PLACEHOLDER),
ImmutableMap.of(),
Collections.singletonMap(output, value),
/* outputSymlinks= */ null,
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 adb9788..76407e4 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
@@ -32,7 +32,6 @@
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.Artifact.TreeFileArtifact;
-import com.google.devtools.build.lib.actions.ArtifactFileMetadata;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.BasicActionLookupValue;
import com.google.devtools.build.lib.actions.FileArtifactValue;
@@ -279,7 +278,7 @@
@Override
public SkyValue compute(SkyKey skyKey, Environment env)
throws SkyFunctionException, InterruptedException {
- Map<Artifact, ArtifactFileMetadata> fileData = new HashMap<>();
+ Map<Artifact, FileArtifactValue> fileData = new HashMap<>();
Map<TreeFileArtifact, FileArtifactValue> treeArtifactData = new HashMap<>();
ActionLookupData actionLookupData = (ActionLookupData) skyKey.argument();
ActionLookupValue actionLookupValue =
@@ -289,10 +288,11 @@
for (PathFragment subpath : testTreeArtifactContents) {
try {
TreeFileArtifact suboutput = ActionInputHelper.treeFileArtifact(output, subpath);
- ArtifactFileMetadata fileValue =
- ActionMetadataHandler.fileMetadataFromArtifact(suboutput, null, null);
+ FileArtifactValue fileValue =
+ ActionMetadataHandler.fileArtifactValueFromArtifact(suboutput, null, null);
fileData.put(suboutput, fileValue);
- treeArtifactData.put(suboutput, FileArtifactValue.createForTesting(suboutput, fileValue));
+ treeArtifactData.put(
+ suboutput, FileArtifactValue.injectDigestForTesting(suboutput, fileValue));
} catch (IOException e) {
throw new SkyFunctionException(e, Transience.TRANSIENT) {};
}
