Move plugin injection to just after pub get #14560
Conversation
mravn-google
force-pushed
the
dep_inj
branch
from
ad41d05 to
436e2ef
Compare
mravn-google
changed the title
mravn-google
force-pushed
the
dep_inj
branch
2 times, most recently
from
7a0fde0 to
645b251
Compare
mravn-google
force-pushed
the
dep_inj
branch
from
645b251 to
582a112
Compare
|
@jakobr-google, @xster This PR is ready for code review. |
|
|
||
| // Check if you need to run pod install. | ||
| // The pod install will run if any of below is true. | ||
| // 1. Any plugins changed (add/update/delete) |
| bool flutterPodChanged: true, | ||
| }) async { | ||
| if (await _checkPodCondition()) { | ||
| if (!fs.file(fs.path.join(appIosDir.path, 'Podfile')).existsSync()) { |
There was a problem hiding this comment.
I'm assuming this implies we've built in some knowledge here that flutter run will necessarily run flutter packages get before and will never have cases where pod install is run in a project that doesn't have a Podfile?
There was a problem hiding this comment.
That's the idea. flutter run already invokes flutter packages get if your pubspec.yaml file has changed relative to your .packages and/or .pubspec.lock. So you'll get a Podfile if your project needs it and you don't already have one; it just isn't done by our buildXcodeProject code path.
I guess I should add checks for those assumptions here.
There was a problem hiding this comment.
A check would be good to proof against future refactors by maintainers unaware of this contract.
| if (plugins.isNotEmpty) | ||
| const CocoaPods().setupPodfile(directory); | ||
| if (changed) | ||
| _ensurePodInstallIsExecutedOnNextIOSBuild(directory); |
There was a problem hiding this comment.
Based on your PR description, wouldn't you want to run pod install here too?
i.e. is the desired outcome that after flutter packages get, the user can just open Xcode and press run?
There was a problem hiding this comment.
Can we also move this into the cocoapods.dart? To keep the encapsulation and discoverability clearer.
There was a problem hiding this comment.
The intention of this PR is that flutter packages get brings the project into a state where platform-specific tooling works without flutter xxx help.
For Android that means Gradle builds work. Gradle nicely does both dependency resolution of non-pub packages and actual building.
The directly corresponding iOS situation is that Cocoapods+Xcode work. Doing pod install as part of flutter packages get would break this symmetry. That may be just an academic point, but we seem to have gone to great lengths to avoid doing pod install until there is no way around it (I assume because the pods repo is painful to download). If the user just wants to run her Flutter project on Android, pod install is not needed. We don't know the user's intention at flutter packages get time.
There was a problem hiding this comment.
Move into cocoapods.dart: good point, done.
| projectPath: fs.currentDirectory.path, | ||
| buildInfo: buildInfo, | ||
| target: target, | ||
| hasPlugins: hasFlutterPlugins, |
There was a problem hiding this comment.
Depending on if I understood the PR description intent correctly, it sounds like you want to move this call to updateXcodeGeneratedProperties to flutter packages get in a future PR too?
There was a problem hiding this comment.
I think you are right. I left that out because 1) it was not necessary on a newly created project, and 2) the properties include information like the build mode which is not available when we run flutter packages get.
But 1) is because flutter create generates the Generated.xcconfig file with some default settings along with a .gitignore entry that causes it not to be present on subsequent git checkout. And for 2) those default settings can be used during flutter packages get too.
|
Should we add some new gitignores too? |
|
@xster Thanks for the review. I've restructured some of the code to make the intention clearer. I also worked on improving test coverage. PTAL and let me know whether your concerns have been addressed by the changes and the comments above. |
This reverts commit b3e4976.
…lutter#14734) This reverts commit b3e4976.
4 participants
Move injection of Flutter plugins from just before build to just after getting/updating pub packages.
Plugin injection here means
.flutter-pluginsfileGeneratedPluginRegistrant.(java|h|m)filesPodfile, if it doesn't exit#include "Pods/Target Support Files/Pods-Runner/Pods-Runner.xxx.xcconfig"toios/Flutter/Xxx.xcconfigfiles (cf. Emit CocoaPods #include in Debug.xcconfig, Release.xcconfig #12752).The intention is to get us to a situation where upon initial project checkout or change to
pubspec.yaml, the only step needed to get to a project buildable by platform-specific tooling isflutter packages get.Discrepancies between
pubspec.yamland injected plugin files have caused usability problems (#11797). And we have been forced to check in the injected files as if they were source code (#13554) to make our project examples work out of the box.Fixes #12752
Fixes #11797
Fixes #13554