Skip to content

Avoid thinning frameworks in iOS extensions #65198

Merged
jmagman merged 2 commits intoflutter:masterfrom
jmagman:lipo
Sep 15, 2020
Merged

Avoid thinning frameworks in iOS extensions #65198
jmagman merged 2 commits intoflutter:masterfrom
jmagman:lipo

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Sep 3, 2020 •
edited
Loading

Reland #64674
Always run the pod install fingerprint check if there is a Podfile, even if there are no Flutter plugins.

Description

Watch extensions are embedded in iOS apps. Avoid lipo'ing those frameworks to iOS architectures. Stick to the app frameworks.

Related Issues

Fixes #60351

Tests

Updated ios_app_with_extensions_test to include a watch framework. Check the build succeeds, and that the watch extension frameworks have the expected watch architectures.

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 read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • 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

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@jmagman jmagman added platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. labels Sep 3, 2020
@jmagman jmagman self-assigned this Sep 3, 2020
@flutter-dashboard flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Sep 3, 2020
@jmagman jmagman changed the title Avoid thinning frameworks in iOS extensions (#64674) Avoid thinning frameworks in iOS extensions Sep 3, 2020
jmagman
jmagman commented Sep 3, 2020
View reviewed changes
packages/flutter_tools/lib/src/macos/cocoapod_utils.dart Outdated
Copy link
Member Author

@jmagman jmagman Sep 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is the part that should fix the post-commit failure from #64674.
Run pod install (assuming the fingerprint says it's needed) when there's a Podfile even if there aren't Flutter plugins.
@xster What do you think about this? Looks like the project.isModule caveat was added back in #21276.

Copy link
Contributor

@christopherfujino christopherfujino Sep 3, 2020

Choose a reason for hiding this comment

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

nit. Maybe a comment to explain why we have to pod install even if it doesn't have plugins?

Copy link
Member Author

@jmagman jmagman Sep 3, 2020

Choose a reason for hiding this comment

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

Good idea, done.

Copy link
Member

@xster xster Sep 4, 2020

Choose a reason for hiding this comment

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

I wish that original line could be DeMorgan rewritten 😂

That isModule part is definitely weird. I can see the argument for just generally not running pod install when there's only a user Podfile without plugin. Since at that point, it doesn't have much to do with Flutter's build pipeline. When a user changes his own Podfile, the user should run pod install themselves. Our customers did complain back in beta days about pod install always running for no changes and being slow.

I can't think of why isModule + has Podfile means pod instead has to run. Removing isModule from this consideration altogether seems reasonable. "'Cause if you like it, then you shoulda put a test on it"

Copy link
Member Author

@jmagman jmagman Sep 15, 2020

Choose a reason for hiding this comment

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

I reverted this, and instead added a Flutter plugin to the integration test so the tool runs pod install for us and I don't need to call it manually.

christopherfujino
christopherfujino approved these changes Sep 3, 2020
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@zanderso
Copy link
Member

zanderso commented Sep 10, 2020

What's the status of this PR?

@jmagman
Copy link
Member Author

jmagman commented Sep 10, 2020

When I get back from vacation I am going to revert the changes to cocoapod_utils.dart based on @xster's feedback and update the integration test to pass (either add a Flutter plugin or run pod install manually)

@jmagman jmagman merged commit 7e27b14 into flutter:master Sep 15, 2020
@jmagman jmagman deleted the lipo branch September 15, 2020 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xster xster Awaiting requested review from xster

1 more reviewer

@christopherfujino christopherfujino christopherfujino 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. platform-ios iOS applications specifically 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.

Must run flutter clean before building when iOS app has watch extension, Failed to extract arm64 for .../PlugIns/WatchApp.appex/Frameworks

5 participants

@jmagman @zanderso @xster @christopherfujino @googlebot