Skip to content

Remove bitcode=NO from add-to-app flows#39503

Merged
jmagman merged 1 commit intoflutter:masterfrom
jmagman:add-bitcode
Sep 3, 2019
Merged

Remove bitcode=NO from add-to-app flows#39503
jmagman merged 1 commit intoflutter:masterfrom
jmagman:add-bitcode

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Aug 29, 2019 •
edited
Loading

Description

Enable bitcode on add-to-app flows.

Related Issues

Code-half of #39359.
Fixes #24366.

Tests

Make sure add-to-app and module_test_ios still pass.

Checklist

  • 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

  • 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.

@jmagman jmagman requested a review from dnfield August 29, 2019 18:30
@fluttergithubbot fluttergithubbot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Aug 29, 2019
@jmagman
Copy link
Member Author

jmagman commented Aug 29, 2019

@dnfield When I turn off bitcode I get this new failure in module_test_ios:

$ cd dev/devicelab
$ dart bin/run.dart -t module_test_ios
...
Task result:
{
  "success": false,
  "reason": "Ephemeral host app /var/folders/mv/qlhc4vsj6tqct166rw3zwmth00mfq2/T/flutter_module_test.l1wvRn/hello/build/ios/iphoneos/Runner.app's App.framework does not contain debug symbols"
}

It passes on master. Landed this change so you can check it out and take a look and to see if any other tests fail.

@jmagman jmagman self-assigned this Aug 29, 2019
dnfield
dnfield reviewed Aug 29, 2019
View reviewed changes
packages/flutter_tools/templates/module/ios/host_app_ephemeral_cocoapods/Podfile.copy.tmpl Outdated
Copy link
Contributor

@dnfield dnfield Aug 29, 2019

Choose a reason for hiding this comment

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

This makes me so happy. I believe we have some open issues around making this less painful for people right? Or were those already resolved by moving it here instead of into the podhelper.rb?

Copy link
Member Author

@jmagman jmagman Aug 29, 2019

Choose a reason for hiding this comment

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

You're maybe thinking of issues like #24342. I removed the post_install from the helper script with https://github.com/flutter/flutter/pull/36793/files#diff-b528f954e369c153bb31a631a8033be3L64, so I don't think it's painful anymore. This one only runs for the ephemeral module, so no one should be editing it.

Copy link
Member Author

@jmagman jmagman Aug 29, 2019

Choose a reason for hiding this comment

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

It makes me happy too, though!

@dnfield
Copy link
Contributor

dnfield commented Aug 29, 2019

We're not providing debug symbols with the profile build after the changes. I think we could, because profile is still bitcode-marker - but I'm not entirely sure that it's valuable to do so since they're symbols for assembly files. @xster, you added this requirement to the test a month ago - is there something I'm missing here about why it's improtant for App.framework in particular?

@dnfield
Copy link
Contributor

dnfield commented Aug 29, 2019

Ok, I see in #37065 the rationale for this - I'll see if I can add them back for profile.

@dnfield dnfield mentioned this pull request Aug 29, 2019
Merged
9 tasks
@dnfield
Copy link
Contributor

dnfield commented Aug 29, 2019

#39530 puts the symbols back for profile bitcode builds.

xster
xster reviewed Aug 30, 2019
View reviewed changes
Copy link
Member

@xster xster left a comment

Choose a reason for hiding this comment

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

LGTM

dnfield
dnfield approved these changes Aug 30, 2019
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM. Change to keep symbols in profile framework has landed, so hopefully this is good now.

@jmagman jmagman mentioned this pull request Aug 30, 2019
Closed
@jmagman jmagman changed the title WIP Remove bitcode=NO from add-to-app flows Remove bitcode=NO from add-to-app flows Aug 31, 2019
@jmagman jmagman merged commit 0b93c96 into flutter:master Sep 3, 2019
@jmagman jmagman deleted the add-bitcode branch September 9, 2019 20:25
@jmagman jmagman mentioned this pull request Oct 17, 2019
Merged
9 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@xster xster xster left review comments

+1 more reviewer

@dnfield dnfield dnfield approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmagman jmagman

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Changing device targets re-enabled ENABLE_BITCODE for iOS Add2App

5 participants

@jmagman @dnfield @xster @googlebot @fluttergithubbot