Automated g4 rollback of commit c78c947e6a8cbb323304f872a3dcabb989a3d76b.

*** Reason for rollback ***

Breaks android targets in the nightly - see []

*** Original change description ***

Do not retain transitive data in AndroidLocalTestBase.

The argument strings and artifacts were both transitive and flattened, causing O(N^2) memory consumption.

PiperOrigin-RevId: 156745610

5 files changed, 76 insertions, 88 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibrary.java
index a6d7f39718..eae1f2dfb4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibrary.java
@@ -67,9 +67,7 @@ public abstract class AndroidLibrary implements RuleConfiguredTargetFactory {
       return null;
     }
     checkResourceInlining(ruleContext);
-    NestedSetBuilder<Aar> transitiveAars = NestedSetBuilder.naiveLinkOrder();
-    NestedSetBuilder<Artifact> transitiveAarArtifacts = NestedSetBuilder.stableOrder();
-    collectTransitiveAars(ruleContext, transitiveAars, transitiveAarArtifacts);
+    NestedSetBuilder<Aar> transitiveAars = collectTransitiveAars(ruleContext);
 
     NestedSetBuilder<Artifact> proguardConfigsbuilder = NestedSetBuilder.stableOrder();
     proguardConfigsbuilder.addTransitive(new ProguardLibrary(ruleContext).collectProguardSpecs());
@@ -150,12 +148,12 @@ public abstract class AndroidLibrary implements RuleConfiguredTargetFactory {
       // applicationManifest has already been checked for nullness above in this method
       ApplicationManifest applicationManifest = androidSemantics.getManifestForRule(ruleContext);
       aar = Aar.create(aarOut, applicationManifest.getManifest());
-      addAarToProvider(aar, transitiveAars, transitiveAarArtifacts);
+      transitiveAars.add(aar);
     } else if (AndroidCommon.getAndroidResources(ruleContext) != null) {
       primaryResources = Iterables.getOnlyElement(
           AndroidCommon.getAndroidResources(ruleContext).getDirectAndroidResources());
       aar = Aar.create(aarOut, primaryResources.getManifest());
-      addAarToProvider(aar, transitiveAars, transitiveAarArtifacts);
+      transitiveAars.add(aar);
     } else {
       // there are no local resources and resources attribute was not specified either
       aar = null;
@@ -236,26 +234,12 @@ public abstract class AndroidLibrary implements RuleConfiguredTargetFactory {
     if (!JavaCommon.isNeverLink(ruleContext)) {
       builder.add(
           AndroidLibraryAarProvider.class,
-          AndroidLibraryAarProvider.create(
-              aar, transitiveAars.build(), transitiveAarArtifacts.build()));
+          AndroidLibraryAarProvider.create(aar, transitiveAars.build()));
     }
 
     return builder.build();
   }
 
