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

[3.2] Add support of iOS's dynamic libraries to GDNative #39996

Merged
merged 5 commits into from
Jul 3, 2020

Conversation

naithar
Copy link
Contributor

@naithar naithar commented Jun 30, 2020

For iOS platform it is not allowed by AppStore to use .dylib directly, so not loading them at this moment makes sense.
But since the release of iOS 8 developers are allowed to use Frameworks, which essentially are dylibs packed with Info.plist file and additional install_name value set to them.

Because of that .dylibs can be replaced with .framework or .xcframework files built with XCode. They can also be turned into .framework automatically. This way the dynamic library loading will be available for iOS platform.

This repo contains the starter project, allowing the creation of .a, .dylib, .framework or .xcframework, which could be used for GDNative library in project.

I've tested this PR with both statically and dynamically linked libraries in different combinations, running on multiple devices that's available to me.
I've also tested the way .framework generation works and how AppStore handles resulting application.
I haven't found any issues, but I could have missed some edge case.

Should probably solve godotengine/godot-headers#30 and godotengine/gdnative-demos#19 for iOS part

@naithar naithar requested a review from akien-mga as a code owner June 30, 2020 18:03
@bruvzg
Copy link
Member

bruvzg commented Jun 30, 2020

dylibs should work on iOS without any issues, but AFAIK it is not allowed by App Store rules to have custom dylibs, is it allowed to use dynamic framework? If it is this is kinda strange, cause framework is the same thing as dylib (or static lib) but bundled with the info plist and headers. Or rules have changed?

@naithar
Copy link
Contributor Author

naithar commented Jun 30, 2020

dylibs should work on iOS without any issues, but AFAIK it is not allowed by App Store rules to have custom dylibs, is it allowed to use dynamic framework? If it is this is kinda strange, cause framework is the same thing as dylib (or static lib) but bundled with the info plist and headers. Or rules have changed?

Yeah, not being allowed by AppStore is what I meant. And yes dynamic frameworks are allowed (at least from iOS 8 that's for sure), they just need to be bundled in Frameworks folder in archived application.

I'm also not sure if dylibs are exactly the same as Frameworks, as when I was trying to workaround not being able to directly send dylibs to Review nothing helped.
I've tried bundling it to the framework, emulating framework folder structure. As the result archived application was always rejected as incorrect binary or something like that, even if I managed to pass initial unload state.
Binaries produced by creating dylib and framework also seems to be different, but I'm not sure if it's relevant.

@bruvzg
Copy link
Member

bruvzg commented Jun 30, 2020

Binaries produced by creating dylib and framework also seems to be different, but I'm not sure if it's relevant.

At least macOS dylibs and Frameworks seems to be identical, except the install name and LC_UUID (it's unique for every binary, but it's only used to find matching dSYM), it's possible to change install name using command like

install_name_tool -id @rpath/{NAME}.framework/Versions/A/{NAME} ./{NAME}.dylib

@naithar
Copy link
Contributor Author

naithar commented Jun 30, 2020

Binaries produced by creating dylib and framework also seems to be different, but I'm not sure if it's relevant.

At least macOS dylibs and Frameworks seems to be identical, except the install name and LC_UUID (it's unique for every binary, but it's only used to find matching dSYM), it's possible to change install name using command like

install_name_tool -id @rpath/{NAME}.framework/Versions/A/{NAME} ./{NAME}.dylib

Well it seems they are the same, but still .dylib are not allowed and even gets rejected on upload, while .framework and .xcframework are allowed and passes every check there are.
It would be great to automate turning .dylib into .framework, but I haven't found the way to do that which would not get rejected.

@naithar
Copy link
Contributor Author

naithar commented Jun 30, 2020

I will try to look more into turning dylibs into framework library, but as I said - using Frameworks is a recommended way to handle dynamic libraries

@naithar
Copy link
Contributor Author

naithar commented Jun 30, 2020

Well, it seems I've missed something before, but with install_name I've managed to create correct .framework library. Adding Info.plist with correct values seems to be good enough for upload step to pass.
Two builds that I sent still need to pass binary review to be available for testing.

I've also noticed that dylib are bigger than the library that gets built by XCode for about 1MB.

.xcframework with all architectures is lower in size by 5MB than all architectures dylib, but AppStore does not allow x86_64 architecture framework to be uploaded, so it would probably require to create a .xcframework.

@naithar
Copy link
Contributor Author

naithar commented Jul 1, 2020

I've updated the code, so now iOS exporter can turn dylib into framework.
But as I previously stated - the resulting frameworks are bigger then the ones generated from XCode. Which results in bigger app size even after Slicing on AppStore side is finished. The difference between .framework or .xcframework is 2MB when switching one framework to dylib.

I've also updated starter project

And on AppStore side everything is ok with build processing.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

I haven't fully tested it, but changes look fine.

@@ -26,6 +26,8 @@
<string>$signature</string>
<key>CFBundleVersion</key>
<string>$version</string>
<key>ITSAppUsesNonExemptEncryption</key>
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Godot includes some encryption features, both in its thirdparty libraries and some of it are exposed to the public scripting API (notably via the Crypto singleton). Did you assess if they fall within the "exempt" category for Apple?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I've seen from the source code, Godot does not use any self-written encryption algorithm, so it's should be ok. So far any app that I've published was okay with using any standard encryption.

@akien-mga
Copy link
Member

akien-mga commented Jul 2, 2020

It would be good to have those changes in master too, but since it can't build right now I understand that it's tricky :) Should we just open an issue instead as a reminder to port this code to master once iOS support is fixed?

Additionally, I plan to release 3.2.3 with a focus on fixing a few regressions in 2-3 weeks, so I'm cautious about merging new features to 3.2 right now. How risky do you see those changes for the "normal" iOS deployment workflow (without GDNative)?

Finally, I think it's good to keep separate commits for this PR (i.e. not squash them all into one) as they do separate updates to different components. Some of them might be worth grouping together though to reduce the number of commits (e.g. the commit adding .framework selection to the editor, and then the commit adding .dylib selection too).

@naithar
Copy link
Contributor Author

naithar commented Jul 2, 2020

It would be good to have those changes in master too, but since it can't build right now I understand that it's tricky :) Should we just open an issue instead as a reminder to port this code to master once iOS support is fixed?

I have a code ready for master branch, it was only waiting for a review :)

Additionally, I plan to release 3.2.3 with a focus on fixing a few regressions in 2-3 weeks, so I'm cautious about merging new features to 3.2 right now. How risky do you see those changes for the "normal" iOS deployment workflow (without GDNative)?

Normal deployment should work pretty much the same, since the code that got changed only handles .framework export, which wasn't really used. It event should result in even less warnings, since Framework Search Path got changes to support spaces.

Finally, I think it's good to keep separate commits for this PR (i.e. not squash them all into one) as they do separate updates to different components. Some of them might be worth grouping together though to reduce the number of commits (e.g. the commit adding .framework selection to the editor, and then the commit adding .dylib selection too).

I'll group commits and force push then. And make PR for master

@akien-mga akien-mga merged commit 24f527b into godotengine:3.2 Jul 3, 2020
@akien-mga
Copy link
Member

Thanks!

@naithar naithar deleted the feature/ios-gdnative branch July 3, 2020 08:30
@ArdaE
Copy link
Contributor

ArdaE commented Jul 20, 2020

Thanks for your work @naithar!

Here's a relevant article from Apple on the embedding of .framework (supported) vs .dylib (not supported):
https://developer.apple.com/library/archive/technotes/tn2435/_index.html

I thought it might be of interest to some who end up on this page.

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