Bug 171679 – buildbot: Cleanup simulators after running tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

In some cases, our simulator tear-down code fails to run.  Buildbot should explicitly search for simulators created by our WebKit scripts and clean them up after running layout tests on iOS simulators.

[Radar WebKit Bug Importer]

<rdar://problem/31994361>

[Jonathan Bedard]

Created attachment 309071 [details]
Patch

[Aakash Jain]

Comment on attachment 309071 [details]
Patch

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

> Tools/BuildSlaveSupport/cleanup-simulators:34
> +

Nit: don't we need two empty lines here? how does style check pass this.

> Tools/BuildSlaveSupport/cleanup-simulators:42
> +    subprocess.call(['killall', '-9', '-m', 'Simulator'])

I feel that since this script shares code with webkitpy and currently webkitpy has all the simulator code, it might be better to have this script in webkitpy and try to re-use the existing code.

> Tools/BuildSlaveSupport/cleanup-simulators:50
> +    simctllist = run_command(['xcrun', 'simctl', 'list']).splitlines()

Nit: can consider renaming simctllist to simctl_list to make it more readable.

> Tools/BuildSlaveSupport/cleanup-simulators:69
> +        if device_state(udid) != 1:

might be better to use something like Simulator.DeviceState.SHUTDOWN to clearly indicate what does 1 indicate.

[Jonathan Bedard]

Created attachment 309110 [details]
Patch

[Jonathan Bedard]

Sending the 'shutdown' command to a booted simulator will actually shut all booted simulators down. This seems like the simplest approach.

[Aakash Jain]

looks good to me, assuming it pass your local testing and EWS.

[Daniel Bates]

Comment on attachment 309110 [details]
Patch

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

> Tools/BuildSlaveSupport/kill-old-processes:116
> +        os.system("xcrun simctl shutdown booted")

Does this shutdown all booted simulators?

[Daniel Bates]

(In reply to Daniel Bates from comment #7)
> Comment on attachment 309110 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309110&action=review
> 
> > Tools/BuildSlaveSupport/kill-old-processes:116
> > +        os.system("xcrun simctl shutdown booted")
> 
> Does this shutdown all booted simulators?

Never mind you answered this in comment #5.

[Alexey Proskuryakov]

Comment on attachment 309110 [details]
Patch

What will happen if iOS SDK isn't installed? I think that there won't be a simctl to run.

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #9)
> Comment on attachment 309110 [details]
> Patch
> 
> What will happen if iOS SDK isn't installed? I think that there won't be a
> simctl to run.

Actually, simctl is still there, just without any iOS SDKs or simulators.  I tested this on some of our Mac bots.  This command will succeed, but do nothing on them.

[WebKit Commit Bot]

Comment on attachment 309110 [details]
Patch

Clearing flags on attachment: 309110

Committed r216244: <http://trac.webkit.org/changeset/216244>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

We need to re-open this bug since builedbot runs kill-old-processes as sudo.  Simctl separates simulators for each user, so the original patch did not work as intended.

[Jonathan Bedard]

Created attachment 309379 [details]
Patch

[Aakash Jain]

Comment on attachment 309379 [details]
Patch

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

looks fine to me.

> Tools/ChangeLog:8
> +        Simctl is partitioned for each user. Since kill-old-processes

Nit: "Simctl" -> "simctl", since its a command name.

> Tools/BuildSlaveSupport/kill-old-processes:116
> +        if os.geteuid() != 0 or not user:

better to reverse the if else to make it more readable.

[Jonathan Bedard]

Created attachment 309394 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 309394 [details]
Patch for landing

Clearing flags on attachment: 309394

Committed r216447: <http://trac.webkit.org/changeset/216447>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.