-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Enable bitcode compilation for AOT #36471
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
Conversation
| exit -1 | ||
| fi | ||
|
|
||
| local bitcode_flag="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this into the if [[ -n "$LOCAL_ENGINE" ]]; block since it currently will fail without a local engine built with bitcode? Since ENABLE_BITCODE is the default in Xcode I'm afraid someone in an add-to-app trying Flutter out for the first time will hit a bunch of weird errors if they don't swap it to NO.
Of course it doesn't work now if a dev keeps the default YES, but the "local-engine required" error is more confusing for someone just messing around with Flutter in their app the first time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense - what I was hoping here is that this would prompt people interested in using it to see how they can do it, but that's probably the wrong way right now. Eventually we could move this back out.
jmagman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plan here is to make this a bit more accessible to early adopters who want to test it. They can test it by either using flutter build aot --target-platform=ios --bitcode --profile --local-engine=ios_profile, or by setting ENABLE_BITCODE to true in their xcode project (and building with a local engine).
Do they also need to remove any
config.build_settings['ENABLE_BITCODE'] = 'NO'
lines from the Podfile?
|
Yes, they would have to do that. I wrote some instructions here https://github.com/flutter/flutter/wiki/Creating-an-iOS-Bitcode-enabled-app and will update them. |
jmagman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exciting!
Codecov Report
@@ Coverage Diff @@
## master #36471 +/- ##
=========================================
Coverage ? 54.67%
=========================================
Files ? 188
Lines ? 17402
Branches ? 0
=========================================
Hits ? 9514
Misses ? 7888
Partials ? 0
Continue to review full report at Codecov.
|
| const String kTargetFile = 'TargetFile'; | ||
|
|
||
| /// The define to control whether the AOT snapshot is built with bitcode. | ||
| const String kBitcodeFlag = 'bitcode'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call this EnableBitcode or similar
jonahwilliams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you for all of the testing
| printError('Failed to link AOT snapshot. Linker terminated with exit code ${compileResult.exitCode}'); | ||
| return linkResult; | ||
| } | ||
| final RunResult dsymResult = await xcode.dsymutil(<String>[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ends up creating 3 App.framework.dSYM folders in the build/ directory. Unfortunately, it seems like this issue #22540 affects local xcode too. If spotlight finds these files, any attempt to desymbolicate locally will attach to one of these files. But since we don't know which binary architecture the developer ended up using, xcode gets confused. Can we put these transient files in .noindex folders too?
Description
See https://github.com/flutter/flutter/wiki/Creating-an-iOS-Bitcode-enabled-app for instructions on how to use this functionality in its present state.
This makes it possible to build a bitcode enabled iOS app if you're using a local engine built with bitcode.
The plan here is to make this a bit more accessible to early adopters who want to test it. They can test it by either using
flutter build aot --target-platform=ios --bitcode --profile --local-engine=ios_profile, or by settingENABLE_BITCODEto true in their xcode project (and building with a local engine). We're not yet vending engine binaries with bitcode - I'd like to enable this as a hidden flag for now that people can start testing and validating.@jonahwilliams mentioned this should get analytics - I'm not really sure how to do that but would be happy to add some.
This also really wants an integration test, but that would be very expensive to do right now - we'd have to checkout the engine sources for the framework commit, build them (and we can't use goma), and then use a local engine. I've added some unit tests to the parts of the build system I changed, except for xcode_backend.sh which isn't really testable (I've tested it manually).
Related Issues
Part of #15288
Tests
I added the following tests:
Tests that building AOT with and without bitcode support makes the expected invocations and modifies the generated assembly as expected.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?