Add runfiles to metadata caches and make them isFile aware

--
MOS_MIGRATED_REVID=94318260

16 files changed, 169 insertions, 10 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
index f686334815..7db67c409a 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -59,6 +59,7 @@ public abstract class AbstractAction implements Action {
   private final ActionOwner owner;
   // The variable inputs is non-final only so that actions that discover their inputs can modify it.
   private Iterable<Artifact> inputs;
+  private final RunfilesSupplier runfilesSupplier;
   private final ImmutableSet<Artifact> outputs;
 
   private String cachedKey;
@@ -69,11 +70,20 @@ public abstract class AbstractAction implements Action {
   protected AbstractAction(ActionOwner owner,
                            Iterable<Artifact> inputs,
                            Iterable<Artifact> outputs) {
+    this(owner, inputs, EmptyRunfilesSupplier.INSTANCE, outputs);
+  }
+
+  protected AbstractAction(ActionOwner owner,
+      Iterable<Artifact> inputs,
+      RunfilesSupplier runfilesSupplier,
+      Iterable<Artifact> outputs) {
     Preconditions.checkNotNull(owner);
     // TODO(bazel-team): Use RuleContext.actionOwner here instead
     this.owner = new ActionOwnerDescription(owner);
     this.inputs = CollectionUtils.makeImmutable(inputs);
     this.outputs = ImmutableSet.copyOf(outputs);
+    this.runfilesSupplier = Preconditions.checkNotNull(runfilesSupplier,
+        "runfilesSupplier may not be null");
     Preconditions.checkArgument(!this.outputs.isEmpty(), owner);
   }
 
@@ -123,6 +133,11 @@ public abstract class AbstractAction implements Action {
     return inputs;
   }
 
+  @Override
+  public RunfilesSupplier getRunfilesSupplier() {
+    return runfilesSupplier;
+  }
+
   /**
    * Set the inputs of the action. May only be used by an action that {@link #discoversInputs()}.
    * The iterable passed in is automatically made immutable.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputFileCache.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputFileCache.java
index 79c8785e92..8815b9fd6f 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputFileCache.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputFileCache.java
@@ -46,6 +46,14 @@ public interface ActionInputFileCache {
   ByteString getDigest(ActionInput input) throws IOException;
 
   /**
+   * Retrieves whether or not the input Artifact is a file or symlink to an existing file.
+   *
+   * @param input the input
+   * @return true if input is a file or symlink to an existing file, otherwise false
+   */
+  boolean isFile(Artifact input);
+
+  /**
    * Retrieve the size of the file at the given path. Will usually return 0 on failure instead of
    * throwing an IOException. Returns 0 for files inaccessible to user, but available to the
    * execution environment.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ActionMetadata.java
index b941a590e0..27ecc51cb1 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionMetadata.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionMetadata.java
@@ -108,6 +108,11 @@ public interface ActionMetadata {
   Iterable<Artifact> getInputs();
 
   /**
+   * Get the {@link RunfilesSupplier} providing runfiles needed by this action.
+   */
+  RunfilesSupplier getRunfilesSupplier();
+
+  /**
    * Returns the (unordered, immutable) set of output Artifacts that
    * this action generates.  (It would not make sense for this to be empty.)
    */
diff --git a/src/main/java/com/google/devtools/build/lib/actions/EmptyRunfilesSupplier.java b/src/main/java/com/google/devtools/build/lib/actions/EmptyRunfilesSupplier.java
index 506c5e3f3c..87f28e73ce 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/EmptyRunfilesSupplier.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/EmptyRunfilesSupplier.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.actions;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -28,6 +29,11 @@ public class EmptyRunfilesSupplier implements RunfilesSupplier {
   private EmptyRunfilesSupplier() {}
 
   @Override
+  public Iterable<Artifact> getArtifacts() {
+    return ImmutableList.<Artifact>of();
+  }
+
+  @Override
   public ImmutableSet<PathFragment> getRunfilesDirs() {
     return ImmutableSet.of();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java b/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java
index c2ef8ceb66..204afc4de9 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java
@@ -24,6 +24,9 @@ import java.util.Map;
 /** Convenience wrapper around runfiles allowing lazy expansion. */
 public interface RunfilesSupplier {
 
+  /** @return the contained artifacts */
+  Iterable<Artifact> getArtifacts();
+
   /** @return the runfiles' root directories. */
   ImmutableSet<PathFragment> getRunfilesDirs();
 
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/DigestUtils.java b/src/main/java/com/google/devtools/build/lib/actions/cache/DigestUtils.java
index 7295fb5317..0b7f7abcd7 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/DigestUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/DigestUtils.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.actions.cache;
 
 import com.google.common.base.Preconditions;
 import com.google.common.io.BaseEncoding;
-import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.util.BlazeClock;
@@ -41,11 +40,10 @@ public class DigestUtils {
   /**
    * Returns true iff using MD5 digests is appropriate for an artifact.
    *
-   * @param artifact Artifact in question.
    * @param isFile whether or not Artifact is a file versus a directory, isFile() on its stat.
    * @param size size of Artifact on filesystem in bytes, getSize() on its stat.
    */
-  public static boolean useFileDigest(Artifact artifact, boolean isFile, long size) {
+  public static boolean useFileDigest(boolean isFile, long size) {
     // Use timestamps for directories. Use digests for everything else.
     return isFile && size != 0;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
index 2dba69c303..d2da3c65e8 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
@@ -252,7 +252,7 @@ public final class Runfiles {
 
   /**
    * Returns the collection of runfiles as artifacts, including both unconditional artifacts
-   * and pruning manifest candidates, but not including any artifacts produced by expansion.
+   * and pruning manifest candidates.
    */
   public NestedSet<Artifact> getArtifacts() {
     NestedSetBuilder<Artifact> allArtifacts = NestedSetBuilder.stableOrder();
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupplierImpl.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupplierImpl.java
index cfb84c2bbf..ccdc89021d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupplierImpl.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupplierImpl.java
@@ -48,6 +48,17 @@ public class RunfilesSupplierImpl implements RunfilesSupplier {
   }
 
   @Override
+  public Iterable<Artifact> getArtifacts() {
+    ImmutableSet.Builder<Artifact> builder = ImmutableSet.builder();
+    for (Entry<PathFragment, Runfiles> entry : inputRunfiles.entrySet()) {
+      // TODO(bazel-team): We can likely do without middlemen here, but we should filter that at
+      // the Runfiles level.
+      builder.addAll(entry.getValue().getAllArtifacts());
+    }
+    return builder.build();
+  }
+
+  @Override
   public ImmutableSet<PathFragment> getRunfilesDirs() {
     return inputRunfiles.keySet();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java b/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java
index cbe30564be..09b0eb8425 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java
@@ -23,6 +23,7 @@ import com.google.common.collect.Maps;
 import com.google.common.io.BaseEncoding;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputFileCache;
+import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.DigestOfDirectoryException;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
@@ -114,6 +115,12 @@ public class SingleBuildFileCache implements ActionInputFileCache {
   }
 
   @Override
+  public boolean isFile(Artifact input) {
+    // We shouldn't fall back on this functionality ever.
+    throw new UnsupportedOperationException();
+  }
+
+  @Override
   public boolean contentsAvailableLocally(ByteString digest) {
     return digestToPath.containsKey(digest);
   }
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 770899e346..fec72255c2 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
@@ -179,9 +179,12 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
    */
   @Nullable
   private AllInputs collectInputs(Action action, Environment env) {
+    Iterable<Artifact> allKnownInputs = Iterables.concat(
+        action.getInputs(), action.getRunfilesSupplier().getArtifacts());
     if (action.inputsKnown()) {
-      return new AllInputs(action.getInputs());
+      return new AllInputs(allKnownInputs);
     }
+
     Preconditions.checkState(action.discoversInputs(), action);
     PackageRootResolverWithEnvironment resolver = new PackageRootResolverWithEnvironment(env);
     Iterable<Artifact> actionCacheInputs =
@@ -190,7 +193,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
       Preconditions.checkState(env.valuesMissing(), action);
       return null;
     }
-    return new AllInputs(action.getInputs(), actionCacheInputs, resolver.keysRequested);
+    return new AllInputs(allKnownInputs, actionCacheInputs, resolver.keysRequested);
   }
 
   private static class AllInputs {
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 7275222664..59224fbdc2 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
@@ -199,7 +199,7 @@ public class ActionMetadataHandler implements MetadataHandler {
       throw new FileNotFoundException(artifact.prettyPrint() + " does not exist");
     }
     boolean isFile = data.isFile();
-    boolean useDigest = DigestUtils.useFileDigest(artifact, isFile, isFile ? data.getSize() : 0);
+    boolean useDigest = DigestUtils.useFileDigest(isFile, isFile ? data.getSize() : 0);
     if (useDigest && 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.
@@ -301,7 +301,7 @@ public class ActionMetadataHandler implements MetadataHandler {
     // Currently this method is used only for genrule input directory checks. If we need to call
     // this on output artifacts too, this could be more efficient.
     FileArtifactValue value = getInputFileArtifactValue(artifact);
-    if (value != null && value.getDigest() != null) {
+    if (value != null && value.isFile()) {
       return true;
     }
     return artifact.getPath().isFile();
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 bac115fd2d..776e1c7d2b 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
@@ -42,6 +42,7 @@ public class FileArtifactValue extends ArtifactValue {
    */
   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; }
@@ -87,7 +88,7 @@ public class FileArtifactValue extends ArtifactValue {
     if (isFile && digest == null) {
       digest = DigestUtils.getDigestOrFail(artifact.getPath(), size);
     }
-    if (!DigestUtils.useFileDigest(artifact, isFile, size)) {
+    if (!DigestUtils.useFileDigest(isFile, size)) {
       // In this case, we need to store the mtime because the action cache uses mtime to determine
       // if this artifact has changed. This is currently true for empty files and directories. We
       // do not optimize for this code path (by storing the mtime in a FileValue) because we do not
@@ -112,6 +113,11 @@ public class FileArtifactValue extends ArtifactValue {
     return digest;
   }
 
+  /** @return true if this is a file or a symlink to an existing file */
+  boolean isFile() {
+    return digest != null;
+  }
+
   /** Gets the size of the file. Directories have size 0. */
   long getSize() {
     return size;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java
index b375facd60..f41272c1c9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java
@@ -96,6 +96,12 @@ class PerActionFileCache implements ActionInputFileCache {
   }
 
   @Override
+  public boolean isFile(Artifact input) {
+    // getInputArtifactValue always returns a value when supplied with an Artifact.
+    return getInputFileArtifactValue(input).isFile();
+  }
+
+  @Override
   public boolean contentsAvailableLocally(ByteString digest) {
     return reverseMap.containsKey(digest);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index 9b986c17c5..ed01fa979a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -1052,7 +1052,7 @@ public final class SkyframeActionExecutor {
     private final ActionInputFileCache perActionCache;
     private final ActionInputFileCache perBuildFileCache;
 
-    private DelegatingPairFileCache(ActionInputFileCache mainCache,
+    private DelegatingPairFileCache(PerActionFileCache mainCache,
         ActionInputFileCache perBuildFileCache) {
       this.perActionCache = mainCache;
       this.perBuildFileCache = perBuildFileCache;
@@ -1065,6 +1065,12 @@ public final class SkyframeActionExecutor {
     }
 
     @Override
+    public boolean isFile(Artifact input) {
+      // PerActionCache must have a value for all artifacts.
+      return perActionCache.isFile(input);
+    }
+
+    @Override
     public long getSizeInBytes(ActionInput actionInput) throws IOException {
       long size = perActionCache.getSizeInBytes(actionInput);
       return size > -1 ? size : perBuildFileCache.getSizeInBytes(actionInput);
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java
index 351a419b9b..053e1edba9 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java
@@ -367,6 +367,11 @@ public class ResourceManagerTest {
     }
 
     @Override
+    public RunfilesSupplier getRunfilesSupplier() {
+      throw new IllegalStateException();
+    }
+
+    @Override
     public ImmutableSet<Artifact> getOutputs() {
       throw new IllegalStateException();
     }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesSupplierImplTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesSupplierImplTest.java
new file mode 100644
index 0000000000..14899b4ebb
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesSupplierImplTest.java
@@ -0,0 +1,80 @@
+// Copyright 2015 Google Inc. 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.analysis;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Root;
+import com.google.devtools.build.lib.testutil.Scratch;
+import com.google.devtools.build.lib.vfs.PathFragment;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+import java.io.IOException;
+import java.util.List;
+
+/** Tests for RunfilesSupplierImpl */
+@RunWith(JUnit4.class)
+public class RunfilesSupplierImplTest {
+
+  private Root rootDir;
+
+  @Before
+  public void setup() throws IOException {
+    Scratch scratch = new Scratch();
+    rootDir = Root.asDerivedRoot(scratch.dir("/fake/root/dont/matter"));
+  }
+
+  @Test
+  public void testGetArtifactsWithSingleMapping() {
+    List<Artifact> artifacts = mkArtifacts(rootDir, "thing1", "thing2");
+
+    RunfilesSupplierImpl underTest = new RunfilesSupplierImpl(
+        ImmutableMap.of(new PathFragment("notimportant"), mkRunfiles(artifacts)));
+
+    assertThat(underTest.getArtifacts()).containsExactlyElementsIn(artifacts);
+  }
+
+  @Test
+  public void testGetArtifactsWithMultipleMappings() {
+    List<Artifact> artifacts1 = mkArtifacts(rootDir, "thing_1", "thing_2", "duplicated");
+    List<Artifact> artifacts2 = mkArtifacts(rootDir, "thing_3", "thing_4", "duplicated");
+
+    RunfilesSupplierImpl underTest = new RunfilesSupplierImpl(ImmutableMap.of(
+        new PathFragment("notimportant"), mkRunfiles(artifacts1),
+        new PathFragment("stillnotimportant"), mkRunfiles(artifacts2)));
+
+    assertThat(underTest.getArtifacts()).containsExactlyElementsIn(
+        mkArtifacts(rootDir, "thing_1", "thing_2", "thing_3", "thing_4", "duplicated"));
+  }
+
+  private static Runfiles mkRunfiles(List<Artifact> artifacts) {
+    return new Runfiles.Builder().addArtifacts(artifacts).build();
+  }
+
+  private static List<Artifact> mkArtifacts(Root rootDir, String... paths) {
+    ImmutableList.Builder<Artifact> builder = ImmutableList.builder();
+    for (String path : paths) {
+      builder.add(new Artifact(new PathFragment(path), rootDir));
+    }
+    return builder.build();
+  }
+}