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

chore(android): include x86_64 in module template #11384

Merged
merged 3 commits into from
Jan 21, 2020

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Dec 9, 2019

NOTE: REQUIRES #11294
  • Include x86_64 architecture in module builds by default
TEST CASE #1
  • Attempt to rebuild existing module such as ti.map
  • Built module should contain x86_64 libraries
TEST CASE #2
  • Attempt to build an application with Titanium 9.0.0 including modules built with Titanium 7.0.0+
  • Application should build successfully, but warn x86_64 has been excluded:
[INFO]  Module ti.map does not contain x86_64 ABI. Application will build without x86_64 ABI support!

JIRA Ticket

@build
Copy link
Contributor

build commented Dec 9, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 6690 tests are passing.
(There are 701 skipped tests not included in that total)

Generated by 🚫 dangerJS against d369767

@jquick-axway jquick-axway self-requested a review December 10, 2019 18:23
@lokeshchdhry
Copy link
Contributor

Found some issues. Have updated @garymathews.

@sgtcoolguy
Copy link
Contributor

@garymathews I think this is having issues with the native modules used in the test suite now. Maybe because of the addition of x86_64 architecture, it's requiring that arch in the modules (which obviously isn't there yet)?

@jquick-axway
Copy link
Contributor

jquick-axway commented Jan 7, 2020

Ideally, we want to support including 9.x.x modules and older modules for backward compatibility. But the issue with this is that the older built modules don't have x86_64 built libraries, which means you'll crash when running on an x86_64 device/emulator (it's all or nothing).

To do this, we can't bump up the apiversion. Otherwise it prevents us from using older modules in 9.0.0.

Proposal 1: (Best Solution?)
In our _build.js script's module validation code, we can check each module's architectures in their manifest. If at least 1 architecture is missing from a module compared to all other modules, then we remove that architecture from the app's architectures array.

Proposal 2: (Next Best)
In our module validation code, we can check the minsdk of all included modules. If we find at least 1 module that has a minsdk older than 9.0.0, then we should remove the x86_64 string from the architectures array. We should log a warning about if both an old and new module are included (mixed mode) and we can't support x86_64 in this case.

The problem with this is if a module requires a 3rd party C/C++ library (such as our encrypted DB module) and a prebuilt *.so does not exist for x86 or x86_64, then the above check will not help us. Only "Proposal 1" above handles this case.

Proposal 3: (Worst Case)
If the app includes at least 1 module whose mindsdk is 9.0.0 or higher, then all modules need to be 9.0.0 or higher. That is, older modules cannot be included and we trigger a build failure. It's all or nothing.

I don't think this is a good solution though. We're going to need to rebuild ti.playservices for 9.0.0, so, odds are most devs are going to get bit by this.

@jquick-axway
Copy link
Contributor

jquick-axway commented Jan 7, 2020

Also, our _buildModule.js script's manifest migration should probably force devs to add x86_64 to the architectures: property in the module's manifest file.

Should it even be configurable at all? Most of the time no... unless you're including a 3rd party C/C++ *.so library like our encrypted database module. Hmm...

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@jquick-axway
Copy link
Contributor

@lokeshchdhry, I suggest that you test with our ti.imagefactory module instead. Now that our AndroidX PR has been merged, the ti.map module won't build without code changes since it has Google Support library class references (it needs to be migrated).

@lokeshchdhry
Copy link
Contributor

FR Passed.

  1. Module built with SDK 9.0.0 builds & works as expected with an app. It has the x86_64 arch.
  2. Module built with 7.0.0+ also works when used in an app built with SDK 9.0.0. As it does not have the x86_84 arch we see the warning as defined in this PR.

Studio Ver: 5.1.4.201909061933
SDK Ver: 9.0.0
OS Ver: 10.14.5
Xcode Ver: Xcode 11.3
Appc NPM: 4.2.15
Appc CLI: 7.1.2
Daemon Ver: 1.1.3
Ti CLI Ver: 5.2.2
Alloy Ver: 1.14.4
Node Ver: 12.13.1
NPM Ver: 6.12.1
Java Ver: 11.0.1
Android Devices: ⇨ google Pixel (Android 10)

@garymathews garymathews merged commit 1d1667e into tidev:master Jan 21, 2020
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.

5 participants