Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIMOB-25441] Android: Support ARM64 #9561

Merged
merged 5 commits into from Nov 7, 2017

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Oct 27, 2017

MERGE #9082 FIRST

  • Add native support for arm64-v8a
  • Update V8 to 6.2.414.36
    • Major performance improvements over the currently used V8 5.1
    • ES6 performance and compatibility improvements

JIRA Ticket

@lokeshchdhry
Copy link
Contributor

FR Passed.

  1. Built an app to device each time having modules ti.map 4.0.0, ti,cloudpush 5.0.0, ti.admob 4.0.0, facebook 7.0.0 & hyperloop 7.0.0 & SDK 7.0.0 having this fix.
  2. App built fine with above modules. No crash, errors seen.
  3. Also each module had a arm64-v8a folder in the libs folder.

Studio Ver: 4.10.0.201709271713
SDK Ver: 7.0.0 local build
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.10
Appc CLI: 6.3.0
Ti CLI Ver: 5.0.14
Alloy Ver: 1.10.7
Node Ver: 7.10.1
Java Ver: 1.8.0_101
Devices: ⇨ samsung SM-G955U1 --- Android 7.0

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me.

@sgtcoolguy
Copy link
Contributor

Build appears to be failing due to duplicated facebook classes? I took a quick look at the zip from github releases and it appears OK, but the build output shows two instances of the AudienceNetwork library being on the class path somehow. I'm gonna kick a rebuild and see if it just works. Maybe there was an earlier 7.0.0 of the module with both a jar and jar for the AudienceNetwork? cc @hansemannn

@sgtcoolguy
Copy link
Contributor

@hansemannn @garymathews So, unrelated to this PR directly is the issue that the Facebook 7.0.0 module for Android seems to be causing the test app to crash. Specifically it looks like now you must set the facebook application id in the AndroidManifest.xml or else just including the module causes a crash at startup:

[INFO] : �� � TiApplication: (main) [7,484] Titanium Javascript runtime: v8
[WARN] : �� � W/com.facebook.internal.Validate: FacebookActivity is not declared in the AndroidManifest.xml, please add com.facebook.FacebookActivity to your AndroidManifest.xml file. See https://developers.facebook.com/docs/android/getting-started for more info.
[ERROR] : �� �TiApplication: (main) [6,490] Sending event: exception on thread: main msg:java.lang.RuntimeException: Unable to create application com.appcelerator.testApp.testing.MochaApplication: A valid Facebook app id must be set in the AndroidManifest.xml or set by calling FacebookSdk.setApplicationId before initializing the sdk.; Titanium 7.0.0,2017/11/06 10:32,undefined
[ERROR] : �� �TiApplication: java.lang.RuntimeException: Unable to create application com.appcelerator.testApp.testing.MochaApplication: A valid Facebook app id must be set in the AndroidManifest.xml or set by calling FacebookSdk.setApplicationId before initializing the sdk.
[ERROR] : �� �TiApplication: 	at android.app.ActivityThread.handleBindApplication(ActivityThread.java:4710)
[ERROR] : �� �TiApplication: 	at android.app.ActivityThread.-wrap1(ActivityThread.java)
[ERROR] : �� �TiApplication: 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1405)
[ERROR] : �� �TiApplication: 	at android.os.Handler.dispatchMessage(Handler.java:102)
[ERROR] : �� �TiApplication: 	at android.os.Looper.loop(Looper.java:148)
[ERROR] : �� �TiApplication: 	at android.app.ActivityThread.main(ActivityThread.java:5417)
[ERROR] : �� �TiApplication: 	at java.lang.reflect.Method.invoke(Native Method)
[ERROR] : �� �TiApplication: 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
[ERROR] : �� �TiApplication: 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
[ERROR] : �� �TiApplication: Caused by: A valid Facebook app id must be set in the AndroidManifest.xml or set by calling FacebookSdk.setApplicationId before initializing the sdk.
[ERROR] : �� �TiApplication: 	at com.facebook.FacebookSdk.sdkInitialize(FacebookSdk.java:275)
[ERROR] : �� �TiApplication: 	at com.facebook.FacebookSdk.sdkInitialize(FacebookSdk.java:231)
[ERROR] : �� �TiApplication: 	at facebook.TiFacebookModule.onAppCreate(TiFacebookModule.java:140)
[ERROR] : �� �TiApplication: 	at com.appcelerator.testApp.testing.MochaApplication.onCreate(MochaApplication.java:129)
[ERROR] : �� �TiApplication: 	at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1013)
[ERROR] : �� �TiApplication: 	at android.app.ActivityThread.handleBindApplication(ActivityThread.java:4707)
[ERROR] : �� �TiApplication: 	... 8 more

I suppose we could re-work the test suite to not include that module (I think we only do some require() tests that relate to it, that may be able to use a different module). Any ideas?

@hansemannn
Copy link
Collaborator

Yep, the facebook app-id is required. We should rip it out of the test-suite and may consider own test-suites for modules in the future instead.

@sgtcoolguy
Copy link
Contributor

Well, the test suite isn't really testing facebook. It's testing some edge cases around require() with native modules (require a native module by id, require a JS file inside a folder whose names matches a native module's id). We just happen to have a native facebook module on both iOS and Android, so I used that. It may be a little tricky because we don't have a native module with the same id for all 3 major platforms, and the second test requires the folder name match the native module id.

@lokeshchdhry lokeshchdhry merged commit 130bf00 into tidev:master Nov 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants