Rollback of commit 588a6a04c748a02f583d552660434d74190bb3ba.

*** Reason for rollback ***

Broke some targets

--
MOS_MIGRATED_REVID=126574275

5 files changed, 81 insertions, 83 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
index 082fe72374..db2e28470b 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
@@ -174,13 +174,10 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
       toolchainMap.put(config.getCpu(), CppHelper.getToolchain(ruleContext));
     }
 
-    NativeLibs nativeLibs =
-        NativeLibs.fromLinkedNativeDeps(
-            ruleContext,
-            androidSemantics.getNativeDepsFileName(),
-            depsByArchitecture,
-            toolchainMap,
-            configurationMap);
+    NativeLibs nativeLibs = shouldLinkNativeDeps(ruleContext)
+        ? NativeLibs.fromLinkedNativeDeps(ruleContext, androidSemantics.getNativeDepsFileName(),
+            depsByArchitecture, toolchainMap, configurationMap)
+        : NativeLibs.fromPrecompiledObjects(ruleContext, depsByArchitecture);
 
     // TODO(bazel-team): Resolve all the different cases of resource handling so this conditional
     // can go away: recompile from android_resources, and recompile from android_binary attributes.
@@ -1703,6 +1700,19 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
   }
 
   /**
+   * Returns whether to use NativeDepsHelper to link native dependencies.
+   */
+  public static boolean shouldLinkNativeDeps(RuleContext ruleContext) {
+    TriState attributeValue = ruleContext.attributes().get(
+        "legacy_native_support", BuildType.TRISTATE);
+    if (attributeValue == TriState.AUTO) {
+      return !ruleContext.getFragment(AndroidConfiguration.class).getLegacyNativeSupport();
+    } else {
+      return attributeValue == TriState.NO;
+    }
+  }
+
+  /**
    * Returns the multidex mode to apply to this target.
    */
   public static MultidexMode getMultidexMode(RuleContext ruleContext) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
index c33a27365b..d8f7cbb724 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
@@ -231,6 +231,13 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
             help = "Specifies Android SDK/platform that is used to build Android applications.")
     public Label sdk;
 
