Set DEFINES_MODULE=YES in plugin templates #41828

jmagman · 2019-10-02T21:42:06Z

Description

Limit the supported podspec platform to iOS so tests don't run (and fail) for macOS.
Define the module by setting DEFINES_MODULE in the podspec. See CocoaPod modular headers docs. DEFINES_MODULE=YES by default when I make a new Obj-C or Swift framework in Xcode 11.
Explicitly set VALID_ARCHS to x86_64 for the simulator. This is the CocoaPods-suggested workaround to prevent tests from running on the i386 simulator. See Fix linting when armv7 is included but i386 isn't CocoaPods/CocoaPods#8159.
Flutter.framework doesn't include i386 and ONLY_ACTIVE_ARCH=NO on Release means Xcode will try to run the i386 simulator. Limit it to x86_64 on the simulator No way to add passing test_spec that doesn't cause build warning "Mapping architecture armv7 to i386. Ensure that this target's Architectures and Valid Architectures build settings are configured correctly for the iOS Simulator platform" CocoaPods/CocoaPods#9210. Not necessary on macOS since there is no simulator!

Related Issues

Fixes #41801
See also #41007.

Tests

I added the following tests:

plugin_lint_mac creates and runs pod lib lint for Objective-C plugins.

But why not lint Swift plugins?

you may ask. Anything importing Swift plugins does not build when ONLY_ACTIVE_ARCH=NO (the default in Release) and a simulator is targeted since there is no i386 bits in the Flutter.framework. This was true before this patch since Flutter doesn't run in Release on the simulator. In any case, the Swift import doesn't build in Release, though I'm not really sure why it works for Objc.

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.

fluttergithubbot · 2019-10-02T21:42:08Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

jmagman · 2019-10-02T21:48:25Z

Tests

I added the following tests:

plugin_lint_mac creates and runs pod lib lint for Objective-C plugins.

[plugin_lint_mac] [STDOUT] 
[plugin_lint_mac] [STDOUT] 
[plugin_lint_mac] [STDOUT] ══════════════╡ ••• Lint Objective-C iOS plugin as framework ••• ╞══════════════
[plugin_lint_mac] [STDOUT] 
[plugin_lint_mac] [STDOUT] 
[plugin_lint_mac] [STDOUT] Executing: pod lib lint /var/folders/mv/qlhc4vsj6tqct166rw3zwmth00mfq2/T/flutter_plugin_test.xD49g7/test_plugin_objc/ios/test_plugin_objc.podspec --allow-warnings
[plugin_lint_mac] [STDOUT] stdout: 
[plugin_lint_mac] [STDOUT] stdout:  -> test_plugin_objc
[plugin_lint_mac] [STDOUT] stdout:  -> test_plugin_objc (0.0.1)
[plugin_lint_mac] [STDOUT] stdout:     - WARN  | license: Missing license type.
[plugin_lint_mac] [STDOUT] stdout:     - WARN  | description: The description is equal to the summary.
[plugin_lint_mac] [STDOUT] stdout:     - WARN  | [iOS] keys: Missing primary key for `source` attribute. The acceptable ones are: `git, hg, http, svn`.
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | xcodebuild:  note: Using new build system
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | [iOS] xcodebuild:  note: Planning build
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | [iOS] xcodebuild:  note: Constructing build description
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | [iOS] xcodebuild:  ld: warning: ignoring file /Users/m/Library/Developer/Xcode/DerivedData/App-aycfmfvsahumpbfprgugltlwsvlz/Build/Products/Release-iphonesimulator/test_plugin_objc/test_plugin_objc.framework/test_plugin_objc, building for iOS Simulator-i386 but attempting to link with file built for iOS Simulator-x86_64
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | [iOS] xcodebuild:  ld: warning: ignoring file Flutter/Flutter.framework/Flutter, missing required architecture i386 in file Flutter/Flutter.framework/Flutter (3 slices)
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | [iOS] xcodebuild:  warning: Skipping code signing because the target does not have an Info.plist file and one is not being generated automatically. (in target 'App' from project 'App')
[plugin_lint_mac] [STDOUT] stdout: 
[plugin_lint_mac] [STDOUT] stdout: test_plugin_objc passed validation.
[plugin_lint_mac] [STDOUT] "pod" exit code: 0
[plugin_lint_mac] [STDOUT] 
[plugin_lint_mac] [STDOUT] 
[plugin_lint_mac] [STDOUT] ═══════════════╡ ••• Lint Objective-C iOS plugin as library ••• ╞═══════════════
[plugin_lint_mac] [STDOUT] 
[plugin_lint_mac] [STDOUT] 
[plugin_lint_mac] [STDOUT] Executing: pod lib lint /var/folders/mv/qlhc4vsj6tqct166rw3zwmth00mfq2/T/flutter_plugin_test.xD49g7/test_plugin_objc/ios/test_plugin_objc.podspec --allow-warnings --use-libraries
[plugin_lint_mac] [STDOUT] stdout: 
[plugin_lint_mac] [STDOUT] stdout:  -> test_plugin_objc
[plugin_lint_mac] [STDOUT] stdout:  -> test_plugin_objc (0.0.1)
[plugin_lint_mac] [STDOUT] stdout:     - WARN  | license: Missing license type.
[plugin_lint_mac] [STDOUT] stdout:     - WARN  | description: The description is equal to the summary.
[plugin_lint_mac] [STDOUT] stdout:     - WARN  | [iOS] keys: Missing primary key for `source` attribute. The acceptable ones are: `git, hg, http, svn`.
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | xcodebuild:  note: Using new build system
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | [iOS] xcodebuild:  note: Planning build
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | [iOS] xcodebuild:  note: Constructing build description
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | [iOS] xcodebuild:  warning: Skipping code signing because the target does not have an Info.plist file and one is not being generated automatically. (in target 'App' from project 'App')
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | [iOS] xcodebuild:  ld: warning: ignoring file /Users/m/Library/Developer/Xcode/DerivedData/App-cpheslfzecttzkbjakovdldlcjsw/Build/Products/Release-iphonesimulator/test_plugin_objc/libtest_plugin_objc.a, building for iOS Simulator-i386 but attempting to link with file built for iOS Simulator-x86_64
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | [iOS] xcodebuild:  ld: warning: ignoring file Flutter/Flutter.framework/Flutter, missing required architecture i386 in file Flutter/Flutter.framework/Flutter (3 slices)
[plugin_lint_mac] [STDOUT] stdout: 
[plugin_lint_mac] [STDOUT] stdout: test_plugin_objc passed validation.
[plugin_lint_mac] [STDOUT] "pod" exit code: 0
[plugin_lint_mac] [STDOUT] 
[plugin_lint_mac] [STDOUT]

