Add more supported simulator debugging options and improve tests #114628

vashworth · 2022-11-03T17:26:54Z

Add more supported simulator debugging options update tests. Also, add tests for other launch arguments.

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.

…sts for other launch arguements

vashworth · 2022-11-03T22:22:09Z

Discussed with @jmagman about improving this code by using a single function that returns the list of launch arguments that can then be used for both the iOS simulator and physical iOS. The upside of this would be less repeated code, and if someone wants to add a new flag in the future, they can add to just one place and won't have to remember to add to both simulators.dart and devices.dart.

vashworth · 2022-11-03T22:28:04Z

Also discussed how the simulator version only adds the flags if the debuggingOptions.debuggingEnabled is true

flutter/packages/flutter_tools/lib/src/ios/simulators.dart

Lines 432 to 448 in d40f6d1

    
           if (debuggingOptions.debuggingEnabled) ...<String>[ 
        
             if (debuggingOptions.buildInfo.isDebug) ...<String>[ 
        
               '--enable-checked-mode', 
        
               '--verify-entry-points', 
        
             ], 
        
             if (debuggingOptions.enableSoftwareRendering) '--enable-software-rendering', 
        
             if (debuggingOptions.startPaused) '--start-paused', 
        
             if (debuggingOptions.disableServiceAuthCodes) '--disable-service-auth-codes', 
        
             if (debuggingOptions.skiaDeterministicRendering) '--skia-deterministic-rendering', 
        
             if (debuggingOptions.useTestFonts) '--use-test-fonts', 
        
             if (debuggingOptions.traceAllowlist != null) '--trace-allowlist="${debuggingOptions.traceAllowlist}"', 
        
             if (debuggingOptions.traceSkiaAllowlist != null) '--trace-skia-allowlist="${debuggingOptions.traceSkiaAllowlist}"', 
        
             if (dartVmFlags.isNotEmpty) '--dart-flags=$dartVmFlags', 
        
             if (debuggingOptions.enableImpeller) '--enable-impeller', 
        
             '--observatory-port=${debuggingOptions.hostVmServicePort ?? 0}', 
        
             if (route != null) '--route=$route', 
        
           ],

Whereas the device version adds them regardless, and only adds 2 specific ones if debuggingOptions.debuggingEnabled is true.

flutter/packages/flutter_tools/lib/src/ios/devices.dart

Lines 359 to 363 in d40f6d1

    
           if (debuggingOptions.debuggingEnabled) ...<String>[ 
        
             '--enable-checked-mode', 
        
             '--verify-entry-points', 
        
           ], 
        
           if (debuggingOptions.enableSoftwareRendering) '--enable-software-rendering',

Since the iOS simulator only runs in debug mode (profile and release mode are not supported), we think it's okay to remove the debuggingOptions.debuggingEnabled check for the simulator.

…and physical devices

vashworth · 2022-11-04T20:25:52Z

Note about cc03fe2.

So I've discovered that having the quotes surrounding the --dart-flags launch argument matters and differs between simulator and physical iOS devices.

flutter/packages/flutter_tools/lib/src/device.dart

Lines 926 to 927 in cc03fe2

    
           if (environmentType == EnvironmentType.physical && dartVmFlags.isNotEmpty) '--dart-flags="$dartVmFlags"', 
        
           if (environmentType == EnvironmentType.simulator && dartVmFlags.isNotEmpty) '--dart-flags=$dartVmFlags',

I tested this locally by using a single check for the dart flags and tried with and without quotes.
if (dartVmFlags.isNotEmpty) '--dart-flags="$dartVmFlags"',

Command I ran flutter run --dart-flags="--max_profile_depth 255,--foo"
Expected Result: Encountered disallowed Dart VM flag: --foo

Test 1 - with quotes, physical device - PASS
Result: [VERBOSE-3:switches.cc(441)] Encountered disallowed Dart VM flag: --foo

Test 2 - with quotes, simulator device - FAIL
Result: [VERBOSE-3:switches.cc(441)] Encountered disallowed Dart VM flag: "--max_profile_depth 255

Test 3 - without quotes, physical device - FAIL
Result: Runner[781:39983] Ignoring flag: true is an invalid value for flag max_profile_depth

Test 4 - without quotes, simulator device - PASS
Result: [VERBOSE-3:switches.cc(441)] Encountered disallowed Dart VM flag: --foo

packages/flutter_tools/lib/src/device.dart

jmagman · 2022-11-04T20:10:56Z

packages/flutter_tools/lib/src/device.dart

