Clean up LocalResourceContainer.Builder

LocalResourceContainer.Builder is referenced a bunch of times, but most of
those calls are identical. Replace them with a couple of factory methods. This
lets us make the builder private.

Additionally, LocalResourceContainer called attributeError() on the 'resources'
attribute, even though 'resource_files' is now used most of the time instead.
Now, the error is always given for the correct attribute.

RELNOTES: none
PiperOrigin-RevId: 174038035

4 files changed, 99 insertions, 114 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java b/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java
index 6eebc7f41c..09b0af66d6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java
@@ -99,9 +99,8 @@ public class AarImport implements RuleConfiguredTargetFactory {
     ResourceApk resourceApk =
         androidManifest.packAarWithDataAndResources(
             ruleContext,
-            new LocalResourceContainer.Builder(ruleContext)
-                .withResources(ImmutableList.of(resourcesProvider))
-                .build(),
+            LocalResourceContainer.forResourceFileProvider(
+                ruleContext, resourcesProvider, "resources"),
             ResourceDependencies.fromRuleDeps(ruleContext, JavaCommon.isNeverLink(ruleContext)),
             ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_R_TXT),
             ruleContext.getImplicitOutputArtifact(AndroidRuleClasses.ANDROID_LOCAL_SYMBOLS),
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java b/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java
index 9afd00e6bc..e7b338e8bb 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/ApplicationManifest.java
@@ -23,7 +23,6 @@ import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
@@ -350,16 +349,12 @@ public final class ApplicationManifest {
     resourceDeps = resourceDeps.filter(ruleContext, resourceFilter);
 
     LocalResourceContainer data =
-        new LocalResourceContainer.Builder(ruleContext)
-            .withAssets(
+        LocalResourceContainer.forAssetsAndResources(
+                ruleContext,
+                "assets",
                 AndroidCommon.getAssetDir(ruleContext),
-                ruleContext.getPrerequisitesIf(
-                    // TODO(bazel-team): Remove the ResourceType construct.
-                    ResourceType.ASSETS.getAttribute(), Mode.TARGET, FileProvider.class))
-            .withResources(
-                ruleContext.getPrerequisites(
-                    "local_resource_files", Mode.TARGET, FileProvider.class))
-            .build().filter(ruleContext, resourceFilter);
+                "local_resource_files")
+            .filter(ruleContext, resourceFilter);
 
     // Now that the LocalResourceContainer has been filtered, we can build a filtered resource
     // container from it.
@@ -524,15 +519,8 @@ public final class ApplicationManifest {
       Artifact proguardCfg)
       throws InterruptedException, RuleErrorException {
     LocalResourceContainer data =
-        new LocalResourceContainer.Builder(ruleContext)
-            .withAssets(
-                AndroidCommon.getAssetDir(ruleContext),
-                ruleContext.getPrerequisitesIf(
-                    // TODO(bazel-team): Remove the ResourceType construct.
-                    ResourceType.ASSETS.getAttribute(), Mode.TARGET, FileProvider.class))
-            .withResources(
-                ruleContext.getPrerequisites("resource_files", Mode.TARGET, FileProvider.class))
-            .build();
+        LocalResourceContainer.forAssetsAndResources(
+            ruleContext, "assets", AndroidCommon.getAssetDir(ruleContext), "resource_files");
 
     // Filter the resources during analysis to prevent processing of dependencies on unwanted
     // resources during execution.
@@ -610,15 +598,8 @@ public final class ApplicationManifest {
       @Nullable Artifact featureAfter)
       throws InterruptedException, RuleErrorException {
     LocalResourceContainer data =
-        new LocalResourceContainer.Builder(ruleContext)
-            .withAssets(
-                AndroidCommon.getAssetDir(ruleContext),
-                ruleContext.getPrerequisitesIf(
-                    // TODO(bazel-team): Remove the ResourceType construct.
-                    ResourceType.ASSETS.getAttribute(), Mode.TARGET, FileProvider.class))
-            .withResources(
-                ruleContext.getPrerequisites("resource_files", Mode.TARGET, FileProvider.class))
-            .build()
+        LocalResourceContainer.forAssetsAndResources(
+                ruleContext, "assets", AndroidCommon.getAssetDir(ruleContext), "resource_files")
             .filter(ruleContext, resourceFilter);
 
     resourceDeps = resourceDeps.filter(ruleContext, resourceFilter);
@@ -697,15 +678,8 @@ public final class ApplicationManifest {
     // resources during execution.
     ResourceFilter resourceFilter = ResourceFilter.fromRuleContext(ruleContext);
     LocalResourceContainer data =
-        new LocalResourceContainer.Builder(ruleContext)
-            .withAssets(
-                AndroidCommon.getAssetDir(ruleContext),
-                ruleContext.getPrerequisitesIf(
-                    // TODO(bazel-team): Remove the ResourceType construct.
-                    ResourceType.ASSETS.getAttribute(), Mode.TARGET, FileProvider.class))
-            .withResources(
-                ruleContext.getPrerequisites("resource_files", Mode.TARGET, FileProvider.class))
-            .build()
+        LocalResourceContainer.forAssetsAndResources(
+                ruleContext, "assets", AndroidCommon.getAssetDir(ruleContext), "resource_files")
             .filter(ruleContext, resourceFilter);
     resourceDeps = resourceDeps.filter(ruleContext, resourceFilter);
     ResourceContainer.Builder builder =
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/LocalResourceContainer.java b/src/main/java/com/google/devtools/build/lib/rules/android/LocalResourceContainer.java
index 7f89fd406b..53dd15100e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/LocalResourceContainer.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/LocalResourceContainer.java
@@ -131,77 +131,82 @@ public final class LocalResourceContainer {
     }
   }
 
-  public static class Builder {
-
-    private final RuleContext ruleContext;
-    private final Set<Artifact> assets = new LinkedHashSet<>();
-    private final Set<Artifact> resources = new LinkedHashSet<>();
-    private final Set<PathFragment> resourceRoots = new LinkedHashSet<>();
-    private final Set<PathFragment> assetRoots = new LinkedHashSet<>();
-
-    public Builder(RuleContext ruleContext) {
-      this.ruleContext = ruleContext;
-    }
-
-    /**
-     * Retrieves the asset {@link Artifact} and asset root {@link PathFragment}.
-     * @param assetsDir The common directory for the assets, used to get the directory roots and
-     *   verify the artifacts are located beneath the assetsDir
-     * @param targets {@link FileProvider}s for a given set of assets.
-     * @return The Builder.
-     */
-    public LocalResourceContainer.Builder withAssets(
-        PathFragment assetsDir, Iterable<? extends TransitiveInfoCollection> targets)
-        throws RuleErrorException {
-      for (TransitiveInfoCollection target : targets) {
-        for (Artifact file : target.getProvider(FileProvider.class).getFilesToBuild()) {
-          PathFragment packageFragment = file.getArtifactOwner().getLabel()
-              .getPackageIdentifier().getSourceRoot();
-          PathFragment packageRelativePath =
-              file.getRootRelativePath().relativeTo(packageFragment);
-          if (packageRelativePath.startsWith(assetsDir)) {
-            PathFragment relativePath = packageRelativePath.relativeTo(assetsDir);
-            assetRoots.add(trimTail(file.getExecPath(), relativePath));
-          } else {
-            ruleContext.attributeError(ResourceType.ASSETS.getAttribute(), String.format(
-                "'%s' (generated by '%s') is not beneath '%s'",
-                file.getRootRelativePath(), target.getLabel(), assetsDir));
-            throw new RuleErrorException();
-          }
-          assets.add(file);
+  /**
+   * Creates a {@link LocalResourceContainer} containing this target's assets and resources.
+   *
+   * @param ruleContext the current context
+   * @param assetsAttr the attribute used to refer to assets in this target
+   * @param assetsDir the path to the assets directory
+   * @param resourcesAttr the attribute used to refer to resource files in this target
+   */
+  public static LocalResourceContainer forAssetsAndResources(
+      RuleContext ruleContext, String assetsAttr, PathFragment assetsDir, String resourcesAttr)
+      throws RuleErrorException {
+    ImmutableList.Builder<Artifact> assets = ImmutableList.builder();
+    ImmutableList.Builder<PathFragment> assetRoots = ImmutableList.builder();
+
+    for (TransitiveInfoCollection target :
+        ruleContext.getPrerequisitesIf(assetsAttr, Mode.TARGET, FileProvider.class)) {
+      for (Artifact file : target.getProvider(FileProvider.class).getFilesToBuild()) {
+        PathFragment packageFragment =
+            file.getArtifactOwner().getLabel().getPackageIdentifier().getSourceRoot();
+        PathFragment packageRelativePath = file.getRootRelativePath().relativeTo(packageFragment);
+        if (packageRelativePath.startsWith(assetsDir)) {
+          PathFragment relativePath = packageRelativePath.relativeTo(assetsDir);
+          assetRoots.add(trimTail(file.getExecPath(), relativePath));
+        } else {
+          ruleContext.attributeError(
+              ResourceType.ASSETS.getAttribute(),
+              String.format(
+                  "'%s' (generated by '%s') is not beneath '%s'",
+                  file.getRootRelativePath(), target.getLabel(), assetsDir));
+          throw new RuleErrorException();
         }
+        assets.add(file);
       }
-      return this;
     }
 
-    /**
-     * Retrieves the resource {@link Artifact} and resource root {@link PathFragment}.
-     * @param targets {@link FileProvider}s for a given set of resource.
-     * @return The Builder.
-     */
-    public LocalResourceContainer.Builder withResources(Iterable<FileProvider> targets)
-        throws RuleErrorException {
-      PathFragment lastResourceDir = null;
-      Artifact lastFile = null;
-      for (FileProvider target : targets) {
-        for (Artifact file : target.getFilesToBuild()) {
-          PathFragment resourceDir =
-              addResourceDir(file, lastFile, lastResourceDir, resourceRoots, ruleContext);
-          resources.add(file);
-          lastFile = file;
-          lastResourceDir = resourceDir;
-        }
-      }
-      return this;
-    }
+    ImmutableList<Artifact> resources =
+        getResources(ruleContext.getPrerequisites(resourcesAttr, Mode.TARGET, FileProvider.class));
+
+    return new LocalResourceContainer(
+        resources,
+        getResourceRoots(ruleContext, resources, resourcesAttr),
+        assets.build(),
+        assetRoots.build());
+  }
+
+  /**
+   * Creates a {@link LocalResourceContainer} containing the resources included in a {@link
+   * FileProvider}.
+   *
+   * <p>In general, {@link #forAssetsAndResources(RuleContext, String, String, String)} should be
+   * used instead. No assets or transitive resources will be included in the container produced by
+   * this method.
+   *
+   * @param ruleContext the current context
+   * @param resourceFileProvider the provider containing resources
+   * @param resourcesAttr the attribute used to refer to resource files in this target.
+   */
+  public static LocalResourceContainer forResourceFileProvider(
+      RuleContext ruleContext, FileProvider resourceFileProvider, String resourcesAttr)
+      throws RuleErrorException {
+    ImmutableList<Artifact> resources = getResources(ImmutableList.of(resourceFileProvider));
+
+    return new LocalResourceContainer(
+        resources,
+        getResourceRoots(ruleContext, resources, resourcesAttr),
+        ImmutableList.of(),
+        ImmutableList.of());
+  }
 
-    public LocalResourceContainer build() {
-      return new LocalResourceContainer(
-          ImmutableList.copyOf(resources),
-          ImmutableList.copyOf(resourceRoots),
-          ImmutableList.copyOf(assets),
-          ImmutableList.copyOf(assetRoots));
+  private static ImmutableList<Artifact> getResources(Iterable<FileProvider> targets) {
+    ImmutableList.Builder<Artifact> builder = ImmutableList.builder();
+    for (FileProvider target : targets) {
+      builder.addAll(target.getFilesToBuild());
     }
+
+    return builder.build();
   }
 
   /**
@@ -213,13 +218,15 @@ public final class LocalResourceContainer {
    */
   @VisibleForTesting
   static ImmutableList<PathFragment> getResourceRoots(
-      RuleErrorConsumer ruleErrorConsumer, Iterable<Artifact> files) throws RuleErrorException {
+      RuleErrorConsumer ruleErrorConsumer, Iterable<Artifact> files, String resourcesAttr)
+      throws RuleErrorException {
     Artifact lastFile = null;
     PathFragment lastResourceDir = null;
     Set<PathFragment> resourceRoots = new LinkedHashSet<>();
     for (Artifact file : files) {
       PathFragment resourceDir =
-          addResourceDir(file, lastFile, lastResourceDir, resourceRoots, ruleErrorConsumer);
+          addResourceDir(
+              file, lastFile, lastResourceDir, resourceRoots, resourcesAttr, ruleErrorConsumer);
       lastFile = file;
       lastResourceDir = resourceDir;
     }
@@ -237,6 +244,9 @@ public final class LocalResourceContainer {
    * @param lastResourceDir the resource directory of the last file, as returned by the most recent
    *     call to this method, or null if this is the first call.
    * @param resourceRoots the collection to add resources to
+   * @param resourcesAttr the attribute used to refer to resources. While we're moving towards
+   *     "resource_files" everywhere, there are still uses of other attributes for different kinds
+   *     of rules.
    * @param ruleErrorConsumer for reporting errors
    * @return the resource root of {@code file}.
    * @throws RuleErrorException if the current resource has no resource directory or if it is
@@ -248,18 +258,20 @@ public final class LocalResourceContainer {
       @Nullable Artifact lastFile,
       @Nullable PathFragment lastResourceDir,
       Set<PathFragment> resourceRoots,
-      RuleErrorConsumer ruleErrorConsumer) throws RuleErrorException {
+      String resourcesAttr,
+      RuleErrorConsumer ruleErrorConsumer)
+      throws RuleErrorException {
     PathFragment resourceDir = findResourceDir(file);
     if (resourceDir == null) {
       ruleErrorConsumer.attributeError(
-          ResourceType.RESOURCES.getAttribute(),
+          resourcesAttr,
           String.format(INCORRECT_RESOURCE_LAYOUT_MESSAGE, file.getRootRelativePath()));
       throw new RuleErrorException();
     }
 
     if (lastResourceDir != null && !resourceDir.equals(lastResourceDir)) {
       ruleErrorConsumer.attributeError(
-          ResourceType.RESOURCES.getAttribute(),
+          resourcesAttr,
           String.format(
               "'%s' (generated by '%s') is not in the same directory '%s' (derived from %s)."
                   + " All resources must share a common directory.",
@@ -380,7 +392,7 @@ public final class LocalResourceContainer {
 
     return new LocalResourceContainer(
         filteredResources,
-        getResourceRoots(ruleErrorConsumer, filteredResources),
+        getResourceRoots(ruleErrorConsumer, filteredResources, "resource_files"),
         assets,
         assetRoots);
   }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/LocalResourceContainerTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/LocalResourceContainerTest.java
index 6f397bf5fb..302ff4afdc 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/LocalResourceContainerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/LocalResourceContainerTest.java
@@ -51,7 +51,7 @@ public class LocalResourceContainerTest extends ResourceTestBase {
     }
 
     errorConsumer.assertAttributeError(
-        "resources", "is not in the expected resource directory structure");
+        "resource_files", "is not in the expected resource directory structure");
   }
 
   @Test
@@ -64,7 +64,7 @@ public class LocalResourceContainerTest extends ResourceTestBase {
     }
 
     errorConsumer.assertAttributeError(
-        "resources", "All resources must share a common directory");
+        "resource_files", "All resources must share a common directory");
   }
 
   @Test
@@ -86,7 +86,7 @@ public class LocalResourceContainerTest extends ResourceTestBase {
 
   private ImmutableList<PathFragment> getResourceRoots(ImmutableList<Artifact> artifacts)
       throws Exception {
-    return LocalResourceContainer.getResourceRoots(errorConsumer, artifacts);
+    return LocalResourceContainer.getResourceRoots(errorConsumer, artifacts, "resource_files");
   }
 
   @Test