This is CocoaPods/CocoaPods#9210. Essentially it's trying to build Release against the i386 simulator, and failing. This is caused because ONLY_ACTIVE_ARCH=NO in Release and Flutter.framework doesn't have i386 bits.

[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | [iOS] xcodebuild:  ld: warning: ignoring file /Users/m/Library/Developer/Xcode/DerivedData/App-cpheslfzecttzkbjakovdldlcjsw/Build/Products/Release-iphonesimulator/test_plugin_objc/libtest_plugin_objc.a, building for iOS Simulator-i386 but attempting to link with file built for iOS Simulator-x86_64
[plugin_lint_mac] [STDOUT] stdout:     - NOTE  | [iOS] xcodebuild:  ld: warning: ignoring file Flutter/Flutter.framework/Flutter, missing required architecture i386 in file Flutter/Flutter.framework/Flutter (3 slices)

jmagman · 2019-10-02T21:54:45Z

Also note: DEFINES_MODULE=YES by default when I make a new Obj-C or Swift framework in Xcode 11.

dnfield

LGTM

codecov · 2019-10-02T22:26:23Z

Codecov Report

Merging #41828 into master will decrease coverage by 0.83%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #41828      +/-   ##
==========================================
- Coverage   59.25%   58.41%   -0.84%     
==========================================
  Files         193      194       +1     
  Lines       18882    18934      +52     
==========================================
- Hits        11188    11061     -127     
- Misses       7694     7873     +179

Flag	Coverage Δ
#flutter_tool	`58.41% <ø> (-0.84%)`	⬇️

Impacted Files	Coverage Δ
packages/flutter_tools/lib/src/ios/xcodeproj.dart	`48% <0%> (-41.72%)`	⬇️
packages/flutter_tools/lib/src/web/web_device.dart	`7.22% <0%> (-40.97%)`	⬇️
...ackages/flutter_tools/lib/src/resident_runner.dart	`42.71% <0%> (-14.58%)`	⬇️
packages/flutter_tools/lib/src/base/utils.dart	`87.5% <0%> (-8%)`	⬇️
packages/flutter_tools/lib/src/device.dart	`57.89% <0%> (-3.51%)`	⬇️
.../flutter_tools/lib/src/runner/flutter_command.dart	`81.32% <0%> (-2.08%)`	⬇️
packages/flutter_tools/lib/src/version.dart	`91.9% <0%> (-1.43%)`	⬇️
packages/flutter_tools/lib/src/cache.dart	`45.75% <0%> (-0.67%)`	⬇️
...utter_tools/lib/src/build_runner/build_script.dart	`40.38% <0%> (ø)`
packages/flutter_tools/lib/src/base/terminal.dart	`81.48% <0%> (+3.7%)`	⬆️
... and 3 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 2a7395c...84f18ae. Read the comment docs.

zanderso

The PR description has some good info in it that sounds like it would be useful to have as comments in the code as well.

zanderso · 2019-10-03T15:31:02Z

dev/bots/test.dart

Is there an issue filed for this TODO?

It's #37525. #37519 is a really good idea.

jmagman · 2019-10-03T20:33:12Z

The PR description has some good info in it that sounds like it would be useful to have as comments in the code as well.

I'll add a comment for VALID_ARCHS[sdk=iphonesimulator*] because that is a bit weird and essentially a hack to workaround a limitation in CocoaPods.
The platform change is making it more idiomatic and DEFINES_MODULE is leveraging a documented CocoaPods/Xcode/clang feature, so they probably don't need comments.

codecov · 2019-10-03T22:15:25Z

Codecov Report

Merging #41828 into master will decrease coverage by 0.83%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #41828      +/-   ##
==========================================
- Coverage   59.25%   58.41%   -0.84%     
==========================================
  Files         193      194       +1     
  Lines       18882    18934      +52     
==========================================
- Hits        11188    11061     -127     
- Misses       7694     7873     +179

Flag	Coverage Δ
#flutter_tool	`58.41% <ø> (-0.84%)`	⬇️

Impacted Files	Coverage Δ
packages/flutter_tools/lib/src/ios/xcodeproj.dart	`48% <0%> (-41.72%)`	⬇️
packages/flutter_tools/lib/src/web/web_device.dart	`7.22% <0%> (-40.97%)`	⬇️
...ackages/flutter_tools/lib/src/resident_runner.dart	`42.71% <0%> (-14.58%)`	⬇️
packages/flutter_tools/lib/src/base/utils.dart	`87.5% <0%> (-8%)`	⬇️
packages/flutter_tools/lib/src/device.dart	`57.89% <0%> (-3.51%)`	⬇️
.../flutter_tools/lib/src/runner/flutter_command.dart	`81.32% <0%> (-2.08%)`	⬇️
packages/flutter_tools/lib/src/version.dart	`91.9% <0%> (-1.43%)`	⬇️
packages/flutter_tools/lib/src/cache.dart	`45.75% <0%> (-0.67%)`	⬇️
...utter_tools/lib/src/build_runner/build_script.dart	`40.38% <0%> (ø)`
packages/flutter_tools/lib/src/base/terminal.dart	`81.48% <0%> (+3.7%)`	⬆️
... and 3 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 2a7395c...84f18ae. Read the comment docs.

jmagman · 2019-10-04T01:35:13Z

Merging, dashboard is lying, Cirrus is really green.

Hixie · 2019-10-04T19:43:21Z

FWIW, I would rather we not add more "devicelab" tests that don't run in the device lab. It's very confusing that we have tests that claim to run in one environment but run in another.

jmagman · 2019-10-04T21:21:25Z

FWIW, I would rather we not add more "devicelab" tests that don't run in the device lab. It's very confusing that we have tests that claim to run in one environment but run in another.

Gotcha, I was keeping it consistent with module_test_ios. I can move it wherever you want.

jmagman requested review from dnfield, stuartmorgan-g and zanderso October 2, 2019 21:42

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

googlebot added the cla: yes label Oct 2, 2019

jmagman self-assigned this Oct 2, 2019

jmagman added t: xcode "xcodebuild" on iOS and general Xcode project management platform-mac Building on or for macOS specifically platform-ios iOS applications specifically labels Oct 2, 2019

dnfield approved these changes Oct 2, 2019

View reviewed changes

zanderso reviewed Oct 3, 2019

View reviewed changes

zanderso approved these changes Oct 3, 2019

View reviewed changes

Set DEFINES_MODULE=YES in plugin templates

3b16f25

Comment about i386

84f18ae

jmagman force-pushed the plugin-module branch from bf54ebb to 84f18ae Compare October 3, 2019 21:38

jmagman added the waiting for tree to go green label Oct 3, 2019

jmagman merged commit 4331c17 into flutter:master Oct 4, 2019

jmagman deleted the plugin-module branch October 4, 2019 01:35

jmagman mentioned this pull request Oct 10, 2019

Added a help page for iOS add-to-app. flutter/website#3084

Merged

jmagman mentioned this pull request Nov 9, 2019

Friction adding swift plugin to objective-c project #16049

Open

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019

Set DEFINES_MODULE=YES in plugin templates (flutter#41828)

d694054

jmagman mentioned this pull request Jan 22, 2020

Unable to run plugin from generated flutter create --template=plugin -i swift #21096

Closed

jmagman mentioned this pull request Aug 24, 2020

Change iOS plugin simulator archs from allow list to deny list #64504

Merged

10 tasks

jmagman mentioned this pull request Jul 1, 2021

When plugins do not support ARM iOS simulators, the app fails to build #85713

Closed

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

Set DEFINES_MODULE=YES in plugin templates #41828

Set DEFINES_MODULE=YES in plugin templates #41828

Uh oh!

Conversation

jmagman commented Oct 2, 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!

fluttergithubbot commented Oct 2, 2019

Uh oh!

jmagman commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Uh oh!

jmagman commented Oct 2, 2019

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

jmagman Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

jmagman commented Oct 3, 2019

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

jmagman commented Oct 4, 2019

Uh oh!

Hixie commented Oct 4, 2019

Uh oh!

jmagman commented Oct 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jmagman commented Oct 2, 2019 •

edited

Loading

jmagman commented Oct 2, 2019 •

edited

Loading

codecov bot commented Oct 2, 2019 •

edited

Loading

codecov bot commented Oct 3, 2019 •

edited

Loading