Makes scheme and target optional parameter when getting universal lin… #134571

chunhtai · 2023-09-12T19:28:45Z

…k settings

the show build settings xcode command can only accept one of the target or scheme flag. Therefore I make them optional.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…k settings

chunhtai · 2023-09-12T19:32:56Z

packages/flutter_tools/lib/src/xcode_project.dart

@@ -221,9 +222,12 @@ class IosProject extends XcodeBasedProject {
  /// The return future will resolve to string path to the output file.
  Future<String> outputsUniversalLinkSettings({
    required String configuration,
-    required String scheme,
-    required String target,
+    String? scheme,


Actually I am not sure if we should support scheme at all. My understanding of scheme is that it contains a set of targets. I am not sure what does it mean to get build settings for a scheme.

@andrewkolos can you review this PR? I'm still fuzzy on the relationship between targets and schemes.

Hah, I hardly know the difference myself, but—no worries—I'll take a look.

As I understand, projects and targets can have Build Settings, while schemes can have a Build Configuration. Settings are applied in this order: project settings, build configuration settings, then target settings¹.

Footnotes

https://developer.apple.com/documentation/xcode/adding-a-build-configuration-file-to-your-project#Map-build-settings-to-a-build-configuration ↩

the show build settings xcode command can only accept one of the target or scheme flag. Therefore I make them optional.

Running xcodebuild -scheme iosapp -configuration jk -target iosapp -showBuildSettings in one of my local projects doesn't result in any error. Are you sure that the command only accepts exactly one of -scheme and -target?

So getBuildSettings specifies -project, which if -project is specified, you can only use either -scheme or -target. However, if you were to exclude the -project flag and use -scheme and -target together, it basically just ignores the -target flag.

So @andrewkolos xcodebuild -scheme iosapp -configuration jk -target iosapp -showBuildSettings works because -project is not specified.

Also, Build Configuration is not per scheme but per project, although in flutter tooling we use the scheme to determine the configuration for flavors.

When -scheme is specified, it shows the build settings for all runnable targets. This can be seen if you edit the scheme and check the Run column for more than one targets.

So if I ran xcodebuild -project xxx -configuration xxx -scheme Runner -showBuildSettings, it would show something like:

Build settings for action build and target Runner: ... Build settings for action build and target RunnerTests: ...

When -target is set, it will only show the build settings for only the set target.

Looking at our code, seems like we don't really handle multiple targets per scheme. It operates under the assumption that there's only one target per scheme.

I think there are improvements that could be made here, but probably out of scope for this PR. I filed an issue to follow up on that: #134669

For your PR, since -configuration is required, I think it's okay to only use -target.

If -configuration was not set, you would have needed -scheme to get the correct -configuration for flavors.

Sounds good, thanks for explanation. I will update the code.

andrewkolos

I added some comments. I will do a full review soon.

andrewkolos · 2023-09-12T19:54:07Z

packages/flutter_tools/lib/src/commands/analyze.dart

+          'The "--configuration", and one of "--scheme" or "--target" must also '
+          'be set.',


nit: grammar

Suggested change

'The "--configuration", and one of "--scheme" or "--target" must also '

'be set.',

'The "--configuration" and either "--scheme" or "--target" must also '

'be set.',

On second thought, "one of" is fine too. Feel free to ignore.

andrewkolos · 2023-09-12T20:19:27Z

packages/flutter_tools/lib/src/xcode_project.dart

@@ -221,9 +222,12 @@ class IosProject extends XcodeBasedProject {
  /// The return future will resolve to string path to the output file.
  Future<String> outputsUniversalLinkSettings({
    required String configuration,
-    required String scheme,
-    required String target,
+    String? scheme,


As I understand, projects and targets can have Build Settings, while schemes can have a Build Configuration. Settings are applied in this order: project settings, build configuration settings, then target settings¹.

Footnotes

https://developer.apple.com/documentation/xcode/adding-a-build-configuration-file-to-your-project#Map-build-settings-to-a-build-configuration ↩

chunhtai · 2023-09-13T18:46:57Z

I completely removed scheme from ios analyzer and related API. PTAL :)

vashworth

LGTM with nits

packages/flutter_tools/lib/src/commands/analyze.dart

packages/flutter_tools/lib/src/commands/ios_analyze.dart

Co-authored-by: Victoria Ashworth <15619084+vashworth@users.noreply.github.com>

auto-submit · 2023-09-14T19:32:37Z

auto label is removed for flutter/flutter/134571, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

…versal lin… (flutter/flutter#134571)

flutter/flutter@72b69f9...e5e36ad 2023-09-16 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from 326faf1762d6 to 30b7e9ded7a0 (5 revisions) (flutter/flutter#134876) 2023-09-16 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from 51e643de62aa to 326faf1762d6 (6 revisions) (flutter/flutter#134875) 2023-09-16 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from d623ecf43c66 to 51e643de62aa (5 revisions) (flutter/flutter#134865) 2023-09-16 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from c0eaf2633686 to d623ecf43c66 (5 revisions) (flutter/flutter#134861) 2023-09-16 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from 5aa9db365ed6 to c0eaf2633686 (5 revisions) (flutter/flutter#134860) 2023-09-16 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from 67dd12f8dfca to 5aa9db365ed6 (5 revisions) (flutter/flutter#134856) 2023-09-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from 45bc4307cda3 to 67dd12f8dfca (6 revisions) (flutter/flutter#134791) 2023-09-15 47866232+chunhtai@users.noreply.github.com Makes scheme and target optional parameter when getting universal linâ�¦ (flutter/flutter#134571) 2023-09-15 polinach@google.com Dispose layers in test. (flutter/flutter#134802) 2023-09-15 31859944+LongCatIsLooong@users.noreply.github.com [Windows_android channels_integration_test] Column -> ListView (flutter/flutter#134836) 2023-09-15 30870216+gaaclarke@users.noreply.github.com moved hello_world_impeller to a 7pro (flutter/flutter#134830) 2023-09-15 dacoharkes@google.com Speed up native assets target (flutter/flutter#134523) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

flutter#134571) â�¦k settings the show build settings xcode command can only accept one of the target or scheme flag. Therefore I make them optional.

…versal lin… (flutter/flutter#134571)

…r#4938) flutter/flutter@72b69f9...e5e36ad 2023-09-16 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from 326faf1762d6 to 30b7e9ded7a0 (5 revisions) (flutter/flutter#134876) 2023-09-16 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from 51e643de62aa to 326faf1762d6 (6 revisions) (flutter/flutter#134875) 2023-09-16 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from d623ecf43c66 to 51e643de62aa (5 revisions) (flutter/flutter#134865) 2023-09-16 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from c0eaf2633686 to d623ecf43c66 (5 revisions) (flutter/flutter#134861) 2023-09-16 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from 5aa9db365ed6 to c0eaf2633686 (5 revisions) (flutter/flutter#134860) 2023-09-16 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from 67dd12f8dfca to 5aa9db365ed6 (5 revisions) (flutter/flutter#134856) 2023-09-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from 45bc4307cda3 to 67dd12f8dfca (6 revisions) (flutter/flutter#134791) 2023-09-15 47866232+chunhtai@users.noreply.github.com Makes scheme and target optional parameter when getting universal linâ�¦ (flutter/flutter#134571) 2023-09-15 polinach@google.com Dispose layers in test. (flutter/flutter#134802) 2023-09-15 31859944+LongCatIsLooong@users.noreply.github.com [Windows_android channels_integration_test] Column -> ListView (flutter/flutter#134836) 2023-09-15 30870216+gaaclarke@users.noreply.github.com moved hello_world_impeller to a 7pro (flutter/flutter#134830) 2023-09-15 dacoharkes@google.com Speed up native assets target (flutter/flutter#134523) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Makes scheme and target optional parameter when getting universal lin…

9b7a1fd

…k settings

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 12, 2023

chunhtai commented Sep 12, 2023

View reviewed changes

chunhtai requested review from christopherfujino and vashworth September 12, 2023 19:44

christopherfujino requested review from andrewkolos and removed request for christopherfujino September 12, 2023 19:46

andrewkolos reviewed Sep 12, 2023

View reviewed changes

andrewkolos self-requested a review September 13, 2023 04:12

vashworth mentioned this pull request Sep 13, 2023

Reevaluate getting and parsing of iOS/macOS build settings #134669

Open

remove schemes from analyze API

50c6a83

vashworth approved these changes Sep 14, 2023

View reviewed changes

packages/flutter_tools/lib/src/commands/analyze.dart Outdated Show resolved Hide resolved

packages/flutter_tools/lib/src/commands/ios_analyze.dart Outdated Show resolved Hide resolved

Apply suggestions from code review

8c353e5

Co-authored-by: Victoria Ashworth <15619084+vashworth@users.noreply.github.com>

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 14, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 14, 2023

Merge branch 'master' into xcode-optional

4264323

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 15, 2023

auto-submit bot merged commit 367203b into flutter:master Sep 15, 2023
119 checks passed

engine-flutter-autoroll mentioned this pull request Sep 16, 2023

Roll Flutter from 72b69f944915 to b8dc118b7f55 (10 revisions) flutter/packages#4936

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 16, 2023

367203b30 Makes scheme and target optional parameter when getting uni…

6201bd6

…versal lin… (flutter/flutter#134571)

engine-flutter-autoroll mentioned this pull request Sep 16, 2023

Roll Flutter from 72b69f944915 to e1428bffd464 (11 revisions) flutter/packages#4937

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 16, 2023

367203b30 Makes scheme and target optional parameter when getting uni…

c6feabd

…versal lin… (flutter/flutter#134571)

engine-flutter-autoroll mentioned this pull request Sep 16, 2023

Roll Flutter from 72b69f944915 to e5e36ad34058 (12 revisions) flutter/packages#4938

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 16, 2023

367203b30 Makes scheme and target optional parameter when getting uni…

c977966

…versal lin… (flutter/flutter#134571)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023

367203b30 Makes scheme and target optional parameter when getting uni…

817ba8a

…versal lin… (flutter/flutter#134571)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes scheme and target optional parameter when getting universal lin… #134571

Makes scheme and target optional parameter when getting universal lin… #134571

chunhtai commented Sep 12, 2023

chunhtai Sep 12, 2023

christopherfujino Sep 12, 2023

andrewkolos Sep 12, 2023

andrewkolos Sep 12, 2023

andrewkolos Sep 13, 2023

vashworth Sep 13, 2023 •

edited

vashworth Sep 13, 2023 •

edited

chunhtai Sep 13, 2023

andrewkolos left a comment

andrewkolos Sep 12, 2023

andrewkolos Sep 12, 2023

andrewkolos Sep 12, 2023

chunhtai commented Sep 13, 2023

vashworth left a comment

auto-submit bot commented Sep 14, 2023

		'The "--configuration", and one of "--scheme" or "--target" must also '
		'be set.',

Makes scheme and target optional parameter when getting universal lin… #134571

Makes scheme and target optional parameter when getting universal lin… #134571

Conversation

chunhtai commented Sep 12, 2023

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

vashworth Sep 13, 2023 • edited

Choose a reason for hiding this comment

vashworth Sep 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkolos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

chunhtai commented Sep 13, 2023

vashworth left a comment

Choose a reason for hiding this comment

auto-submit bot commented Sep 14, 2023

vashworth Sep 13, 2023 •

edited

vashworth Sep 13, 2023 •

edited