Skip to content

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Jul 18, 2019

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 setting ENABLE_BITCODE to 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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

exit -1
fi

local bitcode_flag=""
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@jmagman jmagman left a 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?

@dnfield
Copy link
Contributor Author

dnfield commented Jul 18, 2019

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.

Copy link
Member

@jmagman jmagman left a 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
Copy link

codecov bot commented Jul 18, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@5a34e79). Click here to learn what that means.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #36471   +/-   ##
=========================================
  Coverage          ?   54.67%           
=========================================
  Files             ?      188           
  Lines             ?    17402           
  Branches          ?        0           
=========================================
  Hits              ?     9514           
  Misses            ?     7888           
  Partials          ?        0
Flag Coverage Δ
#flutter_tool 54.67% <73.33%> (?)
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/build_info.dart 70.71% <ø> (ø)
packages/flutter_tools/lib/src/ios/mac.dart 27.83% <100%> (ø)
...utter_tools/lib/src/build_system/targets/dart.dart 91.13% <100%> (ø)
packages/flutter_tools/lib/src/macos/xcode.dart 54.71% <33.33%> (ø)
...ages/flutter_tools/lib/src/commands/build_aot.dart 35.71% <65.51%> (ø)
packages/flutter_tools/lib/src/base/build.dart 73.86% <88.23%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a34e79...a8a6ecd. Read the comment docs.

const String kTargetFile = 'TargetFile';

/// The define to control whether the AOT snapshot is built with bitcode.
const String kBitcodeFlag = 'bitcode';
Copy link
Contributor

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

Copy link
Contributor

@jonahwilliams jonahwilliams left a 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

@dnfield dnfield merged commit c953cd1 into flutter:master Jul 19, 2019
@dnfield dnfield deleted the bitcode branch July 19, 2019 05:42
printError('Failed to link AOT snapshot. Linker terminated with exit code ${compileResult.exitCode}');
return linkResult;
}
final RunResult dsymResult = await xcode.dsymutil(<String>[
Copy link
Member

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?

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants