Support copy-only native code in modern native support for android_binary.

legacy_native_support makes it so that Bazel copies .so's instead of linking.
This allows Bazel to include third-party native code without running an NDK.
Turning it off allows Bazel to actually compile native code, but at the cost
of this copy case no longer working without an NDK, and creating an unnecessary
shared library which will most likely never be used.

This CL makes it so that if no actual source code or static libraries are
in the transitive closure, the linker is not run at the android_binary level.

This means that legacy_native_support as a separate concept can be removed,
and it is - the attribute remains, but is a no-op. This will be removed in
a future change. The matching flag (--legacy_android_native_support) has
already been removed.

RELNOTES: The link mode for Android native code is now automatically determined
based on the transitive closure of cc_libraries, and legacy_native_support is
now a no-op. --legacy_android_native_support has been removed.

--
MOS_MIGRATED_REVID=126244755

5 files changed, 83 insertions, 81 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 0048c1565f..c1f129d029 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,10 +174,13 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
       toolchainMap.put(config.getCpu(), CppHelper.getToolchain(ruleContext));
     }
 
-    NativeLibs nativeLibs = shouldLinkNativeDeps(ruleContext)
-        ? NativeLibs.fromLinkedNativeDeps(ruleContext, androidSemantics.getNativeDepsFileName(),
-            depsByArchitecture, toolchainMap, configurationMap)
-        : NativeLibs.fromPrecompiledObjects(ruleContext, depsByArchitecture);
+    NativeLibs nativeLibs =
+        NativeLibs.fromLinkedNativeDeps(
+            ruleContext,
+            androidSemantics.getNativeDepsFileName(),
+            depsByArchitecture,
+            toolchainMap,
+            configurationMap);
 
     // 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.
@@ -1698,19 +1701,6 @@ 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 d8f7cbb724..c33a27365b 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,13 +231,6 @@ 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,
@@ -395,7 +388,6 @@ 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;
@@ -414,7 +406,6 @@ 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;
@@ -443,10 +434,6 @@ 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 6d116f1970..2a8d499f40 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
@@ -689,28 +689,10 @@ 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())
-          /* <!-- #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))
+          // 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"))
           .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 fc736ae5e1..a4b55a469c 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,18 +50,6 @@ 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,
@@ -69,20 +57,26 @@ 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.maybeCreateAndroidNativeDepsAction(
-          ruleContext, linkParams, configurationMap.get(entry.getKey()),
-          toolchainMap.get(entry.getKey()));
+      Artifact nativeDepsLibrary =
+          NativeDepsHelper.linkAndroidNativeDepsIfPresent(
+              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()) {
@@ -91,17 +85,18 @@ 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,
-          anyNativeLibrary.getExecPath().getBaseName(), false));
+      ruleContext.registerAction(
+          new FileWriteAction(
+              ruleContext.getActionOwner(), nativeDepsName, nativeDepsLibraryBasename, false));
 
       return new NativeLibs(ImmutableMap.copyOf(result), nativeDepsName);
     }
@@ -186,9 +181,12 @@ public final class NativeLibs {
   }
 
   private static Iterable<Artifact> filterUniqueSharedLibraries(
-      RuleContext ruleContext, NestedSet<? extends LinkerInput> libraries) {
+      RuleContext ruleContext, Artifact linkedLibrary, 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)
@@ -206,9 +204,17 @@ 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, but two libraries had the basename '" + basename + "': "
-            + artifact.prettyPrint() + " and " + oldArtifact.prettyPrint());
+        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)"
+                    : ""));
       }
     }
     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 30f4d58962..ed1bdbbfb8 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,6 +27,7 @@ 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;
@@ -82,16 +83,22 @@ 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 runfiles. If the transitive deps closure of the rule contains
-   *         no native code libraries, its fields are null.
+   * @return the native deps library, or null if there was no code which needed to be linked in the
+   *         transitive closure.
    */
-  public static Artifact maybeCreateAndroidNativeDepsAction(final RuleContext ruleContext,
-      CcLinkParams linkParams, final BuildConfiguration configuration,
+  public static Artifact linkAndroidNativeDepsIfPresent(
+      final RuleContext ruleContext,
+      CcLinkParams linkParams,
+      final BuildConfiguration configuration,
       CcToolchainProvider toolchain) {
-    if (linkParams.getLibraries().isEmpty()) {
+    if (!containsCodeToLink(linkParams.getLibraries())) {
       return null;
     }
 
@@ -112,6 +119,36 @@ 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,