+    @Option(name = "legacy_android_native_support",
+        defaultValue = "true",
+        category = "semantics",
+        help = "Switches back to old native support for android_binaries. Disable to link together "
+            + "native deps of android_binaries into a single .so by default.")
+    public boolean legacyNativeSupport;
+
     // TODO(bazel-team): Maybe merge this with --android_cpu above.
     @Option(name = "fat_apk_cpu",
             converter = Converters.CommaSeparatedOptionListConverter.class,
@@ -388,6 +395,7 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
 
   private final Label sdk;
   private final StrictDepsMode strictDeps;
+  private final boolean legacyNativeSupport;
   private final String cpu;
   private final boolean incrementalNativeLibs;
   private final boolean usesAndroidCrosstool;
@@ -406,6 +414,7 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
     this.sdk = androidSdk;
     this.incrementalNativeLibs = options.incrementalNativeLibs;
     this.strictDeps = options.strictDeps;
+    this.legacyNativeSupport = options.legacyNativeSupport;
     this.cpu = options.cpu;
     this.usesAndroidCrosstool = (options.androidCrosstoolTop != null);
     this.configurationDistinguisher = options.configurationDistinguisher;
@@ -434,6 +443,10 @@ public class AndroidConfiguration extends BuildConfiguration.Fragment {
     return sdk;
   }
 
+  public boolean getLegacyNativeSupport() {
+    return legacyNativeSupport;
+  }
+
   public StrictDepsMode getStrictDeps() {
     return strictDeps;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
index 30d659b870..9be507bcfd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
@@ -691,10 +691,28 @@ com/google/common/base/Objects.class
           re-used to apply the same mapping to a new build.
           <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
           .add(attr("proguard_apply_mapping", LABEL).legacyAllowAnyFileType())
-          // TODO(mstaib): Remove this attribute and the matching flag after some cleanup of users
-          .add(attr("legacy_native_support", TRISTATE)
-              .value(TriState.AUTO)
-              .undocumented("No-op, soon to be removed"))
+          /* <!-- #BLAZE_RULE($android_binary_base).ATTRIBUTE(legacy_native_support) -->
+          Enables legacy native support, where pre-compiled native libraries are copied
+          directly into the APK.
+          Possible values:
+          <ul>
+              <li><code>legacy_native_support = 1</code>: Pre-built .so files found in the
+                dependencies of cc_libraries in the transitive closure will be copied into
+                the APK without being modified in any way. All cc_libraries in the transitive
+                closure of this rule must wrap .so files. (<em class="harmful">deprecated</em> -
+                legacy_native_support = 0 will become the default and this attribute will be
+                removed in a future Blaze release.)</li>
+              <li><code>legacy_native_support = 0</code>: Native dependencies in the transitive
+                closure will be linked together into a single lib[ruleName].so
+                before being placed in the APK. This ensures that, e.g., only one copy of
+                //base will be loaded into memory. This lib[ruleName].so can be loaded
+                via System.loadLibrary as normal.</li>
+              <li><code>legacy_native_support = -1</code>: Linking is controlled by the
+                <a href="../blaze-user-manual.html#flag--legacy_android_native_support">
+                --[no]legacy_android_native_support</a> Blaze flag.</li>
+            </ul>
+          <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+          .add(attr("legacy_native_support", TRISTATE).value(TriState.AUTO))
           .add(attr(":extra_proguard_specs", LABEL_LIST).value(JavaSemantics.EXTRA_PROGUARD_SPECS))
           .advertiseProvider(JavaCompilationArgsProvider.class)
           .build();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/NativeLibs.java b/src/main/java/com/google/devtools/build/lib/rules/android/NativeLibs.java
index a4b55a469c..fc736ae5e1 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/NativeLibs.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/NativeLibs.java
@@ -50,6 +50,18 @@ public final class NativeLibs {
   public static final NativeLibs EMPTY =
       new NativeLibs(ImmutableMap.<String, Iterable<Artifact>>of(), null);
 
+  public static NativeLibs fromPrecompiledObjects(
+      RuleContext ruleContext, Multimap<String, TransitiveInfoCollection> depsByArchitecture) {
+    ImmutableMap.Builder<String, Iterable<Artifact>> builder = ImmutableMap.builder();
+    for (Map.Entry<String, Collection<TransitiveInfoCollection>> entry :
+        depsByArchitecture.asMap().entrySet()) {
+      NestedSet<LinkerInput> nativeLibraries =
+          AndroidCommon.collectTransitiveNativeLibraries(entry.getValue());
+      builder.put(entry.getKey(), filterUniqueSharedLibraries(ruleContext, nativeLibraries));
+    }
+    return new NativeLibs(builder.build(), null);
+  }
+
   public static NativeLibs fromLinkedNativeDeps(
       RuleContext ruleContext,
       String nativeDepsFileName,
@@ -57,26 +69,20 @@ public final class NativeLibs {
       Map<String, CcToolchainProvider> toolchainMap,
       Map<String, BuildConfiguration> configurationMap) {
     Map<String, Iterable<Artifact>> result = new LinkedHashMap<>();
-    String nativeDepsLibraryBasename = null;
     for (Map.Entry<String, Collection<TransitiveInfoCollection>> entry :
         depsByArchitecture.asMap().entrySet()) {
       CcLinkParams linkParams = AndroidCommon.getCcLinkParamsStore(entry.getValue())
           .get(/* linkingStatically */ true, /* linkShared */ true);
 
-      Artifact nativeDepsLibrary =
-          NativeDepsHelper.linkAndroidNativeDepsIfPresent(
-              ruleContext,
-              linkParams,
-              configurationMap.get(entry.getKey()),
-              toolchainMap.get(entry.getKey()));
+      Artifact nativeDepsLibrary = NativeDepsHelper.maybeCreateAndroidNativeDepsAction(
+          ruleContext, linkParams, configurationMap.get(entry.getKey()),
+          toolchainMap.get(entry.getKey()));
 
       ImmutableList.Builder<Artifact> librariesBuilder = ImmutableList.builder();
+      librariesBuilder.addAll(filterUniqueSharedLibraries(ruleContext, linkParams.getLibraries()));
       if (nativeDepsLibrary != null) {
         librariesBuilder.add(nativeDepsLibrary);
-        nativeDepsLibraryBasename = nativeDepsLibrary.getExecPath().getBaseName();
       }
-      librariesBuilder.addAll(
-          filterUniqueSharedLibraries(ruleContext, nativeDepsLibrary, linkParams.getLibraries()));
       ImmutableList<Artifact> libraries = librariesBuilder.build();
 
       if (!libraries.isEmpty()) {
@@ -85,18 +91,17 @@ public final class NativeLibs {
     }
     if (result.isEmpty()) {
       return NativeLibs.EMPTY;
-    } else if (nativeDepsLibraryBasename == null) {
-      return new NativeLibs(ImmutableMap.copyOf(result), null);
     } else {
+      Artifact anyNativeLibrary =
+          result.entrySet().iterator().next().getValue().iterator().next();
       // The native deps name file must be the only file in its directory because ApkBuilder does
       // not have an option to add a particular file to the .apk, only one to add every file in a
       // particular directory.
       Artifact nativeDepsName = ruleContext.getUniqueDirectoryArtifact(
           "nativedeps_filename", nativeDepsFileName,
           ruleContext.getBinOrGenfilesDirectory());
-      ruleContext.registerAction(
-          new FileWriteAction(
-              ruleContext.getActionOwner(), nativeDepsName, nativeDepsLibraryBasename, false));
+      ruleContext.registerAction(new FileWriteAction(ruleContext.getActionOwner(), nativeDepsName,
+          anyNativeLibrary.getExecPath().getBaseName(), false));
 
       return new NativeLibs(ImmutableMap.copyOf(result), nativeDepsName);
     }
@@ -181,12 +186,9 @@ public final class NativeLibs {
   }
 
   private static Iterable<Artifact> filterUniqueSharedLibraries(
-      RuleContext ruleContext, Artifact linkedLibrary, NestedSet<? extends LinkerInput> libraries) {
+      RuleContext ruleContext, NestedSet<? extends LinkerInput> libraries) {
     Map<String, Artifact> basenames = new HashMap<>();
     Set<Artifact> artifacts = new HashSet<>();
-    if (linkedLibrary != null) {
-      basenames.put(linkedLibrary.getExecPath().getBaseName(), linkedLibrary);
-    }
     for (LinkerInput linkerInput : libraries) {
       String name = linkerInput.getArtifact().getFilename();
       if (!(CppFileTypes.SHARED_LIBRARY.matches(name)
@@ -204,17 +206,9 @@ public final class NativeLibs {
       if (oldArtifact != null) {
         // There may be name collisions in the libraries which were provided, so
         // check for this at this step.
-        ruleContext.ruleError(
-            "Each library in the transitive closure must have a unique basename to avoid "
-                + "name collisions when packaged into an apk, but two libraries have the basename '"
-                + basename
-                + "': "
-                + artifact.prettyPrint()
-                + " and "
-                + oldArtifact.prettyPrint()
-                + ((oldArtifact == linkedLibrary)
-                    ? " (the library compiled for this target)"
-                    : ""));
+        ruleContext.ruleError("Each library in the transitive closure must have a unique "
+            + "basename, but two libraries had the basename '" + basename + "': "
+            + artifact.prettyPrint() + " and " + oldArtifact.prettyPrint());
       }
     }
     return artifacts;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java b/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java
index 0b36ed61aa..fda5b4e8aa 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java
@@ -27,7 +27,6 @@ import com.google.devtools.build.lib.rules.cpp.CppBuildInfo;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
 import com.google.devtools.build.lib.rules.cpp.CppHelper;
 import com.google.devtools.build.lib.rules.cpp.CppLinkAction;
-import com.google.devtools.build.lib.rules.cpp.Link;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
 import com.google.devtools.build.lib.rules.cpp.LinkerInputs;
@@ -85,22 +84,16 @@ public abstract class NativeDepsHelper {
    * <p>linkshared=1 means we prefer the ".pic.a" files to the ".a" files, and the LinkTargetType is
    * set to DYNAMIC_LIBRARY which causes Link.java to include "-shared" in the linker options.
    *
-   * <p>It is possible that this function may have no work to do if there are no native libraries
-   * in the transitive closure, or if the only native libraries in the transitive closure are
-   * already shared libraries. In this case, this function returns {@code null}.
-   *
    * @param ruleContext the rule context to determine the native deps library
    * @param linkParams the {@link CcLinkParams} for the rule, collected with linkstatic = 1 and
    *        linkshared = 1
-   * @return the native deps library, or null if there was no code which needed to be linked in the
-   *         transitive closure.
+   * @return the native deps library runfiles. If the transitive deps closure of the rule contains
+   *         no native code libraries, its fields are null.
    */
-  public static Artifact linkAndroidNativeDepsIfPresent(
-      final RuleContext ruleContext,
-      CcLinkParams linkParams,
-      final BuildConfiguration configuration,
+  public static Artifact maybeCreateAndroidNativeDepsAction(final RuleContext ruleContext,
+      CcLinkParams linkParams, final BuildConfiguration configuration,
       CcToolchainProvider toolchain) {
-    if (!containsCodeToLink(linkParams.getLibraries())) {
+    if (linkParams.getLibraries().isEmpty()) {
       return null;
     }
 
@@ -121,36 +114,6 @@ public abstract class NativeDepsHelper {
         .getLibrary();
   }
 
-  /** Determines if there is any code to be linked in the input iterable. */
-  private static boolean containsCodeToLink(Iterable<LibraryToLink> libraries) {
-    for (LibraryToLink library : libraries) {
-      if (containsCodeToLink(library)) {
-        return true;
-      }
-    }
-    return false;
-  }
-
-  /** Determines if the input library is or contains an archive which must be linked. */
-  private static boolean containsCodeToLink(LibraryToLink library) {
-    if (Link.SHARED_LIBRARY_FILETYPES.matches(library.getArtifact().getFilename())) {
-      // this is a shared library so we're going to have to copy it
-      return false;
-    }
-    if (!library.containsObjectFiles()) {
-      // this is an opaque library so we're going to have to link it
-      return true;
-    }
-    for (Artifact object : library.getObjectFiles()) {
-      if (!Link.SHARED_LIBRARY_FILETYPES.matches(object.getFilename())) {
-        // this library was built with a non-shared-library object so we should link it
-        return true;
-      }
-    }
-    // there weren't any artifacts besides shared libraries compiled in the library
-    return false;
-  }
-
   public static NativeDepsRunfiles createNativeDepsAction(
       final RuleContext ruleContext,
       CcLinkParams linkParams,