-
Notifications
You must be signed in to change notification settings - Fork 28.9k
In xcode_backend.sh, build flutter assemble arguments as an array of quoted strings #86534
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
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.
Does this fail on master and pass on this PR if you change this:
flutter/packages/flutter_tools/test/integration.shard/ios_content_validation_test.dart
Line 69 in 72ae6e2
'--split-debug-info=foo/', |
to:
'--split-debug-info=foo with space/',
Adding a --dart-define=
with spaces would also be a way to test this (and expand coverage).
macOS has the same issue:
https://github.com/flutter/flutter/blob/master/packages/flutter_tools/bin/macos_assemble.sh
tested in:
flutter/packages/flutter_tools/test/integration.shard/macos_content_validation_test.dart
Line 47 in 72ae6e2
final List<String> buildCommand = <String>[ |
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.
Huh I'm surprised nothing complained about checking in those app_icon_*.png binaries.
They're there in the |
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.
Huh I'm surprised nothing complained about checking in those app_icon_*.png binaries.
They're there in the
ios/
subdir as well.
I noticed that, I was surprised by that too!
The build test on the new macos project passed:
▌16:45:03▐ RUNNING: cd examples/hello_world; ../../bin/flutter build macos --debug -v
expect(result.exitCode, 0); | ||
}, skip: true); // Extremely flaky due to https://github.com/flutter/flutter/issues/68144 |
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.
👍
Prior to this PR, some arguments such as
--sksl-bundle-path
are passed toflutter assemble
without quotes such that a path with a space in it would be interpreted as two separate command line arguments, leading to various errors.This PR assembles the argument array as a bash array of quoted strings, which makes it easier to conditionally add some arguments while ensuring that they are properly quoted.
This will help the benchmarks added in #86460 get closer to running.