Skip to content

Add use_modular_headers! to default Podfile #42204

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 1 commit into from
Oct 15, 2019
Merged

Add use_modular_headers! to default Podfile #42204

jmagman merged 1 commit into from
Oct 15, 2019

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Oct 8, 2019 •
edited
Loading

Description

  • Enable strict search paths and module map generation for pods by adding use_modular_headers! to the default Podfile. Hopefully this lets Swift remove use_frameworks! and build as libraries in the future (soon?). Compilation times will be faster because clang can use the module cache instead of doing pre-processing work. See http://blog.cocoapods.org/CocoaPods-1.5.0/ for how this works.
  • While I was looking at the generated code I added NS_ASSUME_NONNULL_BEGIN to get a compilation warning on a nil registry since the registration methods it calls require nonnull.
  • I wanted to remove the #ifndef GeneratedPluginRegistrant_h in the same spot but I realized we couldn't because the template was using #include instead of #import. Migrate them over to avoid including headers multiple times.

Related Issues

See #41007 for what supporting modules will unlock.

Tests

I updated the integration test Podfiles.

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

  • 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 platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. t: xcode "xcodebuild" on iOS and general Xcode project management labels Oct 8, 2019
@jmagman jmagman requested review from cbracken and dnfield October 8, 2019 03:18
@jmagman jmagman self-assigned this Oct 8, 2019
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Oct 8, 2019

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

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

@fluttergithubbot fluttergithubbot added d: examples Sample code and demos c: contributor-productivity Team-specific productivity, code health, technical debt. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. labels Oct 8, 2019
@codecov
Copy link

codecov bot commented Oct 8, 2019 •
edited
Loading

Codecov Report

Merging #42204 into master will increase coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #42204      +/-   ##
==========================================
+ Coverage   59.93%   60.11%   +0.18%     
==========================================
  Files         194      194              
  Lines       18877    18877              
==========================================
+ Hits        11313    11348      +35     
+ Misses       7564     7529      -35
Flag Coverage Δ
#flutter_tool 60.11% <ø> (+0.18%) ⬆️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/plugins.dart 85.96% <ø> (-0.88%) ⬇️
...ages/flutter_tools/lib/src/commands/build_web.dart 29.41% <0%> (-64.71%) ⬇️
packages/flutter_tools/lib/src/web/compile.dart 10.34% <0%> (-62.07%) ⬇️
packages/flutter_tools/lib/src/version.dart 75.84% <0%> (-19.33%) ⬇️
...flutter_tools/lib/src/android/android_builder.dart 57.14% <0%> (-17.86%) ⬇️
packages/flutter_tools/lib/src/features.dart 80.48% <0%> (-17.08%) ⬇️
packages/flutter_tools/lib/src/base/io.dart 59.45% <0%> (-5.41%) ⬇️
...ools/lib/src/build_runner/resident_web_runner.dart 76.16% <0%> (-2.91%) ⬇️
...r_tools/lib/src/runner/flutter_command_runner.dart 70.46% <0%> (-2.54%) ⬇️
.../flutter_tools/lib/src/runner/flutter_command.dart 83.19% <0%> (-1.27%) ⬇️
... and 17 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 163be41...893d28f. Read the comment docs.

@jmagman
Copy link
Member Author

jmagman commented Oct 8, 2019 •
edited
Loading

Wow this is a nifty and helpful analyzer warning!

The following GeneratedPluginRegistrants are out of date:
 - /tmp/flutter sdk/examples/hello_world/ios/Runner/GeneratedPluginRegistrant.h

Very few Generated files are actually checked in. 🤔
#13541

dev/manual_tests/macos/Flutter/GeneratedPluginRegistrant.swift
examples/hello_world/ios/Runner/GeneratedPluginRegistrant.h
examples/hello_world/ios/Runner/GeneratedPluginRegistrant.m

@jmagman jmagman changed the title Add use_modular_headers! to default Podfile WIP Add use_modular_headers! to default Podfile Oct 8, 2019
jmagman
jmagman commented Oct 10, 2019
View reviewed changes
packages/flutter_tools/lib/src/plugins.dart Outdated
Copy link
Member Author

@jmagman jmagman Oct 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Swift plugins should not need to add an Objective-C header if they don't need it.

@jmagman jmagman mentioned this pull request Oct 10, 2019
Merged
@jmagman jmagman changed the title WIP Add use_modular_headers! to default Podfile Add use_modular_headers! to default Podfile Oct 14, 2019
@jmagman jmagman marked this pull request as ready for review October 14, 2019 21:55
christopherfujino
christopherfujino approved these changes Oct 14, 2019
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.

weak LGTM, given I had to stack overflow search most of what was going on :)

@jmagman jmagman merged commit 649cf82 into flutter:master Oct 15, 2019
@jmagman jmagman deleted the modular-headers branch October 15, 2019 19:37
@jmagman jmagman mentioned this pull request Oct 16, 2019
Merged
9 tasks
@jmagman jmagman mentioned this pull request Nov 4, 2019
Merged
9 tasks
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@jmagman @Inconnu08
use_modular_headers (flutter#42204)
eede768
@jmagman jmagman mentioned this pull request Jan 22, 2020
Closed
@jmagman jmagman mentioned this pull request Feb 26, 2020
Merged
10 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cbracken cbracken Awaiting requested review from cbracken

@dnfield dnfield Awaiting requested review from dnfield

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. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. d: examples Sample code and demos platform-ios iOS applications specifically t: xcode "xcodebuild" on iOS and general Xcode project management 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.

4 participants

@jmagman @fluttergithubbot @christopherfujino @googlebot