Skip to content

Set DEFINES_MODULE for FlutterPluginRegistrant to generate modulemap #40302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jmagman merged 2 commits into from
Sep 13, 2019
Merged

Set DEFINES_MODULE for FlutterPluginRegistrant to generate modulemap #40302

jmagman merged 2 commits into from
Sep 13, 2019

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Sep 12, 2019

Description

Let CocoaPods generate a module map for FlutterPluginRegistrant so it can be imported by existing Swift apps that don't use_frameworks! as a static library.

Bonus: existing Objective-C apps can use @import FlutterPluginRegistrant syntax and any existing imports or includes will become module imports and leverage the module cache.

See http://blog.cocoapods.org/CocoaPods-1.5.0/.

Related Issues

Fixes #40289.
Gives a workaround to #31104 with Swift apps importing static libraries (don't use use_frameworks! in the Podfile).

Tests

I added a Swift app to the module_test_ios. They fail before without the DEFINES_MODULE change:

[module_test_ios] [STDOUT] stdout: import FlutterPluginRegistrant
[module_test_ios] [STDOUT] stdout:        ^
[module_test_ios] [STDOUT] stdout: 
[module_test_ios] [STDOUT] stderr: ** BUILD FAILED **

And pass with the DEFINES_MODULE change.

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 added tool Affects the "flutter" command-line tool. See also t: labels. a: existing-apps Integration with existing apps via the add-to-app flow labels Sep 12, 2019
@jmagman jmagman requested a review from xster September 12, 2019 01:19
@jmagman jmagman self-assigned this Sep 12, 2019
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Sep 12, 2019

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.

@fluttergithubbot fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Sep 12, 2019
jmagman
jmagman commented Sep 12, 2019
View reviewed changes
packages/flutter_tools/lib/src/plugins.dart Outdated
Copy link
Member Author

@jmagman jmagman Sep 12, 2019

Choose a reason for hiding this comment

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

This is the actual change, everything else is tests.

@codecov
Copy link

codecov bot commented Sep 12, 2019 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #40302      +/-   ##
==========================================
- Coverage   59.42%   59.21%   -0.21%     
==========================================
  Files         192      192              
  Lines       18602    18602              
==========================================
- Hits        11055    11016      -39     
- Misses       7547     7586      +39
Flag Coverage Δ
#flutter_tool 59.21% <ø> (-0.21%) ⬇️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/plugins.dart 78.08% <ø> (ø) ⬆️
...lutter_tools/lib/src/commands/build_appbundle.dart 90.32% <0%> (-9.68%) ⬇️
...ckages/flutter_tools/lib/src/flutter_manifest.dart 79.36% <0%> (-5.83%) ⬇️
packages/flutter_tools/lib/src/asset.dart 79.42% <0%> (-4.53%) ⬇️
...ackages/flutter_tools/lib/src/resident_runner.dart 52.9% <0%> (-3.81%) ⬇️
packages/flutter_tools/lib/src/run_hot.dart 65.51% <0%> (-3.45%) ⬇️
packages/flutter_tools/lib/src/version.dart 90.99% <0%> (-1.9%) ⬇️
packages/flutter_tools/lib/src/cache.dart 43.5% <0%> (-0.73%) ⬇️
packages/flutter_tools/lib/src/base/utils.dart 93.56% <0%> (-0.5%) ⬇️
.../flutter_tools/lib/src/runner/flutter_command.dart 80.51% <0%> (-0.44%) ⬇️
... and 10 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 ba13aa9...6920069. Read the comment docs.

This was referenced Sep 12, 2019
Closed
Merged
xster
xster reviewed Sep 13, 2019
View reviewed changes
dev/integration_tests/ios_host_app_swift/Host/ViewController.swift Outdated
Copy link
Member

@xster xster Sep 13, 2019

Choose a reason for hiding this comment

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

This file is built but nothing really runs it currently right? If so, add a comment. People looking at projects in a 'integration_tests' folder might expect this to be test harnessed somewhere.

Copy link
Member Author

@jmagman jmagman Sep 13, 2019

Choose a reason for hiding this comment

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

Sure, I was copying the same behavior from the iOS ios_host_app. I can add the comment there, too.

@xster
Copy link
Member

xster commented Sep 13, 2019

Nice! Thanks for discovering this. LGTM!

@jmagman jmagman merged commit b80b9be into flutter:master Sep 13, 2019
@jmagman jmagman deleted the modular-headers branch September 13, 2019 18:06
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
@jmagman @Inconnu08
Set DEFINES_MODULE for FlutterPluginRegistrant to generate modulemap (f…
2024b12 
…lutter#40302)
@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

Assignees

@jmagman jmagman

Labels

a: existing-apps Integration with existing apps via the add-to-app flow 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.

Support adding Objective-C plugins as static libraries to existing Swift app with CocoaPods

4 participants

@jmagman @fluttergithubbot @xster @googlebot