Add support for ABI-specific NDK app platforms #1889
Conversation
|
CI failures look unrelated. |
LegNeato
force-pushed
the
ndk_abi_platform
branch
from
d6fd69f to
a3764d3
Compare
|
@LegNeato has updated the pull request. |
LegNeato
force-pushed
the
ndk_abi_platform
branch
from
a3764d3 to
0f2b098
Compare
|
@LegNeato has updated the pull request. |
|
Does this also fix #1890 potentially? |
|
@kageiit haven't 100% looked into it, but I believe so from a quick glance. That being said, I think there is more work to do as it would be pretty crappy for buck to not work by default and need to be configured in order to work with the latest ndk. |
|
Friendly poke 👉 @styurin |
| @@ -268,6 +264,7 @@ public static boolean isSupportedConfiguration(Path ndkRoot, NdkCxxRuntime cxxRu | |||
|
|
|||
| // ARM Platform | |||
There was a problem hiding this comment.
Github is not letting me comment on the actual code line in this file, but can we change
public static final ImmutableSet<String> DEFAULT_CPU_ABIS =
ImmutableSet.of("arm", "armv7", "x86");
to
public static final ImmutableSet<String> DEFAULT_CPU_ABIS =
ImmutableSet.of("armv7", "x86");
That will make buck work by default with the latest ndk
There was a problem hiding this comment.
Sure, though that is a different change. I'll put up another PR. Also, it would be a breaking change for people not using the latest ndk so I am not sure if that is acceptable or not.
LegNeato
force-pushed
the
ndk_abi_platform
branch
from
0f2b098 to
beaf18f
Compare
|
@LegNeato has updated the pull request. |
|
Fixed formatting to pass Travis. |
docs/concept/buckconfig.soy
Outdated
| {literal}<pre class="prettyprint lang-ini"> | ||
| [ndk] | ||
| app_platform = android-19 | ||
| app_platform-arm64 = android-21 |
There was a problem hiding this comment.
Can we have a map instead? Buck configuration supports keeping maps in a single option. For example:
app_platform_per_abi = arm64 => android-21, armv7 => android-23
There was a problem hiding this comment.
Sure, though I did it this way to try to generally be consistent with https://buckbuild.com/concept/buckconfig.html#halide.target. Still want me to change it?
There was a problem hiding this comment.
Yeah, could you change that to using a map?
| @@ -65,7 +68,7 @@ public Integer getAdbTimeout() { | |||
| return delegate.getValue("ndk", "ndk_repository_path"); | |||
| } | |||
|
|
|||
| public Optional<String> getNdkAppPlatform() { | |||
| public Optional<String> getNdkCpuAbiAgnosticAppPlatform() { | |||
There was a problem hiding this comment.
I thought "default" might be confusing as it isn't actually a providing real default in the config sense, that happens further down in the stack. So "default" is kinda ambiguous here for callers. I guess I could have used "fallback", which feels more correct than default to me as well (getFallbackAppPlatform or getNdkCpuAbiFallbackAppPlatform). Happy to change it to whatever you want.
|
@styurin Any pointers on how to check that a particular app platform is used when building? I can definitely write integration tests I just can't reason about how to go about it. I guess I can use this to build for arm64...but then running the tests will require a min version of the NDK installed...is that acceptable? Right now it looks like travis uses |
|
Check this class, it has some check for Android SDK: https://github.com/facebook/buck/blob/master/test/com/facebook/buck/android/AssumeAndroidPlatform.java |
|
@styurin Added integration tests that compile and check the symbols. Let me know about your responses to the naming and map stuff and I'll change them if needed...otherwise this is good to go. Not sure what is going on with Travis, it all passes locally. |
There was a problem hiding this comment.
Looks like builds are failing. Are you doing buck build on master locally?
Travis will build with ant and buck itself.
https://travis-ci.org/facebook/buck/builds/383549665?utm_source=github_status&utm_medium=notification
Can you make sure the BUCK files have the correct dependencies listed.
This looks like it might also be a merge conflict somewhere.
LegNeato
force-pushed
the
ndk_abi_platform
branch
from
785ea91 to
1c13944
Compare
|
Travis failure is unrelated and fixed by #1925. |
|
@styurin @bobyangyf this should be ready now! |
|
Thanks. We'll import it now. |
There was a problem hiding this comment.
@bobyangyf has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -0,0 +1,36 @@ | |||
| #ifdef __ANDROID__ | |||
| # include <android/api-level.h> | |||
There was a problem hiding this comment.
Internally, the NdkIntegration tests are failing due to this import being missing.
Are you expecting some specific installation of android?
There was a problem hiding this comment.
@bobyangyf not exactly sure, can't find an answer as to when that was introduced or removed. Does using # include <sys/cdefs.h> instead fix it with your version? What version of the ndk are you building with?
There was a problem hiding this comment.
It looks like only ndk20+ has this header file.
https://android.googlesource.com/platform/development/+/73a5a3b/ndk/platforms
We can change it so that
#ifdef __ANDROID_NDK__ #include <android/api-level.h> #endif
However, this breaks your tests at the places that I am highlighting on your code review.
| zipInspector.assertFileExists("lib/armeabi-v7a/libnative_app_platform_lib.so"); | ||
|
|
||
| info = syms.getSymbolsAndDtNeeded(apkPath, "lib/x86/libnative_app_platform_lib.so"); | ||
| assertThat(info.symbols.global, Matchers.hasItem("Android16")); |
| zipInspector.assertFileExists("lib/armeabi-v7a/libnative_app_platform_lib.so"); | ||
|
|
||
| info = syms.getSymbolsAndDtNeeded(apkPath, "lib/x86/libnative_app_platform_lib.so"); | ||
| assertThat(info.symbols.global, Matchers.hasItem("Android18")); |
Fixes #1836.