+      if (route != null) '--route=$route',
+      if (environmentType == EnvironmentType.physical && (platformArgs['trace-startup'] as bool? ?? false)) '--trace-startup',
+      if (enableImpeller) '--enable-impeller',
+      if (environmentType == EnvironmentType.simulator) '--observatory-port=${hostVmServicePort ?? 0}',


I think it can go back to the behavior in https://github.com/flutter/flutter/pull/49735/files#diff-5a3d215e39f292ac706d495f042ef45fab9f13962022f4e8e4580254e8557d52L334 and pass the port along, if it's been set:

Suggested change

if (environmentType == EnvironmentType.simulator) '--observatory-port=${hostVmServicePort ?? 0}',

if (hasObservatoryPort) '--observatory-port=${hostVmServicePort}',

Should this differ between simulator and physical?

Looking at that change, looks like they used deviceVmServicePort https://github.com/flutter/flutter/pull/49735/files#diff-5a3d215e39f292ac706d495f042ef45fab9f13962022f4e8e4580254e8557d52L335

Whereas it's hostVmServicePort in original simulator args

flutter/packages/flutter_tools/lib/src/ios/simulators.dart

Line 465 in 01507ba

'--observatory-port=${debuggingOptions.hostVmServicePort ?? 0}',

.

Is it possible to have both a deviceVmServicePort and hostVmServicePort and if so, which should be used if both are provided?

Good question, I had to ask @jonahwilliams for clarification here, this behavior has changed over time and not every spot it's used has been kept consistent.

To quote Jonah:

Application runs on device, it starts up a VM Service instance. This instance will broadcast on a port + auth code combo.

(Optional) If Host/Device are different machines, we forward the device port to a selected host port.

(Optional) If dds is enabled, dds connects to the selected host port, then broadcasts on the dds port

For physical devices, the right thing to use is deviceVmServicePort. The confusion is what to do if the "device" and host are really the same, as is the case with desktop apps or the iOS simulator, because there's no need to do port forwarding if they are the same machines. So I think hostVmServicePort is correct in this case, though it's very confusing and it's likely we have some bugs in here.

@christopherfujino the host/device port logic could use cleanup, comments, better help text, perhaps variable renaming, and verification we're doing the right thing for all device types.

@christopherfujino the host/device port logic could use cleanup, comments, better help text, perhaps variable renaming, and verification we're doing the right thing for all device types.

Agreed, let me make a tracking issue.

Agreed, let me make a tracking issue.

#114845

packages/flutter_tools/lib/src/device.dart

packages/flutter_tools/lib/src/ios/devices.dart

packages/flutter_tools/lib/src/ios/simulators.dart

Change --disable-service-auth-codes to not always be included for physical devices, only if disableServiceAuthCodes is true. Change --disable-observatory-publication to be used for simulator devices too. Change --enable-checked-mode & --verify-entry-points to be used if debuggingEnabled is true regardless of device type. Chnage --trace-startup to be used for simulator devices too.

…Port

jmagman

This LGTM with minor nits

packages/flutter_tools/lib/src/device.dart

jmagman

Thanks for tracking down all the weird corner cases.

