From b1ae312f4eb220bc2729f1310dba7cd9b75f8d7f Mon Sep 17 00:00:00 2001 From: Michael Staib Date: Wed, 24 Aug 2016 22:34:37 +0000 Subject: Rollback of commit b5ac354867c4d8deb543285e87e636504078fb69. *** Reason for rollback *** Roll forward with fixes. -- MOS_MIGRATED_REVID=131224077 --- .../build/lib/rules/android/AndroidBinary.java | 24 +++------- .../lib/rules/android/AndroidConfiguration.java | 13 ------ .../lib/rules/android/AndroidRuleClasses.java | 26 ++--------- .../build/lib/rules/android/NativeLibs.java | 54 ++++++++++++---------- .../lib/rules/nativedeps/NativeDepsHelper.java | 43 +++++++++++++++-- 5 files changed, 80 insertions(+), 80 deletions(-) (limited to 'src') 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. @@ -1666,19 +1669,6 @@ public abstract class AndroidBinary implements RuleConfiguredTargetFactory { || ruleContext.attributes().isAttributeValueExplicitlySpecified("nocompress_extensions"); } - /** - * 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. */ 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. */ .add(attr("proguard_apply_mapping", LABEL).legacyAllowAnyFileType()) - /* - Enables legacy native support, where pre-compiled native libraries are copied - directly into the APK. - Possible values: - - */ - .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.>of(), null); - public static NativeLibs fromPrecompiledObjects( - RuleContext ruleContext, Multimap depsByArchitecture) { - ImmutableMap.Builder> builder = ImmutableMap.builder(); - for (Map.Entry> entry : - depsByArchitecture.asMap().entrySet()) { - NestedSet 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 configurationMap) throws InterruptedException { Map> result = new LinkedHashMap<>(); + String nativeDepsLibraryBasename = null; for (Map.Entry> 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 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 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 filterUniqueSharedLibraries( - RuleContext ruleContext, NestedSet libraries) { + RuleContext ruleContext, Artifact linkedLibrary, NestedSet libraries) { Map basenames = new HashMap<>(); Set 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 { *

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. * + *

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 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, -- cgit v1.2.3