-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Move iOS Podfile logic into tool #59044
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.
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
Conversation
|
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 on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
The upcoming fix for #39659 will prompt existing projects to regenerate their Podfile like https://github.com/flutter/flutter/pull/42029/files#diff-dd627ef56fd3ab20096b1ac40467cf30R346. |
| generated_key_values | ||
| def ios_application_path | ||
| ios_application_path = File.dirname(defined_in_file.realpath) if self.respond_to?(:defined_in_file) | ||
| ios_application_path ||= File.dirname(File.realpath(__FILE__)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid __dir__, I used it in add-to-app version of this and people have had issues.
| end | ||
| generated_key_values | ||
| def ios_application_path | ||
| ios_application_path = File.dirname(defined_in_file.realpath) if self.respond_to?(:defined_in_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully CocoaPods never removes defined_in_file (the path to the Podfile) but if they do, fallback to... this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we log a warning if it doesn't repond to defined_in_file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was sensitive about this due to #42513 but we know the location of the Podfile--it's this file.
I think I'm over-engineering this a bit. I just need to pass in this file. We can keep the fallback to defined_in_file and subsequent raise in the podhelper.
| # end | ||
| # @param [XCBuildConfiguration] build_configuration Build configuration for Pod target. | ||
| def flutter_additional_ios_build_settings(build_configuration) | ||
| build_configuration.build_settings['ENABLE_BITCODE'] = 'NO' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gives us flexibility to support bitcode (someday).
| end | ||
| end | ||
|
|
||
| def flutter_parse_plugins_file(file, separator='=') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic will be updated to do #39659.
| matches = line.match(/FLUTTER_ROOT\=(.*)/) | ||
| return matches[1].strip if matches | ||
| end | ||
| raise 'FLUTTER_ROOT not found in Generated.xcconfig. Try deleting Generated.xcconfig, then run flutter pub get' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. including generated_xcode_build_settings_path in the message would be helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to include the path
[!] Invalid `Podfile` file: /Users/m/Projects/test_ruby/ios/Flutter/Generated.xcconfig must exist. If you're running pod install manually, make sure flutter pub get is executed first.
and
[!] Invalid `Podfile` file: FLUTTER_ROOT not found in /Users/m/Projects/test_ruby/ios/Flutter/Generated.xcconfig. Try deleting Generated.xcconfig, then run flutter pub get.
| end | ||
|
|
||
| # Plugin Pods | ||
| require File.expand_path(File.join('packages', 'flutter_tools', 'bin', 'podhelper'), flutter_root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\o/
| @@ -0,0 +1,126 @@ | |||
| # Copyright 2014 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the transition plan for add-to-app? Can we change this line in https://flutter.dev/docs/development/add-to-app/ios/project-setup#option-a---embed-with-cocoapods-and-the-flutter-sdk to load File.join(ENV["FLUTTER_ROOT"], '.ios', 'Flutter', 'podhelper.rb') or some such?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the CocoaPods add-to-app module lays down a similar but not quite the same podhelper in the ephemeral directory, which gets recreated on a flutter clean. As long as we didn't rename those methods and break existing host Podfiles (unfortunately the names don't reference iOS or the module: install_all_flutter_pods, install_flutter_engine_pod, install_flutter_plugin_pods) we could transition them to this bin podhelper, but I don't know what it would really buy us (other than they would get updates instantly instead on a clean). I'm concerned it would make the CI story more complicated because flutter would need to be available on every machine, even if the entire module and podspecs and binaries were already placed in the right spot.
P.S. We wouldn't want them to need FLUTTER_ROOT on their path, it would have to be parsed from the Generated files the same way this one is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, it should be a separate task.
What I had in mind actually may or may not work. One small ergonomic annoyance now is if someone checks out the iOS and flutter projects, it currently takes 3 steps to build again. Go to flutter, flutter pub get to make the .ios folder with podhelper.rb, then go to iOS, call pod install, then open xcode, run.
Perhaps we can save one step if the various flutter_install_* code called flutter pub get to make the underlying xcodeproj if needed etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be cool. Want to create a new issue?
| # target 'Runner' do | ||
| # ... | ||
| # end | ||
| def flutter_ios_podfile_setup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this hook for future use (could have been used for the old install! 'cocoapods', :disable_input_output_paths => true, for example)
| # end | ||
| # end | ||
| # @param [PBXAggregateTarget] target Pod target. | ||
| def flutter_additional_ios_build_settings(target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more change to make more future proof: pass in the target instead of the build configuration. This will give us access to the target name so in the future we can just make changes to the Flutter-specific targets (like adding a Flutter linker flag).
Description
Move Podfile logic into the tool so changes can be easily made without the user needing to migrate or regenerate their Podfile.
Related Issues
Fixes #45197
Will make #39659 easier
Tests
Add checks to plugin_lint_mac.
Checklist
///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change