…sts (flutter/flutter#114628)

* 3a656b1 Add more supported simulator debugging options and improve tests (flutter/flutter#114628) * 51c517c [flutter_tools/dap] Add support for forwarding `flutter run --machine` exposeUrl requests to the DAP client (flutter/flutter#114539) * 8e5439c Roll Flutter Engine from e7d7edab98ad to c76035429c36 (14 revisions) (flutter/flutter#115008) * a479718 Update cirrus key (flutter/flutter#115006) * dccc761 Roll Flutter Engine from c76035429c36 to aa4b3ea2f733 (27 revisions) (flutter/flutter#115025) * d0491dc Add the `channel` parameter to the Dartpad samples (flutter/flutter#115018) * befd8b6 722414a26 Implemented threadsafe platform channel replies on windows (flutter/engine#36909) (flutter/flutter#115033) * 154ae0f `updateSemantics` in TestWindow should always be implemented. (flutter/flutter#114857) * 008ac17 remove unnecessary brace in string interpolation (flutter/flutter#115032) * 8926d9e a9fbf6af5 Roll Fuchsia Linux SDK from mDzQK4ZUk_Y4wfZa_... to RNSA2Wp1MObtc7OHy... (flutter/engine#37485) (flutter/flutter#115045) * e6fb124 Roll Flutter Engine from a9fbf6af52c9 to b42ed6933cef (2 revisions) (flutter/flutter#115048) * ff75451 a42b2d9b9 Fix a race in the EmbedderA11yTest.A11yTreeIsConsistent tests (flutter/engine#37488) (flutter/flutter#115060) * c7d1154 a71c0c6c3 [Impeller] Support access the descriptor of PipelineFuture directly (flutter/engine#37415) (flutter/flutter#115063)

…sts (flutter/flutter#114628)

* 3a656b1 Add more supported simulator debugging options and improve tests (flutter/flutter#114628) * 51c517c [flutter_tools/dap] Add support for forwarding `flutter run --machine` exposeUrl requests to the DAP client (flutter/flutter#114539) * 8e5439c Roll Flutter Engine from e7d7edab98ad to c76035429c36 (14 revisions) (flutter/flutter#115008) * a479718 Update cirrus key (flutter/flutter#115006) * dccc761 Roll Flutter Engine from c76035429c36 to aa4b3ea2f733 (27 revisions) (flutter/flutter#115025) * d0491dc Add the `channel` parameter to the Dartpad samples (flutter/flutter#115018) * befd8b6 722414a26 Implemented threadsafe platform channel replies on windows (flutter/engine#36909) (flutter/flutter#115033) * 154ae0f `updateSemantics` in TestWindow should always be implemented. (flutter/flutter#114857) * 008ac17 remove unnecessary brace in string interpolation (flutter/flutter#115032) * 8926d9e a9fbf6af5 Roll Fuchsia Linux SDK from mDzQK4ZUk_Y4wfZa_... to RNSA2Wp1MObtc7OHy... (flutter/engine#37485) (flutter/flutter#115045) * e6fb124 Roll Flutter Engine from a9fbf6af52c9 to b42ed6933cef (2 revisions) (flutter/flutter#115048) * ff75451 a42b2d9b9 Fix a race in the EmbedderA11yTest.A11yTreeIsConsistent tests (flutter/engine#37488) (flutter/flutter#115060) * c7d1154 a71c0c6c3 [Impeller] Support access the descriptor of PipelineFuture directly (flutter/engine#37415) (flutter/flutter#115063)

…tter#114628) * add debugging options to simulator, test more debugging flags, add tests for other launch arguements * refactor iOS launch arguments to use one function for both simulator and physical devices * treat dart flags differently between physical and simulator * Simplify some flags between devices. Change --disable-service-auth-codes to not always be included for physical devices, only if disableServiceAuthCodes is true. Change --disable-observatory-publication to be used for simulator devices too. Change --enable-checked-mode & --verify-entry-points to be used if debuggingEnabled is true regardless of device type. Chnage --trace-startup to be used for simulator devices too. * fix ios release mode with buildable app startApp test * determine observatory-port from deviceVmServicePort and hostVmServicePort * add comments and remove hasObservatoryPort

* 3a656b1 Add more supported simulator debugging options and improve tests (flutter/flutter#114628) * 51c517c [flutter_tools/dap] Add support for forwarding `flutter run --machine` exposeUrl requests to the DAP client (flutter/flutter#114539) * 8e5439c Roll Flutter Engine from e7d7edab98ad to c76035429c36 (14 revisions) (flutter/flutter#115008) * a479718 Update cirrus key (flutter/flutter#115006) * dccc761 Roll Flutter Engine from c76035429c36 to aa4b3ea2f733 (27 revisions) (flutter/flutter#115025) * d0491dc Add the `channel` parameter to the Dartpad samples (flutter/flutter#115018) * befd8b6 722414a26 Implemented threadsafe platform channel replies on windows (flutter/engine#36909) (flutter/flutter#115033) * 154ae0f `updateSemantics` in TestWindow should always be implemented. (flutter/flutter#114857) * 008ac17 remove unnecessary brace in string interpolation (flutter/flutter#115032) * 8926d9e a9fbf6af5 Roll Fuchsia Linux SDK from mDzQK4ZUk_Y4wfZa_... to RNSA2Wp1MObtc7OHy... (flutter/engine#37485) (flutter/flutter#115045) * e6fb124 Roll Flutter Engine from a9fbf6af52c9 to b42ed6933cef (2 revisions) (flutter/flutter#115048) * ff75451 a42b2d9b9 Fix a race in the EmbedderA11yTest.A11yTreeIsConsistent tests (flutter/engine#37488) (flutter/flutter#115060) * c7d1154 a71c0c6c3 [Impeller] Support access the descriptor of PipelineFuture directly (flutter/engine#37415) (flutter/flutter#115063)

add debugging options to simulator, test more debugging flags, add te…

e10b1ec

…sts for other launch arguements

flutter-dashboard bot added platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. labels Nov 3, 2022

refactor iOS launch arguments to use one function for both simulator …

27c4540

…and physical devices

vashworth marked this pull request as ready for review November 3, 2022 23:14

vashworth requested a review from jmagman November 3, 2022 23:15

Merge remote-tracking branch 'upstream/master' into debug_options

b163a74

vashworth removed the request for review from jmagman November 4, 2022 19:40

treat dart flags differently between physical and simulator

cc03fe2

jmagman reviewed Nov 4, 2022

View reviewed changes

vashworth mentioned this pull request Nov 4, 2022

Dart flags in quotes causes error #114622

Closed

vashworth added 3 commits November 4, 2022 16:54

Merge remote-tracking branch 'upstream/master' into debug_options

506bb36

fix ios release mode with buildable app startApp test

0e691a3

christopherfujino mentioned this pull request Nov 7, 2022

[flutter_tools] Tool host/device port needs documentation and cleanup #114845

Closed

determine observatory-port from deviceVmServicePort and hostVmService…

f63aee9

…Port

jmagman reviewed Nov 7, 2022

View reviewed changes

packages/flutter_tools/lib/src/device.dart Outdated Show resolved Hide resolved

packages/flutter_tools/lib/src/device.dart Outdated Show resolved Hide resolved

add comments and remove hasObservatoryPort

aa52c49

jmagman approved these changes Nov 8, 2022

View reviewed changes

vashworth added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 9, 2022

auto-submit bot merged commit 3a656b1 into flutter:master Nov 9, 2022

This was referenced Nov 10, 2022

Roll Flutter from d3dcd7d532b6 to c7d11549a51e (13 revisions) flutter/plugins#6689

Merged

Roll Flutter from d3dcd7d532b6 to c7d11549a51e (13 revisions) flutter/packages#2794

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 10, 2022

3a656b1 Add more supported simulator debugging options and improve te…

c05e5ce

…sts (flutter/flutter#114628)

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

3a656b1 Add more supported simulator debugging options and improve te…

8a9e5df

…sts (flutter/flutter#114628)

engine-flutter-autoroll mentioned this pull request Nov 10, 2022

Roll Flutter from d3dcd7d532b6 to c7d11549a51e (13 revisions) flutter/packages#2795

Closed

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

3a656b1 Add more supported simulator debugging options and improve te…

9d193cf

…sts (flutter/flutter#114628)

engine-flutter-autoroll mentioned this pull request Nov 10, 2022

Roll Flutter from d3dcd7d532b6 to bdcacbf8e3fd (14 revisions) flutter/packages#2796

Closed

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

3a656b1 Add more supported simulator debugging options and improve te…

d0cdf92

…sts (flutter/flutter#114628)

engine-flutter-autoroll mentioned this pull request Nov 10, 2022

Roll Flutter from d3dcd7d532b6 to ca6165244f70 (15 revisions) flutter/packages#2798

Closed

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

3a656b1 Add more supported simulator debugging options and improve te…

6cca38a

…sts (flutter/flutter#114628)

engine-flutter-autoroll mentioned this pull request Nov 10, 2022

Roll Flutter from d3dcd7d532b6 to b31b9dc05f99 (16 revisions) flutter/packages#2799

Closed

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

3a656b1 Add more supported simulator debugging options and improve te…

57ca679

…sts (flutter/flutter#114628)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add more supported simulator debugging options and improve tests #114628

Add more supported simulator debugging options and improve tests #114628

Uh oh!

vashworth commented Nov 3, 2022 •

edited

Loading

Uh oh!

vashworth commented Nov 3, 2022

Uh oh!

vashworth commented Nov 3, 2022

Uh oh!

vashworth commented Nov 4, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmagman Nov 4, 2022

Uh oh!

vashworth Nov 4, 2022

Uh oh!

jmagman Nov 7, 2022

Uh oh!

christopherfujino Nov 7, 2022

Uh oh!

christopherfujino Nov 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmagman left a comment

Uh oh!

Uh oh!

Uh oh!

jmagman left a comment

Uh oh!

Uh oh!

	if (environmentType == EnvironmentType.simulator) '--observatory-port=${hostVmServicePort ?? 0}',
	if (hasObservatoryPort) '--observatory-port=${hostVmServicePort}',

Add more supported simulator debugging options and improve tests #114628

Add more supported simulator debugging options and improve tests #114628

Uh oh!

Conversation

vashworth commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

vashworth commented Nov 3, 2022

Uh oh!

vashworth commented Nov 3, 2022

Uh oh!

vashworth commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmagman Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

vashworth Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

christopherfujino Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

christopherfujino Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vashworth commented Nov 3, 2022 •

edited

Loading

vashworth commented Nov 4, 2022 •

edited

Loading