Become more restrictive around Runfiles#manifestExpander

Previously the contract was pretty liberal and could allow addition of
extra Artifacts. Create a more restrictive interface allowing the bare
minimum- adding empty files to the tree. This makes the contract of
Runfiles#getAllArtifacts() more sound, since arbitrary artifacts can't
be added, and it makes it easier to figure out what implementations of
manifest expansion exist out there.

--
MOS_MIGRATED_REVID=91233821

5 files changed, 55 insertions, 72 deletions

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 e7c07277e3..f365f3cb86 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
@@ -17,7 +17,7 @@ package com.google.devtools.build.lib.analysis;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.Constants;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -42,6 +42,7 @@ import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Set;
 
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
@@ -65,12 +66,11 @@ public final class Runfiles {
         }
       };
 
-  private static final Function<Map<PathFragment, Artifact>, Map<PathFragment, Artifact>>
-      DUMMY_SYMLINK_EXPANDER =
-      new Function<Map<PathFragment, Artifact>, Map<PathFragment, Artifact>>() {
+  private static final EmptyFilesSupplier DUMMY_EMPTY_FILES_SUPPLIER =
+      new EmptyFilesSupplier() {
         @Override
-        public Map<PathFragment, Artifact> apply(Map<PathFragment, Artifact> input) {
-          return ImmutableMap.of();
+        public Iterable<PathFragment> getExtraPaths(Set<PathFragment> manifestPaths) {
+          return ImmutableList.of();
         }
       };
 
@@ -152,10 +152,16 @@ public final class Runfiles {
   private final NestedSet<SymlinkEntry> rootSymlinks;
 
   /**
-   * A function to generate extra manifest entries.
+   * Interface used for adding empty files to the runfiles at the last minute. Mainly to support
+   * python-related rules adding __init__.py files.
    */
-  private final Function<Map<PathFragment, Artifact>, Map<PathFragment, Artifact>>
-      manifestExpander;
+  public interface EmptyFilesSupplier {
+    /** Calculate additional empty files to add based on the existing manifest paths. */
+    Iterable<PathFragment> getExtraPaths(Set<PathFragment> manifestPaths);
+  }
+
+  /** Generates extra (empty file) inputs. */
+  private final EmptyFilesSupplier emptyFilesSupplier;
 
   /**
    * Defines a set of artifacts that may or may not be included in the runfiles directory and
@@ -212,13 +218,13 @@ public final class Runfiles {
       NestedSet<SymlinkEntry> symlinks,
       NestedSet<SymlinkEntry> rootSymlinks,
       NestedSet<PruningManifest> pruningManifests,
-      Function<Map<PathFragment, Artifact>, Map<PathFragment, Artifact>> expander) {
+      EmptyFilesSupplier emptyFilesSupplier) {
     this.suffix = suffix == null ? Constants.DEFAULT_RUNFILES_PREFIX : suffix;
     this.unconditionalArtifacts = Preconditions.checkNotNull(artifacts);
     this.symlinks = Preconditions.checkNotNull(symlinks);
     this.rootSymlinks = Preconditions.checkNotNull(rootSymlinks);
     this.pruningManifests = Preconditions.checkNotNull(pruningManifests);
-    this.manifestExpander = Preconditions.checkNotNull(expander);
+    this.emptyFilesSupplier = Preconditions.checkNotNull(emptyFilesSupplier);
   }
 
   /**
@@ -360,7 +366,11 @@ public final class Runfiles {
     }
 
     manifest = filterListForObscuringSymlinks(eventHandler, location, manifest);
-    manifest.putAll(manifestExpander.apply(manifest));
+
+    // TODO(bazel-team): Create /dev/null-like Artifact to avoid nulls?
+    for (PathFragment extraPath : emptyFilesSupplier.getExtraPaths(manifest.keySet())) {
+      manifest.put(extraPath, null);
+    }
 
     PathFragment path = new PathFragment(suffix);
     Map<PathFragment, Artifact> result = new HashMap<>();
@@ -407,10 +417,10 @@ public final class Runfiles {
   }
 
   /**
-   * Returns the symlinks expander specified for this runfiles tree.
+   * Returns the manifest expander specified for this runfiles tree.
    */
-  private Function<Map<PathFragment, Artifact>, Map<PathFragment, Artifact>> getSymlinkExpander() {
-    return manifestExpander;
+  private EmptyFilesSupplier getEmptyFilesProvider() {
+    return emptyFilesSupplier;
   }
 
   /**
@@ -468,8 +478,7 @@ public final class Runfiles {
         NestedSetBuilder.stableOrder();
     private NestedSetBuilder<PruningManifest> pruningManifestsBuilder =
         NestedSetBuilder.stableOrder();
-    private Function<Map<PathFragment, Artifact>, Map<PathFragment, Artifact>>
-        manifestExpander = DUMMY_SYMLINK_EXPANDER;
+    private EmptyFilesSupplier emptyFilesSupplier = DUMMY_EMPTY_FILES_SUPPLIER;
 
     /**
      * Builds a new Runfiles object.
@@ -477,7 +486,7 @@ public final class Runfiles {
     public Runfiles build() {
       return new Runfiles(suffix, artifactsBuilder.build(), symlinksBuilder.build(),
           rootSymlinksBuilder.build(), pruningManifestsBuilder.build(),
-          manifestExpander);
+          emptyFilesSupplier);
     }
 
     /**
@@ -583,11 +592,11 @@ public final class Runfiles {
     }
 
     /**
-     * Specify a function that can create additional manifest entries based on the input entries.
+     * Specify a function that can create additional manifest entries based on the input entries,
+     * see {@link EmptyFilesSupplier} for more details.
      */
-    public Builder setManifestExpander(
-        Function<Map<PathFragment, Artifact>, Map<PathFragment, Artifact>> expander) {
-      manifestExpander = Preconditions.checkNotNull(expander);
+    public Builder setEmptyFilesSupplier(EmptyFilesSupplier supplier) {
+      emptyFilesSupplier = Preconditions.checkNotNull(supplier);
       return this;
     }
 
@@ -748,13 +757,12 @@ public final class Runfiles {
       if (includePruningManifests) {
         pruningManifestsBuilder.addTransitive(runfiles.getPruningManifests());
       }
-      if (manifestExpander == DUMMY_SYMLINK_EXPANDER) {
-        manifestExpander = runfiles.getSymlinkExpander();
+      if (emptyFilesSupplier == DUMMY_EMPTY_FILES_SUPPLIER) {
+        emptyFilesSupplier = runfiles.getEmptyFilesProvider();
       } else {
-        Function<Map<PathFragment, Artifact>, Map<PathFragment, Artifact>> otherExpander =
-            runfiles.getSymlinkExpander();
-        Preconditions.checkState((otherExpander == DUMMY_SYMLINK_EXPANDER)
-          || manifestExpander.equals(otherExpander));
+        EmptyFilesSupplier otherSupplier = runfiles.getEmptyFilesProvider();
+        Preconditions.checkState((otherSupplier == DUMMY_EMPTY_FILES_SUPPLIER)
+          || emptyFilesSupplier.equals(otherSupplier));
       }
       return this;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java b/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java
index 7929dbc0b2..a46e9f3743 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.analysis;
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -349,12 +348,11 @@ public class SourceManifestAction extends AbstractFileWriteAction {
     }
 
     /**
-     * Set an expander function for the symlinks.
+     * Set the empty files supplier for the manifest, see {@link Runfiles.EmptyFilesSupplier}
+     * for more details.
      */
-    @VisibleForTesting
-    Builder setSymlinksExpander(
-        Function<Map<PathFragment, Artifact>, Map<PathFragment, Artifact>> expander) {
-      runfilesBuilder.setManifestExpander(expander);
+    public Builder setEmptyFilesSupplier(Runfiles.EmptyFilesSupplier supplier) {
+      runfilesBuilder.setEmptyFilesSupplier(supplier);
       return this;
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
index ecb8d48b80..e91f7f256c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
@@ -100,7 +100,7 @@ public abstract class PyBinary implements RuleConfiguredTargetFactory {
     }
     builder.addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES);
     builder.add(ruleContext, PythonRunfilesProvider.TO_RUNFILES);
-    builder.setManifestExpander(PythonUtils.GET_INIT_PY_FILES);
+    builder.setEmptyFilesSupplier(PythonUtils.GET_INIT_PY_FILES);
     return builder;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java
index 89b4fc8c19..5f57c1ff6c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyLibrary.java
@@ -66,7 +66,7 @@ public abstract class PyLibrary implements RuleConfiguredTargetFactory {
     } else {
       runfilesBuilder.addTransitiveArtifacts(filesToBuild);
     }
-    runfilesBuilder.setManifestExpander(PythonUtils.GET_INIT_PY_FILES);
+    runfilesBuilder.setEmptyFilesSupplier(PythonUtils.GET_INIT_PY_FILES);
     runfilesBuilder.add(ruleContext, PythonRunfilesProvider.TO_RUNFILES);
     runfilesBuilder.addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES);
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonUtils.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonUtils.java
index 0c921c8423..c663b263db 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonUtils.java
@@ -13,13 +13,13 @@
 // limitations under the License.
 package com.google.devtools.build.lib.rules.python;
 
-import com.google.common.base.Function;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget;
 import com.google.devtools.build.lib.analysis.RuleContext;
+import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -39,12 +39,11 @@ public final class PythonUtils {
 
   private static final FileType REQUIRES_INIT_PY = FileType.of(".py", ".so", ".pyc");
 
-  public static final Function<Map<PathFragment, Artifact>, Map<PathFragment, Artifact>>
-      GET_INIT_PY_FILES = new Function<Map<PathFragment, Artifact>, Map<PathFragment, Artifact>>()
-      {
+  public static final Runfiles.EmptyFilesSupplier GET_INIT_PY_FILES =
+      new Runfiles.EmptyFilesSupplier() {
     @Override
-    public Map<PathFragment, Artifact> apply(Map<PathFragment, Artifact> input) {
-      return getInitDotPyFiles(input);
+    public Iterable<PathFragment> getExtraPaths(Set<PathFragment> manifestPaths) {
+      return getInitPyFiles(manifestPaths);
     }
   };
 
@@ -62,15 +61,14 @@ public final class PythonUtils {
 
     for (PathFragment source : manifestFiles) {
       // If we have a python or .so file at this level...
-      if (!REQUIRES_INIT_PY.matches(source)) {
-        continue;
-      }
-      // ...then record that we need an __init__.py in this directory...
-      while (source.segmentCount() > 1) {
-        source = source.getParentDirectory();
-        PathFragment initpy = source.getRelative(INIT_PY);
-        if (!manifestFiles.contains(initpy)) {
-          result.add(initpy);
+      if (REQUIRES_INIT_PY.matches(source)) {
+        // ...then record that we need an __init__.py in this directory...
+        while (source.segmentCount() > 1) {
+          source = source.getParentDirectory();
+          PathFragment initpy = source.getRelative(INIT_PY);
+          if (!manifestFiles.contains(initpy)) {
+            result.add(initpy);
+          }
         }
       }
     }
@@ -79,27 +77,6 @@ public final class PythonUtils {
   }
 
   /**
-   * Creates a new map that contains all the <code>__init__.py</code> files that are necessary for
-   * Python to find the <code>.py</code> and <code>.so</code> files in it.
-   *
-   * @param inputManifest The input mapping of source files to absolute paths on disk.
-   * @return The revised mapping. Contains <code>null</code> values for the added
-   * <code>__init.py__</code> files.
-   */
-  private static Map<PathFragment, Artifact> getInitDotPyFiles(
-      Map<PathFragment, Artifact> inputManifest) {
-    Map<PathFragment, Artifact> newManifest = new HashMap<>();
-
-    for (PathFragment initpy : getInitPyFiles(inputManifest.keySet())) {
-      if (newManifest.get(initpy) == null) {
-        newManifest.put(initpy, null);
-      }
-    }
-
-    return newManifest;
-  }
-
-  /**
    * Get the artifact generated by the 2to3 action. The artifact is in a python3
    * subdirectory to avoid conflicts (eg. when the input file is generated).
    */