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

2 files changed, 40 insertions, 34 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;
     }