Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Commit

Permalink
Make default CPU ABIs depend on the NDK version
Browse files Browse the repository at this point in the history 
Summary:
`armeabi` and MIPS* were removed from NDK 17: https://developer.android.com/ndk/downloads/revision_history

This change modifies the logic to not use `arm` by default when building with NDK 17.

Reviewed By: bobyangyf

fbshipit-source-id: 1cb08d6
  • Loading branch information
@facebook-github-bot
Sergey Tyurin authored and facebook-github-bot committed Jul 5, 2018
1 parent d9d6c6f commit 4ef8e96
Show file tree
Hide file tree
Showing 31 changed files with 279 additions and 63 deletions.
3 changes: 1 addition & 2 deletions src/com/facebook/buck/android/AndroidBuckConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Optional;
import java.util.Set;

public class AndroidBuckConfig {

Expand Down Expand Up @@ -78,7 +77,7 @@ public Optional<String> getNdkAppPlatform() {
return delegate.getValue("ndk", "app_platform");
}

public Optional<Set<String>> getNdkCpuAbis() {
public Optional<ImmutableSet<String>> getNdkCpuAbis() {
return delegate.getOptionalListWithoutComments("ndk", "cpu_abis").map(ImmutableSet::copyOf);
}

Expand Down
14 changes: 11 additions & 3 deletions src/com/facebook/buck/android/toolchain/ndk/impl/NdkCxxPlatforms.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,6 @@ public class NdkCxxPlatforms {

public static final NdkCompilerType DEFAULT_COMPILER_TYPE = NdkCompilerType.GCC;
public static final String DEFAULT_TARGET_APP_PLATFORM = "android-16";
public static final ImmutableSet<String> DEFAULT_CPU_ABIS =
ImmutableSet.of("arm", "armv7", "x86");
public static final NdkCxxRuntime DEFAULT_CXX_RUNTIME = NdkCxxRuntime.GNUSTL;

private static final ImmutableMap<Platform, Host> BUILD_PLATFORMS =
Expand Down Expand Up @@ -228,10 +226,20 @@ public static ImmutableMap<TargetCpuType, NdkCxxPlatform> getPlatforms(
androidConfig.getNdkCxxRuntime().orElse(NdkCxxPlatforms.DEFAULT_CXX_RUNTIME),
androidConfig.getNdkCxxRuntimeType().orElse(NdkCxxRuntimeType.DYNAMIC),
androidConfig.getNdkAppPlatform().orElse(NdkCxxPlatforms.DEFAULT_TARGET_APP_PLATFORM),
androidConfig.getNdkCpuAbis().orElse(NdkCxxPlatforms.DEFAULT_CPU_ABIS),
androidConfig.getNdkCpuAbis().orElseGet(() -> getDefaultCpuAbis(ndkVersion)),
platform);
}

@VisibleForTesting
static ImmutableSet<String> getDefaultCpuAbis(String ndkVersion) {
int ndkMajorVersion = getNdkMajorVersion(ndkVersion);
if (ndkMajorVersion > 16) {
return ImmutableSet.of("armv7", "x86");
} else {
return ImmutableSet.of("arm", "armv7", "x86");
}
}

@VisibleForTesting
public static ImmutableMap<TargetCpuType, NdkCxxPlatform> getPlatforms(
CxxBuckConfig config,
Expand Down
35 changes: 32 additions & 3 deletions test/com/facebook/buck/android/AndroidAarIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,10 @@ public void testCxxLibraryDependent() throws InterruptedException, IOException {
zipInspector.assertFileExists("classes.jar");
zipInspector.assertFileExists("R.txt");
zipInspector.assertFileExists("res/");
zipInspector.assertFileExists("jni/armeabi/libdep.so");
zipInspector.assertFileExists("jni/armeabi/libnative.so");
if (AssumeAndroidPlatform.isArmAvailable()) {
zipInspector.assertFileExists("jni/armeabi/libdep.so");
zipInspector.assertFileExists("jni/armeabi/libnative.so");
}
zipInspector.assertFileExists("jni/armeabi-v7a/libdep.so");
zipInspector.assertFileExists("jni/armeabi-v7a/libnative.so");
zipInspector.assertFileExists("jni/x86/libdep.so");
Expand All @@ -254,10 +256,37 @@ public void testNativeLibraryDependent() throws InterruptedException, IOExceptio
zipInspector.assertFileExists("classes.jar");
zipInspector.assertFileExists("R.txt");
zipInspector.assertFileExists("res/");
zipInspector.assertFileExists("assets/lib/armeabi/libfoo.so");
zipInspector.assertFileExists("assets/lib/armeabi-v7a/libfoo.so");
zipInspector.assertFileExists("assets/lib/x86/libfoo.so");
zipInspector.assertFileExists("jni/armeabi-v7a/libbar.so");
zipInspector.assertFileExists("jni/x86/libbar.so");
}

@Test
public void testNativeLibraryDependentWithNDKPrior17() throws IOException {
AssumeAndroidPlatform.assumeNdkIsAvailable();
AssumeAndroidPlatform.assumeArmIsAvailable();
ProjectWorkspace workspace =
TestDataHelper.createProjectWorkspaceForScenario(
this, "android_aar_native_deps/ndk_deps", tmp);
workspace.setKnownBuildRuleTypesFactoryFactory(DefaultKnownBuildRuleTypesFactory::of);
workspace.setUp();
String target = "//:app-16";
workspace.runBuckBuild(target).assertSuccess();

Path aar =
workspace.getPath(
BuildTargets.getGenPath(
filesystem, BuildTargetFactory.newInstance(target), AndroidAar.AAR_FORMAT));
ZipInspector zipInspector = new ZipInspector(aar);
zipInspector.assertFileExists("AndroidManifest.xml");
zipInspector.assertFileExists("classes.jar");
zipInspector.assertFileExists("R.txt");
zipInspector.assertFileExists("res/");
zipInspector.assertFileExists("assets/lib/armeabi/libfoo.so");
zipInspector.assertFileExists("jni/armeabi/libbar.so");
zipInspector.assertFileExists("assets/lib/armeabi-v7a/libfoo.so");
zipInspector.assertFileExists("assets/lib/x86/libfoo.so");
zipInspector.assertFileExists("jni/armeabi-v7a/libbar.so");
zipInspector.assertFileExists("jni/x86/libbar.so");
}
Expand Down
13 changes: 3 additions & 10 deletions test/com/facebook/buck/android/AndroidBinaryCxxIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@ public void testCxxLibraryDep() throws IOException {
workspace.getPath(
BuildTargets.getGenPath(
filesystem, BuildTargetFactory.newInstance(target), "%s.apk")));
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileExists("lib/armeabi/libgnustl_shared.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libnative_cxx_lib.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libgnustl_shared.so");
zipInspector.assertFileExists("lib/x86/libnative_cxx_lib.so");
Expand All @@ -108,8 +106,6 @@ public void testCxxLibraryDepStaticRuntime() throws IOException {
workspace.getPath(
BuildTargets.getGenPath(
filesystem, BuildTargetFactory.newInstance(target), "%s.apk")));
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileDoesNotExist("lib/armeabi/libgnustl_shared.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libnative_cxx_lib.so");
zipInspector.assertFileDoesNotExist("lib/armeabi-v7a/libgnustl_shared.so");
zipInspector.assertFileExists("lib/x86/libnative_cxx_lib.so");
Expand All @@ -126,8 +122,6 @@ public void testCxxLibraryDepModular() throws IOException {
workspace.getPath(
BuildTargets.getGenPath(
filesystem, BuildTargetFactory.newInstance(target), "%s.apk")));
zipInspector.assertFileDoesNotExist("lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileExists("lib/armeabi/libgnustl_shared.so");
zipInspector.assertFileDoesNotExist("lib/armeabi-v7a/libnative_cxx_lib.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libgnustl_shared.so");
zipInspector.assertFileDoesNotExist("lib/x86/libnative_cxx_lib.so");
Expand All @@ -149,8 +143,6 @@ public void testCxxLibraryDepClang() throws IOException {
workspace.getPath(
BuildTargets.getGenPath(
filesystem, BuildTargetFactory.newInstance(target), "%s.apk")));
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileExists("lib/armeabi/libc++_shared.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libnative_cxx_lib.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libc++_shared.so");
zipInspector.assertFileExists("lib/x86/libnative_cxx_lib.so");
Expand All @@ -167,7 +159,9 @@ public void testCxxLibraryDepWithNoFilters() throws IOException {
workspace.getPath(
BuildTargets.getGenPath(
filesystem, BuildTargetFactory.newInstance(target), "%s.apk")));
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
if (AssumeAndroidPlatform.isArmAvailable()) {
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
}
zipInspector.assertFileExists("lib/armeabi-v7a/libnative_cxx_lib.so");
zipInspector.assertFileExists("lib/x86/libnative_cxx_lib.so");
}
Expand All @@ -182,7 +176,6 @@ public void testNoCxxDepsDoesNotIncludeNdkRuntime() throws IOException {
workspace.getPath(
BuildTargets.getGenPath(
filesystem, BuildTargetFactory.newInstance(target), "%s.apk")));
zipInspector.assertFileDoesNotExist("lib/armeabi/libgnustl_shared.so");
zipInspector.assertFileDoesNotExist("lib/armeabi-v7a/libgnustl_shared.so");
zipInspector.assertFileDoesNotExist("lib/x86/libgnustl_shared.so");
}
Expand Down
75 changes: 69 additions & 6 deletions test/com/facebook/buck/android/AndroidBinaryIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,11 @@ public void testNonExopackageHasSecondary() throws IOException {
zipInspector.assertFileDoesNotExist("classes2.dex");

zipInspector.assertFileExists("classes.dex");
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
if (AssumeAndroidPlatform.isArmAvailable()) {
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
}
zipInspector.assertFileExists("lib/armeabi-v7a/libnative_cxx_lib.so");
zipInspector.assertFileExists("lib/x86/libnative_cxx_lib.so");
}

@Test
Expand All @@ -124,7 +128,11 @@ public void testProguardBuild() throws IOException {
zipInspector.assertFileDoesNotExist("classes2.dex");

zipInspector.assertFileExists("classes.dex");
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
if (AssumeAndroidPlatform.isArmAvailable()) {
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
}
zipInspector.assertFileExists("lib/armeabi-v7a/libnative_cxx_lib.so");
zipInspector.assertFileExists("lib/x86/libnative_cxx_lib.so");
}

@Test
Expand All @@ -143,29 +151,56 @@ public void testRawSplitDexHasSecondary() throws IOException {
zipInspector.assertFileExists("classes2.dex");

zipInspector.assertFileExists("classes.dex");
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
if (AssumeAndroidPlatform.isArmAvailable()) {
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
}
zipInspector.assertFileExists("lib/armeabi-v7a/libnative_cxx_lib.so");
zipInspector.assertFileExists("lib/x86/libnative_cxx_lib.so");
}

@Test
public void testDisguisedExecutableIsRenamedWithNDKPrior17() throws IOException {
AssumeAndroidPlatform.assumeArmIsAvailable();
Path output = workspace.buildAndReturnOutput("//apps/sample:app_with_disguised_exe-16");
ZipInspector zipInspector = new ZipInspector(output);
zipInspector.assertFileExists("lib/armeabi/libmybinary.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libmybinary.so");
zipInspector.assertFileExists("lib/x86/libmybinary.so");
}

@Test
public void testDisguisedExecutableIsRenamed() throws IOException {
Path output = workspace.buildAndReturnOutput("//apps/sample:app_with_disguised_exe");
ZipInspector zipInspector = new ZipInspector(output);
zipInspector.assertFileExists("lib/armeabi/libmybinary.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libmybinary.so");
zipInspector.assertFileExists("lib/x86/libmybinary.so");
}

@Test
public void testNdkLibraryIsIncludedWithNdkPrior17() throws IOException {
AssumeAndroidPlatform.assumeArmIsAvailable();
Path output = workspace.buildAndReturnOutput("//apps/sample:app_with_ndk_library-16");
ZipInspector zipInspector = new ZipInspector(output);
zipInspector.assertFileExists("lib/armeabi/libfakenative.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libfakenative.so");
zipInspector.assertFileExists("lib/mips/libfakenative.so");
zipInspector.assertFileExists("lib/x86/libfakenative.so");
}

@Test
public void testNdkLibraryIsIncluded() throws IOException {
Path output = workspace.buildAndReturnOutput("//apps/sample:app_with_ndk_library");
ZipInspector zipInspector = new ZipInspector(output);
zipInspector.assertFileExists("lib/armeabi/libfakenative.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libfakenative.so");
zipInspector.assertFileExists("lib/x86/libfakenative.so");
}

@Test
public void testEditingNdkLibraryForcesRebuild() throws IOException, InterruptedException {
String apkWithNdkLibrary = "//apps/sample:app_with_ndk_library";
Path output = workspace.buildAndReturnOutput(apkWithNdkLibrary);
ZipInspector zipInspector = new ZipInspector(output);
zipInspector.assertFileExists("lib/armeabi/libfakenative.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libfakenative.so");

// Sleep 1 second (plus another half to be super duper safe) to make sure that
// fakesystem.c gets a later timestamp than the fakesystem.o that was produced
Expand Down Expand Up @@ -472,6 +507,34 @@ public void testApkEmptyResDirectoriesBuildsCorrectly() throws IOException {
workspace.runBuckBuild("//apps/sample:app_with_aar_and_no_res").assertSuccess();
}

@Test
public void testNativeLibGeneratedProguardConfigIsUsedByProguardWithNdkPrior17()
throws IOException {
AssumeAndroidPlatform.assumeArmIsAvailable();
String target = "//apps/sample:app_with_native_lib_proguard-16";
workspace.runBuckBuild(target).assertSuccess();

Path generatedConfig =
workspace.getPath(
BuildTargets.getGenPath(
filesystem,
BuildTargetFactory.newInstance(target)
.withFlavors(AndroidBinaryGraphEnhancer.NATIVE_LIBRARY_PROGUARD_FLAVOR),
NativeLibraryProguardGenerator.OUTPUT_FORMAT));

Path proguardDir =
workspace.getPath(
BuildTargets.getGenPath(
filesystem, BuildTargetFactory.newInstance(target), "%s/proguard"));

Path proguardCommandLine = proguardDir.resolve("command-line.txt");
// Check that the proguard command line references the native lib proguard config.
assertTrue(workspace.getFileContents(proguardCommandLine).contains(generatedConfig.toString()));
assertEquals(
workspace.getFileContents("native/proguard_gen/expected-16.pro"),
workspace.getFileContents(generatedConfig));
}

@Test
public void testNativeLibGeneratedProguardConfigIsUsedByProguard() throws IOException {
String target = "//apps/sample:app_with_native_lib_proguard";
Expand Down
18 changes: 9 additions & 9 deletions test/com/facebook/buck/android/AndroidExopackageBinaryIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public void testDexExopackageHasNoSecondary() throws IOException {
zipInspector.assertFileDoesNotExist("classes2.dex");

zipInspector.assertFileExists("classes.dex");
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libnative_cxx_lib.so");

// It would be better if we could call getExopackageInfo on the app rule.
Path secondaryDir =
Expand Down Expand Up @@ -130,7 +130,7 @@ public void testNativeExopackageHasNoNativeLibraries() throws IOException {

zipInspector.assertFileExists("classes.dex");

zipInspector.assertFileDoesNotExist("lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileDoesNotExist("lib/armeabi-v7a/libnative_cxx_lib.so");
}

@Test
Expand All @@ -145,7 +145,7 @@ public void testDexAndNativeExopackageHasNeitherSecondaryNorNativeLibraries() th

zipInspector.assertFileDoesNotExist("assets/secondary-program-dex-jars/metadata.txt");
zipInspector.assertFileDoesNotExist("classes2.dex");
zipInspector.assertFileDoesNotExist("lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileDoesNotExist("lib/armeabi-v7a/libnative_cxx_lib.so");

zipInspector.assertFileExists("classes.dex");
}
Expand Down Expand Up @@ -226,8 +226,8 @@ public void testEditingNativeForcesRebuild() throws IOException {
workspace.getBuildLog().assertTargetBuiltLocally(DEX_EXOPACKAGE_TARGET);
zipInspector =
new ZipInspector(workspace.getPath("buck-out/gen/apps/multidex/app-dex-exo.apk"));
zipInspector.assertFileExists("lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileDoesNotExist("assets/lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libnative_cxx_lib.so");
zipInspector.assertFileDoesNotExist("assets/lib/armeabi-v7a/libnative_cxx_lib.so");

// Now convert it into an asset native library and ensure that we re-run apkbuilder.
workspace.replaceFileContents(
Expand All @@ -239,8 +239,8 @@ public void testEditingNativeForcesRebuild() throws IOException {
workspace.getBuildLog().assertTargetBuiltLocally(DEX_EXOPACKAGE_TARGET);
zipInspector =
new ZipInspector(workspace.getPath("buck-out/gen/apps/multidex/app-dex-exo.apk"));
zipInspector.assertFileDoesNotExist("lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileExists("assets/lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileDoesNotExist("lib/armeabi-v7a/libnative_cxx_lib.so");
zipInspector.assertFileExists("assets/lib/armeabi-v7a/libnative_cxx_lib.so");

// Now edit it again and make sure we re-run apkbuilder.
workspace.replaceFileContents("native/cxx/lib.cpp", "return 4", "return 5");
Expand All @@ -254,8 +254,8 @@ public void testEditingNativeForcesRebuild() throws IOException {
workspace.getPath(
BuildTargets.getGenPath(
filesystem, BuildTargetFactory.newInstance(DEX_EXOPACKAGE_TARGET), "%s.apk")));
zipInspector.assertFileDoesNotExist("lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileExists("assets/lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileDoesNotExist("lib/armeabi-v7a/libnative_cxx_lib.so");
zipInspector.assertFileExists("assets/lib/armeabi-v7a/libnative_cxx_lib.so");
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion test/com/facebook/buck/android/AndroidFullExopackageBinaryIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public void testApkHasABunchOfThingsNotIncluded() throws IOException {

zipInspector.assertFileDoesNotExist("assets/secondary-program-dex-jars/metadata.txt");
zipInspector.assertFileDoesNotExist("classes2.dex");
zipInspector.assertFileDoesNotExist("lib/armeabi/libnative_cxx_lib.so");
zipInspector.assertFileDoesNotExist("lib/armeabi-v7a/libnative_cxx_lib.so");
zipInspector.assertFileDoesNotExist("assets/hilarity.txt");

zipInspector.assertFileExists("classes.dex");
Expand Down
6 changes: 4 additions & 2 deletions test/com/facebook/buck/android/AndroidInstrumentationApkIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,10 @@ public void testCxxLibraryDep() throws InterruptedException, IOException {
workspace.getPath(
BuildTargets.getGenPath(
filesystem, BuildTargetFactory.newInstance(target), "%s.apk")));
zipInspector.assertFileExists("lib/armeabi/libcxx.so");
zipInspector.assertFileExists("lib/armeabi/libgnustl_shared.so");
if (AssumeAndroidPlatform.isArmAvailable()) {
zipInspector.assertFileExists("lib/armeabi/libcxx.so");
zipInspector.assertFileExists("lib/armeabi/libgnustl_shared.so");
}
zipInspector.assertFileExists("lib/armeabi-v7a/libcxx.so");
zipInspector.assertFileExists("lib/armeabi-v7a/libgnustl_shared.so");
zipInspector.assertFileExists("lib/x86/libcxx.so");
Expand Down
14 changes: 7 additions & 7 deletions test/com/facebook/buck/android/AssumeAndroidPlatform.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,22 +46,22 @@ public static void assumeNdkIsAvailable() {
assumeTrue(androidNdk.isPresent());
}

public static void assumeArchIsAvailable(String arch) {
if ("arm".equals(arch)) {
assumeArmIsAvailable();
}
public static void assumeArmIsAvailable() {
assumeTrue(isArmAvailable());
}

public static void assumeArmIsAvailable() {
public static boolean isArmAvailable() {
ProjectFilesystem projectFilesystem =
TestProjectFilesystems.createProjectFilesystem(Paths.get(".").toAbsolutePath());
Optional<AndroidNdk> androidNdk = AndroidNdkHelper.detectAndroidNdk(projectFilesystem);

assumeTrue(androidNdk.isPresent());
if (!androidNdk.isPresent()) {
return false;
}

VersionStringComparator comparator = new VersionStringComparator();

assumeTrue(comparator.compare(androidNdk.get().getNdkVersion(), "17") < 0);
return comparator.compare(androidNdk.get().getNdkVersion(), "17") < 0;
}

public static void assumeUnifiedHeadersAvailable() {
Expand Down
Loading

0 comments on commit 4ef8e96

Please sign in to comment.