Rollback of commit b5ac354867c4d8deb543285e87e636504078fb69.

*** Reason for rollback ***

Roll forward with fixes.

--
MOS_MIGRATED_REVID=131224077

5 files changed, 80 insertions, 80 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 42e911842b..daf7d5b086 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
@@ -165,10 +165,13 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory {
       toolchainMap.put(cpu, toolchain);
     }
 
-    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.
@@ -1667,19 +1670,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 4b9c95003d..be0360514e 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
@@ -230,13 +230,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,
@@ -397,7 +390,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 ConfigurationDistinguisher configurationDistinguisher;
@@ -415,7 +407,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.configurationDistinguisher = options.configurationDistinguisher;
     this.useJackForDexing = options.useJackForDexing;
@@ -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 ba70ab5092..dc1312662e 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
@@ -694,28 +694,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 cbffcd8f0e..24d7c22d7e 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
@@ -48,18 +48,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,
@@ -68,6 +56,7 @@ public final class NativeLibs {
       Map<String, BuildConfiguration> configurationMap)
       throws InterruptedException {
     Map<String, Iterable<Artifact>> result = new LinkedHashMap<>();
+    String nativeDepsLibraryBasename = null;
     for (Map.Entry<String, Collection<TransitiveInfoCollection>> entry :
         depsByArchitecture.asMap().entrySet()) {
       CcLinkParams linkParams =
@@ -76,15 +65,20 @@ public final class NativeLibs {
                   ImmutableList.of("-Wl,-soname=lib" + ruleContext.getLabel().getName()))
               .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()) {
@@ -93,17 +87,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);
     }
@@ -188,9 +183,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)
@@ -208,9 +206,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 98b3867d4d..d724d7f032 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
@@ -29,6 +29,7 @@ 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.CppLinkActionBuilder;
+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,19 +86,23 @@ 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(
+  public static Artifact linkAndroidNativeDepsIfPresent(
       final RuleContext ruleContext,
       CcLinkParams linkParams,
       final BuildConfiguration configuration,
       CcToolchainProvider toolchain)
       throws InterruptedException {
-    if (linkParams.getLibraries().isEmpty()) {
+    if (!containsCodeToLink(linkParams.getLibraries())) {
       return null;
     }
 
@@ -122,6 +127,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,