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

iOS Static Build Pipeline #3436

Closed
wants to merge 20 commits into from
Closed

iOS Static Build Pipeline #3436

wants to merge 20 commits into from

Conversation

CatalinVoss
Copy link
Collaborator

This PR contributes a static build framework build pipeline for iOS, hopefully helping push #3061 along. Credit goes to @xiaoqunSun for pointing me to bazel's ios_static_framework.

Build the static framework like this:

cd tensorflow

bazel build --verbose_failures --config=ios_arm64 --workspace_status_command="bash native_client/bazel_workspace_status_cmd.sh" --apple_bitcode=embedded --copt=-fembed-bitcode --config=monolithic -c opt //native_client:deepspeech_ios --define=runtime=tflite --copt=-DTFLITE_WITH_RUY_GEMV

# Replace .framework folder in Xcode project
rm -rf ../native_client/swift/deepspeech_ios.framework
unzip bazel-bin/native_client/deepspeech_ios.zip
mv deepspeech_ios.framework ../native_client/swift/

If you want a smaller bundle and don't care about shipping apps with bitcode to Apple, you can drop the --apple_bitcode=embedded --copt=-fembed-bitcode flags.

Then the xcodebuild or Xcode app build should work to build a static framework that can be integrated into apps that can be shipped on the iOS App Store or via Testflight.

I tried my best to update the Taskcluster command, but don't want to overpromise.

I also added a basic Podspec which should provide the starting point for turning this into a Cocoa Pod if anyone wants to tackle submitting that somewhere.

@lissyx
Copy link
Collaborator

lissyx commented Nov 24, 2020

I tried my best to update the Taskcluster command, but don't want to overpromise.

Please @CatalinVoss, feel free to push as much as you need to complete taskcluster green, do not worry about abusing the resources: current usage is quite low, and we have implemented back in august optimizations that ensures high usages do not results in too high costs. Current figures shows this has been successfull, and your PR is more than welcome, so push as much as you need.

@CatalinVoss
Copy link
Collaborator Author

@lissyx that's good to know. OK, thanks.

@CatalinVoss
Copy link
Collaborator Author

I think we're good on TC?

@lissyx
Copy link
Collaborator

lissyx commented Nov 25, 2020

I think we're good on TC?

There were à few failures but its all green after rerun. I'll gave à look tomorrow, hopefully. Thanks !

@CatalinVoss
Copy link
Collaborator Author

Yeah those didn't look related to me. OK

@lissyx
Copy link
Collaborator

lissyx commented Nov 26, 2020

Yeah those didn't look related to me. OK

They were not, but they were blocking executing a lot of the tests :)

@lissyx
Copy link
Collaborator

lissyx commented Nov 26, 2020

@CatalinVoss Looks good, there were two things I'm unsure about, please let us know if you need further changes or if that is good to go.

Also @reuben can correct me but I think we would like to have that on r0.9 as well ?

@reuben
Copy link
Contributor

reuben commented Nov 26, 2020

I don't think so. Not suitable for a point release in its current state. Would like to get more testing on it. I'll take a closer look at the PR as well this week.

@CatalinVoss
Copy link
Collaborator Author

Looks like I don't have permissions to re-trigger that one failing windows test, but it doesn't look related

@lissyx
Copy link
Collaborator

lissyx commented Nov 26, 2020

Looks like I don't have permissions to re-trigger that one failing windows test, but it doesn't look related

Yeah dont worry about this one, it's past deadline Anyway and it's just an intermittent

Copy link
Contributor

@reuben reuben left a comment

Choose a reason for hiding this comment

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

I have a few minor comments that need to be addressed before merging but this looks great! Great find with ios_static_framework, this ended up being much simpler than I expected.

native_client/swift/deepspeech-ios.podspec Outdated Show resolved Hide resolved
native_client/BUILD Outdated Show resolved Hide resolved
native_client/BUILD Outdated Show resolved Hide resolved
@lissyx
Copy link
Collaborator

lissyx commented Dec 4, 2020

@CatalinVoss Sorry, but valgrind metadata tests on linux shows a memory leak regression: https://community-tc.services.mozilla.com/tasks/VizPwDWmRa-rx6i2f2z_1w/runs/0/logs/https%3A%2F%2Fcommunity-tc.services.mozilla.com%2Fapi%2Fqueue%2Fv1%2Ftask%2FVizPwDWmRa-rx6i2f2z_1w%2Fruns%2F0%2Fartifacts%2Fpublic%252Fvalgrind_extended.log

@CatalinVoss It might just be some false-positive from Valgrind:

==4364== LEAK SUMMARY:
==4364==    definitely lost: 0 bytes in 0 blocks
==4364==    indirectly lost: 0 bytes in 0 blocks
==4364==      possibly lost: 0 bytes in 0 blocks
==4364==    still reachable: 96 bytes in 2 blocks
==4364==                       of which reachable via heuristic:
==4364==                         newarray           : 9,280 bytes in 11 blocks

And you would just have to update the exclusion lists: https://github.com/mozilla/DeepSpeech/blob/master/taskcluster/tc-valgrind-utils.sh#L5-L9

@reuben
Copy link
Contributor

reuben commented Dec 4, 2020

Looks like re-adding the flags that were dropped when moving from tf_cc_shared_object to cc_library fixed it: #3451

@reuben reuben closed this in bc07842 Dec 4, 2020
@reuben
Copy link
Contributor

reuben commented Dec 4, 2020

Merged with the fix. Thanks @CatalinVoss!

@reuben
Copy link
Contributor

reuben commented Dec 4, 2020

(Please delete the upstream branch once you're done working on it)

@CatalinVoss
Copy link
Collaborator Author

Thank you for that!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants