Relax requirements around local engine, build hello_world with bitcode by dnfield · Pull Request #39357 · flutter/flutter

dnfield · 2019-08-27T21:23:31Z

Description

Now that we build the engine with bitcode on CI, we should no longer require local engine builds in the tool to enable bitcode.

It is still not turned on by default - a user would have to update their Xcode project to set ENABLE_BITCODE to YES, or explicitly pass --bitcode to flutter build aot.

I've also cleaned up some unnecessary changes around the dsym util - apparently it doesn't make sense to have symbols anyway with bitcode, and it was failing locally without some changes for me.

Unfortunately, this no longer validates that bitcode is present in the binary because the method used doesn't work on FAT binaries. We could still validate them by e.g. running xcrun bitcode_strip on the binary and validating that the output is different from the input, but it seems excessive and would slow down build times.

Related Issues

Fixes #15288 (!)

Filed #39356 to track the remaining work once this has had some time to bake.

Tests

I added the following tests:

Updated existing tests for the new changes/removed code.

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.

jonahwilliams · 2019-08-27T21:25:28Z

packages/flutter_tools/lib/src/base/build.dart

        '-miphoneos-version-min=8.0',
    ];

+    final List<String> bitcodeArgs = <String>['-fembed-bitcode'];


Re-using this list seems likely to cause errors in the future.

Changed it to a const string and an expanded single use list below.

jonahwilliams · 2019-08-27T21:25:46Z

packages/flutter_tools/lib/src/macos/xcode.dart


 import 'dart:async';

+import 'package:flutter_tools/src/base/common.dart';


Nit: relative import

jmagman · 2019-08-27T21:27:21Z

examples/hello_world/ios/Runner.xcodeproj/project.pbxproj

 			buildSettings = {
 				ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
-				ENABLE_BITCODE = NO;
+				ENABLE_BITCODE = YES;


YES should be the default, remove the setting instead of overriding.

jmagman · 2019-08-27T21:33:01Z

packages/flutter_tools/lib/src/base/build.dart

    }

+    if (bitcode) {
+      final String iPhoneSdk = await xcode.iPhoneSdkLocation();


How do you know you need the iPhone SDK?

A warning is generated otherwise. It may be harmless but I'd like to avoid it.

Is this codepath is used for iphonesimulator and macosx too?

Or rather, will it be used for macosx in the future since bitcode is only set from xcode_backend. I believe this would need to be iphonesimulator in the Debug simulator case though?

This can't be used for simulator because it's AOT and specific to ARM.

It looks like it's already being used for macOS. I'll move this behind some guards similar to what's used for -miphoneos

We only support x64 desktop, but it is probably better to have semantically named selection

dnfield · 2019-08-27T21:34:55Z

packages/flutter_tools/lib/src/base/build.dart

    // gen_snapshot would provide an argument to do this automatically.
    if (platform == TargetPlatform.ios && bitcode) {
-      final IOSink sink = fs.file('$assembly.bitcode').openWrite();
+      final IOSink sink = fs.file('$assembly.stripped.S').openWrite();


cc will emit a warning if this file doesn't end with .S.

jmagman

LGTM

codecov · 2019-08-27T22:50:41Z

Codecov Report

Merging #39357 into master will decrease coverage by 0.78%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master   #39357      +/-   ##
==========================================
- Coverage   56.76%   55.97%   -0.79%     
==========================================
  Files         195      195              
  Lines       18430    18413      -17     
==========================================
- Hits        10461    10306     -155     
- Misses       7969     8107     +138

Flag	Coverage Δ
#flutter_tool	`55.97% <50%> (-0.79%)`	⬇️

Impacted Files	Coverage Δ
packages/flutter_tools/lib/src/macos/xcode.dart	`57.14% <0%> (-3.58%)`	⬇️
packages/flutter_tools/lib/src/base/build.dart	`68.06% <100%> (+0.03%)`	⬆️
...ages/flutter_tools/lib/src/commands/build_aot.dart	`37.5% <66.66%> (-1.76%)`	⬇️
...s/flutter_tools/lib/src/tester/flutter_tester.dart	`13.4% <0%> (-57.74%)`	⬇️
packages/flutter_tools/lib/src/bundle.dart	`31.81% <0%> (-43.94%)`	⬇️
packages/flutter_tools/lib/src/base/net.dart	`59.57% <0%> (-25.54%)`	⬇️
packages/flutter_tools/lib/src/version.dart	`73.68% <0%> (-19.14%)`	⬇️
packages/flutter_tools/lib/src/device.dart	`55.42% <0%> (-1.81%)`	⬇️
packages/flutter_tools/lib/src/cache.dart	`43.02% <0%> (-0.73%)`	⬇️
packages/flutter_tools/lib/src/base/terminal.dart	`70.32% <0%> (-0.64%)`	⬇️
... and 19 more

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 6f71ce2...f186393. Read the comment docs.

jonahwilliams

LGTM

Please ignore patch coverage, it is bad. Someday, I'll figure out how to fix it....

…h bitcode (#39357)" This reverts commit 202c1b4.

…h bitcode (#39357)" (#39431) This reverts commit 202c1b4.

…h bitcode (flutter#39357)" (flutter#39431)" This reverts commit 393106f.

Relax requirements around local engine, build hello_world with bitcode

0a00c4e

dnfield requested review from jmagman and jonahwilliams August 27, 2019 21:23

fluttergithubbot added d: examples Sample code and demos c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Aug 27, 2019

googlebot added the cla: yes label Aug 27, 2019

dnfield added t: xcode "xcodebuild" on iOS and general Xcode project management platform-ios iOS applications specifically labels Aug 27, 2019

jonahwilliams reviewed Aug 27, 2019

View reviewed changes

jmagman reviewed Aug 27, 2019

View reviewed changes

dnfield mentioned this pull request Aug 27, 2019

Add Bitcode support for iOS #15288

Closed

jmagman reviewed Aug 27, 2019

View reviewed changes

review/remove unintended verbose

3b9587f

dnfield commented Aug 27, 2019

View reviewed changes

dnfield mentioned this pull request Aug 27, 2019

Update documentation about bitcode requirements #39359

Closed

better guard

f186393

jmagman approved these changes Aug 27, 2019

View reviewed changes

jonahwilliams approved these changes Aug 28, 2019

View reviewed changes

dnfield merged commit 202c1b4 into flutter:master Aug 28, 2019

dnfield deleted the bitcode_for_all branch August 28, 2019 17:27

jonahwilliams pushed a commit that referenced this pull request Aug 28, 2019

Revert "Relax requirements around local engine, build hello_world wit…

1a4c938

…h bitcode (#39357)" This reverts commit 202c1b4.

jonahwilliams mentioned this pull request Aug 28, 2019

Revert "Relax requirements around local engine, build hello_world with bitcode" #39431

Merged

jonahwilliams pushed a commit that referenced this pull request Aug 28, 2019

Revert "Relax requirements around local engine, build hello_world wit…

393106f

…h bitcode (#39357)" (#39431) This reverts commit 202c1b4.

dnfield added a commit to dnfield/flutter that referenced this pull request Aug 28, 2019

Reland "Relax requirements around local engine, build hello_world wit…

0e18f1c

…h bitcode (flutter#39357)" (flutter#39431)" This reverts commit 393106f.

dnfield mentioned this pull request Aug 28, 2019

Reland "Relax arguments around local engine, build hello_world with bitcode" #39434

Merged

jmagman mentioned this pull request Sep 3, 2019

Remove ENABLE_BITCODE from Scenarios test app flutter/engine#11839

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021


		import 'dart:async';

		import 'package:flutter_tools/src/base/common.dart';

Conversation

dnfield commented Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dnfield commented Aug 27, 2019 •

edited

Loading

codecov bot commented Aug 27, 2019 •

edited

Loading