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-25621] Android: Do not delete third-party native libraries #9695

Merged
merged 3 commits into from Jan 9, 2018

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Dec 21, 2017

TEST CASE
  • Compile the appcelerator.encrypteddatabase module
  • Verify the appropriate *.so libraries are included in the .zip output

JIRA Ticket

@build
Copy link
Contributor

build commented Dec 22, 2017

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@hansemannn
Copy link
Collaborator

hansemannn commented Dec 29, 2017

This is not a valid change. The native libraries are recreated during the build by compiling for all architectures specified in the package.json of the Ti-Android project. The encrypted database likely failed because before the change the arm64-v8a was not always created which was caused by not cleaning up the existing libs. Please test this very well in case it should be taken.

EDIT: Looking further into it, appcelerator.encryptedatabase 3.0.0 was compiled on November 16, a few weeks before that PR. It should fix exactly that issue we had before. Let me know your thoughts!

@garymathews
Copy link
Contributor Author

@hansemannn This behavior never happened before, the issue is any third-party libraries are deleted during the newly added cleanup phase.

@garymathews
Copy link
Contributor Author

@hansemannn I have updated the PR to only remove the built module *.so files and not third-party libraries.

@lokeshchdhry
Copy link
Contributor

FR Passed.

The .so files are included in the compiled zip in libs/* folder.

Studio Ver: 5.0.0.201712081732
SDK Ver: 7.1.0 local build
OS Ver: 10.13.2
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.11
Appc CLI: 7.0.1
Daemon Ver: 1.0.1
Ti CLI Ver: 5.0.14
Alloy Ver: 1.10.10
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 1.8.0_101

@hansemannn
Copy link
Collaborator

@garymathews @lokeshchdhry Sorry to step in again, but this ticket should not have been merged. The first reason is that it haven't been CR-tested in any way. The second reason is that the binary that caused the ticket was obviously compiled before 7.0.1 was released. And the last third reason is that this PR will likely introduce a regression for the case that we remove an old architecture again, like in 6.0.0. In that case, the .so will be deleted, but the directory will still be present, which will cause a module build error when verifying the build architectures. Please have this in mind once it pops up.

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