-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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
Always embed iOS Flutter.framework build mode version from Xcode configuration #42029
Conversation
@@ -56,7 +56,7 @@ build/ | |||
**/ios/**/xcuserdata | |||
**/ios/.generated/ | |||
**/ios/Flutter/App.framework | |||
**/ios/Flutter/Flutter.framework | |||
**/ios/Flutter/Flutter.* |
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.
The Flutter.podspec is now getting copied in. People will have to add it to their .gitignore or check it in...
puts "Invalid plugin specification: #{line}" | ||
end | ||
end | ||
generated_key_values | ||
end | ||
|
||
target 'Runner' do | ||
use_frameworks! |
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 is the same file as Podfile-ios-obj except for use_frameworks!
.
@@ -1,23 +1,16 @@ | |||
PODS: | |||
- Flutter (1.0.0) |
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 seems to be the only Podfile.lock in the entire repo. You can see how it changes.
I don't know why MaterialControls was in there, but it doesn't seem to be a real dependency and was automatically removed?
@@ -15,49 +15,64 @@ def parse_KV_file(file, separator='=') | |||
if !File.exists? file_abs_path | |||
return []; | |||
end | |||
pods_ary = [] | |||
generated_key_values = {} |
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 is a copy of the template.
@@ -15,49 +15,64 @@ def parse_KV_file(file, separator='=') | |||
if !File.exists? file_abs_path | |||
return []; | |||
end | |||
pods_ary = [] | |||
generated_key_values = {} |
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.
Copy of the template.
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.
Do these mean this will break for consumers?
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.
No, the original format of the Generated.xcconfig is the same, and this method isn't accessible outside this file (unless someone was doing some really weird hackery). The method return type is different now, but I changed the two places we call it.
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.
Sorry, I mean the fact that you had to update all of these - will it not be automatic for people running Flutter on existing projects?
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.
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.
P.S. The stale framework issue this is fixing happens when developers fail to follow the deployment instructions and do not run flutter build ios
in release before archiving. So if the Podfile isn't updated, but they follow instructions, they will be fine.
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.
Maybe we could read the podfile an dlook for the the offending line?
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.
A more detailed message, or a link to some docs on the website, could help folks do the necessary update manually if they have local edits they want to keep. Like, "Your Podfile is out of date. Regenerate it with .... If you have local edits that you'd like to keep, see ... for instructions."
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.
It's easy to see if their Podfile is out of date without parsing (either .symlinks/flutter
exists or it doesn't since that directory gets removed and recreated each pod install
run).
The minimum Podfile change (without the mapping refactor) is pretty large, so it's not just a one-line swap:
generated_xcode_build_settings = parse_KV_file('./Flutter/Generated.xcconfig')
if generated_xcode_build_settings.empty?
puts "Generated.xcconfig must exist. If you're running pod install manually, make sure flutter pub get is executed first."
end
generated_xcode_build_settings.map { |p|
if p[:name] == 'FLUTTER_FRAMEWORK_DIR'
symlink = File.join('.symlinks', 'flutter')
File.symlink(File.dirname(p[:path]), symlink)
pod 'Flutter', :path => File.join(symlink, File.basename(p[:path]))
end
}
becomes:
copied_flutter_dir = File.join(__dir__, 'Flutter')
copied_framework_path = File.join(copied_flutter_dir, 'Flutter.framework')
copied_podspec_path = File.join(copied_flutter_dir, 'Flutter.podspec')
unless File.exist?(copied_framework_path) && File.exist?(copied_podspec_path)
generated_xcode_build_settings = parse_KV_file('./Flutter/Generated.xcconfig')
if generated_xcode_build_settings.empty?
puts "Generated.xcconfig must exist. If you're running pod install manually, make sure flutter pub get is executed first."
end
generated_xcode_build_settings = parse_KV_file(generated_xcode_build_settings_path)
generated_xcode_build_settings.map { |p|
if p[:name] == 'FLUTTER_FRAMEWORK_DIR'
cached_framework_dir = p[:path]
unless File.exist?(copied_framework_path)
FileUtils.cp_r(File.join(cached_framework_dir, 'Flutter.framework'), copied_flutter_dir)
end
unless File.exist?(copied_podspec_path)
FileUtils.cp(File.join(cached_framework_dir, 'Flutter.podspec'), copied_flutter_dir)
end
end
}
end
pod 'Flutter', :path => 'Flutter'
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.
@@ -4,37 +4,75 @@ | |||
# CocoaPods analytics sends network stats synchronously affecting flutter build latency. | |||
ENV['COCOAPODS_DISABLE_STATS'] = 'true' | |||
|
|||
def parse_KV_file(file,separator='=') | |||
project 'Runner', { |
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.
Copy of the template.
@@ -4,56 +4,75 @@ | |||
# CocoaPods analytics sends network stats synchronously affecting flutter build latency. | |||
ENV['COCOAPODS_DISABLE_STATS'] = 'true' | |||
|
|||
project 'Runner', { |
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.
Copy of the template.
puts "Invalid plugin specification: #{line}" | ||
end | ||
end | ||
generated_key_values |
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.
Return a map of keys => values instead of an array of single key => value maps.
|
||
generated_xcode_build_settings_path = File.join(copied_flutter_dir, 'Generated.xcconfig') | ||
unless File.exist?(generated_xcode_build_settings_path) | ||
raise "Generated.xcconfig must exist. If you're running pod install manually, make sure flutter pub get is executed first" |
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.
unless File.exist?(copied_framework_path) | ||
FileUtils.cp_r(File.join(cached_framework_dir, 'Flutter.framework'), copied_flutter_dir) | ||
end | ||
unless File.exist?(copied_podspec_path) |
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 is essentially a migration step since people may already have ios/Flutter/Flutter.framework
but will definitely not have ios/Flutter/Flutter.podspec
.
File.symlink(p[:path], symlink) | ||
pod p[:name], :path => File.join(symlink, 'ios') | ||
} | ||
plugin_pods.each do |name, path| |
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 is more idiomatic Ruby.
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'll take your word for it :)
Codecov Report
@@ Coverage Diff @@
## master #42029 +/- ##
==========================================
- Coverage 60.63% 60.13% -0.51%
==========================================
Files 194 194
Lines 18886 18897 +11
==========================================
- Hits 11452 11364 -88
- Misses 7434 7533 +99
Continue to review full report at Codecov.
|
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 test are we missing that would have caught this? I suppose since the devicelab is building through flutter it wouldn't have tickled this bug at all?
|
||
generated_xcode_build_settings_path = File.join(copied_flutter_dir, 'Generated.xcconfig') | ||
unless File.exist?(generated_xcode_build_settings_path) | ||
raise "Generated.xcconfig must exist. If you're running pod install manually, make sure flutter pub get is executed first" |
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.
flutter packages get
or are we rebranding to flutter pub get
? Both probably work, not sure if we're consistent with one or the other.
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 didn't change the text of this line, I made it a raise
instead of a puts
. I feel like I've seen flutter packages get
in more places, so I'll change it.
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.
Never mind, flutter packages get
is only used in the Podfile-macos (which I didn't change in this PR because I want to test that thoroughly and separately). flutter pub get
is used in 26 places in the repo.
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.
File.symlink(p[:path], symlink) | ||
pod p[:name], :path => File.join(symlink, 'ios') | ||
} | ||
plugin_pods.each do |name, path| |
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'll take your word for it :)
Something like: flutter/dev/bots/deploy_gallery.sh Line 56 in 054e887
but builds with --debug that deploys (uses Release) and then does a smoke test of the archived app.
I'll think more about how to test this... |
For a test, couldn't we take one of the integration tests we have and assert that the .symlinks folder doesn't have Flutter in it? |
5671d55
to
f886874
Compare
What is the status of this PR? |
I added a few location checks to deploy_gallery. |
Description
There were two links/copies of Flutter.framework inside
my_flutter_app/ios/
. One was symlinked from the cache duringpod install
and embedded by CocoaPods:The location of the framework was pulled from FLUTTER_FRAMEWORK_DIR which was only updated from a command line
flutter build
. This caused issues when a user runsflutter build --debug
but then immediately tries to archive their app from Xcode. The FLUTTER_FRAMEWORK_DIR would still point to the Debug version even though the Xcode build config was Release.The second version is copied during xcode_backend.sh. This used the Xcode build configuration to decide which version of Flutter.framework to use. The script runs on every Xcode build from the command line or from Xcode itself, and correctly (assuming schemes are well-named) chooses the right build mode version of the framework.
The CocoaPods version always gets the last word since it is embedded last after xcode_backend runs. Users experience various production blank screens/crashes/app submission rejections due to accidentally submitting the debug version of their engine.
This change removes the symlinked version and points to the xcode_backend location. It pulls a similar trick as podhelper.rb and copies the FLUTTER_FRAMEWORK_DIR version of the engine to the correct place if xcode_backend has not yet run. This fall-back lets
pod install
succeed, and the correct version of the framework is always copied over by xcode_backend on every Xcode build/test/archive.This has the side effect of no longer changing the Podfile.lock (which should be checked into source control) every time the build mode changes.
becomes
Additionally, the Ruby code was updated to make it more idiomatic.
Related Issues
Fixes #24641
Fixes #25167
Fixes #25370
Fixes #28802
Some of these issues have become muddied so it will fix some people chiming in but not others:
#22765
#24887
#37850
Related:
#36247
Tests
Added a few file location checks to deploy_gallery. It will also be exercised by other integration tests that build or run on iOS.
Checklist
///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change