-  private void addAarToProvider(
-      Aar aar,
-      NestedSetBuilder<Aar> transitiveAars,
-      NestedSetBuilder<Artifact> transitiveAarArtifacts) {
-    transitiveAars.add(aar);
-    if (aar.getAar() != null) {
-      transitiveAarArtifacts.add(aar.getAar());
-    }
-    if (aar.getManifest() != null) {
-      transitiveAarArtifacts.add(aar.getManifest());
-    }
-  }
-
   private void checkResourceInlining(RuleContext ruleContext) {
     AndroidResourcesProvider resources = AndroidCommon.getAndroidResources(ruleContext);
     if (resources == null) {
@@ -272,15 +256,13 @@ public abstract class AndroidLibrary implements RuleConfiguredTargetFactory {
     }
   }
 
-  private void collectTransitiveAars(
-      RuleContext ruleContext,
-      NestedSetBuilder<Aar> transitiveAars,
-      NestedSetBuilder<Artifact> transitiveAarArtifacts) {
+  private NestedSetBuilder<Aar> collectTransitiveAars(RuleContext ruleContext) {
+    NestedSetBuilder<Aar> builder = NestedSetBuilder.naiveLinkOrder();
     for (AndroidLibraryAarProvider library : AndroidCommon.getTransitivePrerequisites(
         ruleContext, Mode.TARGET, AndroidLibraryAarProvider.class)) {
-      transitiveAars.addTransitive(library.getTransitiveAars());
-      transitiveAarArtifacts.addTransitive(library.getTransitiveAarArtifacts());
+      builder.addTransitive(library.getTransitiveAars());
     }
+    return builder;
   }
 
   private NestedSetBuilder<Artifact> collectTransitiveResourceJars(RuleContext ruleContext) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibraryAarProvider.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibraryAarProvider.java
index d4a8a8afdd..ff9236e9ce 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibraryAarProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLibraryAarProvider.java
@@ -28,19 +28,14 @@ import javax.annotation.Nullable;
 @Immutable
 public abstract class AndroidLibraryAarProvider implements TransitiveInfoProvider {
 
-  public static AndroidLibraryAarProvider create(
-      @Nullable Aar aar,
-      NestedSet<Aar> transitiveAars,
-      NestedSet<Artifact> transitiveAarArtifacts) {
-    return new AutoValue_AndroidLibraryAarProvider(aar, transitiveAars, transitiveAarArtifacts);
+  public static AndroidLibraryAarProvider create(@Nullable Aar aar, NestedSet<Aar> transitiveAars) {
+    return new AutoValue_AndroidLibraryAarProvider(aar, transitiveAars);
   }
 
   @Nullable public abstract Aar getAar();
 
   public abstract NestedSet<Aar> getTransitiveAars();
 
-  public abstract NestedSet<Artifact> getTransitiveAarArtifacts();
-
   /** The .aar file and associated AndroidManifest.xml contributed by a single target. */
   @AutoValue
   @Immutable
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java
index ddd4e84c2c..1ef476f87f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java
@@ -16,7 +16,7 @@ package com.google.devtools.build.lib.rules.android;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
 import static com.google.devtools.build.lib.rules.java.DeployArchiveBuilder.Compression.COMPRESSED;
 
-import com.google.common.base.Function;
+import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
@@ -31,7 +31,6 @@ import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.RunfilesSupport;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
-import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory;
@@ -55,9 +54,12 @@ import com.google.devtools.build.lib.rules.java.JavaTargetAttributes;
 import com.google.devtools.build.lib.rules.java.SingleJarActionBuilder;
 import com.google.devtools.build.lib.rules.java.proto.GeneratedExtensionRegistryProvider;
 import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Set;
 
 /**
  * An base implementation for the "android_local_test" rule.
@@ -181,46 +183,13 @@ public abstract class AndroidLocalTestBase implements RuleConfiguredTargetFactor
                 ruleContext.getPrerequisites(
                     "deps", Mode.TARGET, AndroidLibraryAarProvider.class)));
 
-    NestedSetBuilder<Aar> transitiveAarsBuilder = NestedSetBuilder.naiveLinkOrder();
-    NestedSetBuilder<Aar> strictAarsBuilder = NestedSetBuilder.naiveLinkOrder();
-    NestedSetBuilder<Artifact> transitiveAarArtifactsBuilder = NestedSetBuilder.stableOrder();
-    for (AndroidLibraryAarProvider aarProvider : androidAarProviders) {
-      transitiveAarsBuilder.addTransitive(aarProvider.getTransitiveAars());
-      transitiveAarArtifactsBuilder.addTransitive(aarProvider.getTransitiveAarArtifacts());
-      if (aarProvider.getAar() != null) {
-        strictAarsBuilder.add(aarProvider.getAar());
-      }
-    }
-    NestedSet<Aar> transitiveAars = transitiveAarsBuilder.build();
-    NestedSet<Aar> strictAars = strictAarsBuilder.build();
-    NestedSet<Artifact> transitiveAarArtifacts = transitiveAarArtifactsBuilder.build();
-
     Runfiles defaultRunfiles =
-        collectDefaultRunfiles(ruleContext, javaCommon, filesToBuild, transitiveAarArtifacts);
-
-    CustomCommandLine cmdLineArgs =
-        CustomCommandLine.builder()
-            .addJoinValues(
-                "--android_libraries",
-                ",",
-                transitiveAars,
-                new Function<Aar, String>() {
-                  @Override
-                  public String apply(Aar aar) {
-                    return aarCmdLineArg(aar);
-                  }
-                })
-            .addJoinValues(
-                "--strict_libraries",
-                ",",
-                strictAars,
-                new Function<Aar, String>() {
-                  @Override
-                  public String apply(Aar aar) {
-                    return aarCmdLineArg(aar);
-                  }
-                })
-            .build();
+        collectDefaultRunfiles(ruleContext, javaCommon, filesToBuild, androidAarProviders);
+
+    ImmutableList<String> cmdLineArgs =
+        ImmutableList.of(
+            "--android_libraries=" + getTransitiveLibrariesArg(androidAarProviders),
+            "--strict_libraries=" + getStrictLibrariesArg(androidAarProviders));
 
     RunfilesSupport runfilesSupport =
         RunfilesSupport.withExecutable(ruleContext, defaultRunfiles, executable, cmdLineArgs);
@@ -298,12 +267,6 @@ public abstract class AndroidLocalTestBase implements RuleConfiguredTargetFactor
         .build();
   }
 
-  private static String aarCmdLineArg(Aar aar) {
-    return aar.getManifest().getRootRelativePathString()
-        + ":"
-        + aar.getAar().getRootRelativePathString();
-  }
-
   protected abstract JavaSemantics createJavaSemantics();
 
   protected abstract void addExtraProviders(
@@ -374,7 +337,7 @@ public abstract class AndroidLocalTestBase implements RuleConfiguredTargetFactor
       RuleContext ruleContext,
       JavaCommon javaCommon,
       NestedSet<Artifact> filesToBuild,
-      NestedSet<Artifact> transitiveAarArtifacts) {
+      Iterable<AndroidLibraryAarProvider> androidLibraryAarProviders) {
     Runfiles.Builder builder = new Runfiles.Builder(ruleContext.getWorkspaceName());
     builder.addTransitiveArtifacts(filesToBuild);
     builder.addArtifacts(javaCommon.getJavaCompilationArtifacts().getRuntimeJars());
@@ -395,7 +358,17 @@ public abstract class AndroidLocalTestBase implements RuleConfiguredTargetFactor
 
     builder.addTargets(depsForRunfiles, JavaRunfilesProvider.TO_RUNFILES);
     builder.addTargets(depsForRunfiles, RunfilesProvider.DEFAULT_RUNFILES);
-    builder.addTransitiveArtifacts(transitiveAarArtifacts);
+
+    for (AndroidLibraryAarProvider aarProvider : androidLibraryAarProviders) {
+      if (aarProvider.getAar() != null) {
+        builder.addArtifact(aarProvider.getAar().getAar());
+        builder.addArtifact(aarProvider.getAar().getManifest());
+      }
+      for (Aar aar : aarProvider.getTransitiveAars()) {
+        builder.addArtifact(aar.getAar());
+        builder.addArtifact(aar.getManifest());
+      }
+    }
 
     if (ruleContext.getConfiguration().isCodeCoverageEnabled()) {
       Artifact instrumentedJar = javaCommon.getJavaCompilationArtifacts().getInstrumentedJar();
@@ -437,4 +410,38 @@ public abstract class AndroidLocalTestBase implements RuleConfiguredTargetFactor
     }
     return testClass;
   }
+
+  protected static String getTransitiveLibrariesArg(
+      Iterable<AndroidLibraryAarProvider> aarProviders) {
+    Set<String> args = new LinkedHashSet<>();
+
+    for (AndroidLibraryAarProvider aarProvider : aarProviders) {
+      for (Aar aar : aarProvider.getTransitiveAars()) {
+        Preconditions.checkNotNull(aar.getAar());
+        Preconditions.checkNotNull(aar.getManifest());
+        args.add(
+            aar.getManifest().getRootRelativePathString()
+                + ":"
+                + aar.getAar().getRootRelativePathString());
+      }
+    }
+    return Joiner.on(",").join(args);
+  }
+
+  protected static String getStrictLibrariesArg(Iterable<AndroidLibraryAarProvider> aarProviders) {
+    Set<String> args = new LinkedHashSet<>();
+
+    for (AndroidLibraryAarProvider aarProvider : aarProviders) {
+      Aar aar = aarProvider.getAar();
+      if (aar != null) {
+        Preconditions.checkNotNull(aar.getAar());
+        Preconditions.checkNotNull(aar.getManifest());
+        args.add(
+            aar.getManifest().getRootRelativePathString()
+                + ":"
+                + aar.getAar().getRootRelativePathString());
+      }
+    }
+    return Joiner.on(",").join(args);
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
index 71eaac6850..dec8790a47 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java
@@ -211,7 +211,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
   protected String computeKey() {
     Fingerprint f = new Fingerprint();
     f.addString(GUID);
-    f.addStrings(executionSettings.getArgs().arguments());
+    f.addStrings(executionSettings.getArgs());
     f.addString(executionSettings.getTestFilter() == null ? "" : executionSettings.getTestFilter());
     RunUnder runUnder = executionSettings.getRunUnder();
     f.addString(runUnder == null ? "" : runUnder.getValue());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestTargetExecutionSettings.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestTargetExecutionSettings.java
index 5a01bcb0ba..9d3947cbc1 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestTargetExecutionSettings.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestTargetExecutionSettings.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.rules.test;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
@@ -22,20 +23,21 @@ import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesSupport;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
-import com.google.devtools.build.lib.analysis.actions.CommandLine;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.Path;
 
+import java.util.List;
+
 /**
  * Container for common test execution settings shared by all
  * all TestRunnerAction instances for the given test target.
  */
 public final class TestTargetExecutionSettings {
 
-  private final CommandLine testArguments;
+  private final List<String> testArguments;
   private final String testFilter;
   private final int totalShards;
   private final RunUnder runUnder;
@@ -53,8 +55,10 @@ public final class TestTargetExecutionSettings {
     Preconditions.checkArgument(shards >= 0);
     BuildConfiguration config = ruleContext.getConfiguration();
 
-    CommandLine targetArgs = runfilesSupport.getArgs();
-    testArguments = CommandLine.concat(targetArgs, ImmutableList.copyOf(config.getTestArguments()));
+    List<String> targetArgs = runfilesSupport.getArgs();
+    testArguments = targetArgs.isEmpty()
+      ? config.getTestArguments()
+      : ImmutableList.copyOf(Iterables.concat(targetArgs, config.getTestArguments()));
 
     totalShards = shards;
     runUnder = config.getRunUnder();
@@ -77,7 +81,7 @@ public final class TestTargetExecutionSettings {
         : runUnderTarget.getProvider(FilesToRunProvider.class).getExecutable();
   }
 
-  public CommandLine getArgs() {
+  public List<String> getArgs() {
     return testArguments;
   }