Bug 167654 – Workaround for simctl install failing to report install failure

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

simctl launch has a known issue where it will sometimes report a successful install even though the install failed.  We need to workaround this issue until it is fixed.

[Jonathan Bedard]

Created attachment 300241 [details]
Complete workaround

[Jonathan Bedard]

Comment on attachment 300241 [details]
Complete workaround

This workaround will recover from any recoverable bug in simctl install, but will take much longer to install an app in the event that any simctl install bug is encountered.

[Jonathan Bedard]

Created attachment 300242 [details]
Partial workaround

[Jonathan Bedard]

Comment on attachment 300242 [details]
Partial workaround

This workaround will work if simctl launch never places the app in the target simulator's directory when it fails without warning.  We have no evidence that this is how simctl launch fails.  This patch, however, will take less time in the event that simctl launch does fail.

[Jonathan Bedard]

Comment on attachment 300241 [details]
Complete workaround

View in context: https://bugs.webkit.org/attachment.cgi?id=300241&action=review

> Tools/Scripts/webkitpy/xcode/simulator.py:266
>      def install_app(self, app_path):

A comment pointing to the appropriate bug needs to be added

[Jonathan Bedard]

Comment on attachment 300242 [details]
Partial workaround

Bugs related to the issue we are working around indicate that an app may exist on simulator but still not entirely be installed.  This workaround will still be susceptible to a failure of simctl install

[Jonathan Bedard]

Created attachment 300261 [details]
Patch

[Daniel Bates]

Comment on attachment 300261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300261&action=review

I am not happy that we have to use launching of a simulator app as a way to validate whether the install was successful. I wish there was a more direct way to test that installation succeeded. Even better, I wish that "simctl install" only returned a zero status (for success) after successful installing the app.

> Tools/Scripts/webkitpy/xcode/simulator.py:267
> +        # FIXME: This is a workaround for <rdar://problem/30273973>, Racey failure of simctl launch.

The bug title for <rdar://problem/30273973> mentions "simctl launch". Is this the correct bug for this install_app workaround?

> Tools/Scripts/webkitpy/xcode/simulator.py:270
> +                break

I would just return from here instead of breaking out of the loop.

> Tools/Scripts/webkitpy/xcode/simulator.py:277
> +                bundle_id = self._host.executive.run_command([
> +                    '/usr/libexec/PlistBuddy',
> +                    '-c',
> +                    'Print CFBundleIdentifier',
> +                    self._host.filesystem.join(app_path, 'Info.plist'),
> +                ]).rstrip()

This duplicates the logic of the function you added in the patch for bug #165927 (https://trac.webkit.org/changeset/211370),  DarwinPort.app_identifier_from_bundle().

> Tools/Scripts/webkitpy/xcode/simulator.py:278
> +                pid = self.launch_app(bundle_id, [], attempts=1)

I take it you find it variable name pid to be helpful in documenting that launch_app returns a process identifier? I would have inlined the right-hand side of this expression into the line below and avoid the need for the local variable pid.

> Tools/Scripts/webkitpy/xcode/simulator.py:294
> +        # FIXME: This is a workaround for <rdar://problem/30172453>

Very Minor: Missing a period at the end of this sentence.

[Jonathan Bedard]

Comment on attachment 300261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300261&action=review

>> Tools/Scripts/webkitpy/xcode/simulator.py:267
>> +        # FIXME: This is a workaround for <rdar://problem/30273973>, Racey failure of simctl launch.
> 
> The bug title for <rdar://problem/30273973> mentions "simctl launch". Is this the correct bug for this install_app workaround?

Right radar, wrong title.  Fixed.

>> Tools/Scripts/webkitpy/xcode/simulator.py:277
>> +                ]).rstrip()
> 
> This duplicates the logic of the function you added in the patch for bug #165927 (https://trac.webkit.org/changeset/211370),  DarwinPort.app_identifier_from_bundle().

Yes, it does.

I thought this was the best solution, rather than refactoring the entire device class just to pass in a port rather than an executive.  Hopefully, this fix is relatively short-lived.

[Jonathan Bedard]

Created attachment 300266 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 300266 [details]
Patch

Rejecting attachment 300266 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 300266, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Dan Bates found in /Volumes/Data/EWS/WebKit/Tools/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/2981688

[Jonathan Bedard]

Created attachment 300274 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 300274 [details]
Patch for landing

Clearing flags on attachment: 300274

Committed r211457: <http://trac.webkit.org/changeset/211457>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.