1162831 - Cannot detect x86 type when running marionette-webapi tests for emu-x86-kk on taskcluster

Status: RESOLVED FIXED

Description

[Hsin-Yi Tsai [:hsinyi]]

With bug 1153774, it correctly detects emulator type when running xpcshell or mochitest on taskcluster but not when running marionette-webapi. 

According to the log in the below link, the emulator type being used was still 'arm' even marionette-webapi tests were run on emu-x86-kk.
https://treeherder.allizom.org/#/jobs?repo=try&revision=e33743cfaa22

Comment 1

[John Dai[:jdai]]

Created attachment 8604022 [details] [diff] [review]
Marionette-webapi detects the emulator type implicitly.

Test cluster result:
[1]https://treeherder.allizom.org/#/jobs?repo=try&revision=40947e13daa0

Mozharness detects the emulator 'x86' type log:
[2]https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/M0jbld6vTWuEv6DLQmQUXQ/0/public/logs/live_backing.log

Comment 2

[Andrew Halberstadt [:ahal]]

Comment on attachment 8604022 [details] [diff] [review]
Marionette-webapi detects the emulator type implicitly.

Review of attachment 8604022 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but please address the comment.

::: scripts/marionette.py
@@ +349,5 @@
>              'xml_output': os.path.join(dirs['abs_work_dir'], 'output.xml'),
>              'html_output': os.path.join(dirs['abs_blob_upload_dir'], 'output.html'),
>              'logcat_dir': dirs['abs_work_dir'],
> +            'emulator': 'x86' if os.path.join(dirs['abs_b2g-distro_dir'], 'out',
> +                'target', 'product', 'generic_x86') else 'arm',

This makes it so --emulator is ignored even if explicitly set. We should either only do this if self.config.get('emulator') is None, or remove --emulator altogether. The latter *may* break some jobs though.

Comment 3

[John Dai[:jdai]]

Created attachment 8604535 [details] [diff] [review]
Marionette-webapi detects the emulator type implicitly. (V2)

Hi Andrew,

I addressed the review comment #2. Could you take a look again? Thank you.

Test cluster result:
https://treeherder.allizom.org/#/jobs?repo=try&revision=bcfb04b06ccc

Mozharness detects the emulator 'x86' type log:
https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/kPGcK7OARum3l4oRXW-JXg/0/public/logs/live_backing.log

Comment 4

[Andrew Halberstadt [:ahal]]

Comment on attachment 8604535 [details] [diff] [review]
Marionette-webapi detects the emulator type implicitly. (V2)

Sorry, I wasn't thinking properly in the first review. So this looks good, but it will fail unless you remove the references to --emulator in:
https://dxr.mozilla.org/mozilla-central/source/testing/config/mozharness/marionette.py

That means it will be tricky to land because we need to land this and the m-c patch at the same time. What we should actually do is:

1) Land your original patch and wait for it to merge
2) Remove --emulator from in-tree configs
3) Remove --emulator in the mozharness script

I'll help out.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8604022 [details] [diff] [review]
Marionette-webapi detects the emulator type implicitly.

Review of attachment 8604022 [details] [diff] [review]:
-----------------------------------------------------------------

::: scripts/marionette.py
@@ +348,5 @@
>              'profile': os.path.join(dirs['abs_gaia_dir'], 'profile'),
>              'xml_output': os.path.join(dirs['abs_work_dir'], 'output.xml'),
>              'html_output': os.path.join(dirs['abs_blob_upload_dir'], 'output.html'),
>              'logcat_dir': dirs['abs_work_dir'],
> +            'emulator': 'x86' if os.path.join(dirs['abs_b2g-distro_dir'], 'out',

Actually looking at this there's a bug here I missed the first time. os.path.join doesn't test for existence, so this will always return True. It needs to be:

os.path.isdir(os.path.join(...))

Comment 6

[John Dai[:jdai]]

(In reply to Andrew Halberstadt [:ahal] from comment #4)
> Comment on attachment 8604535 [details] [diff] [review]
> Marionette-webapi detects the emulator type implicitly. (V2)
> 
> Sorry, I wasn't thinking properly in the first review. So this looks good,
> but it will fail unless you remove the references to --emulator in:
> https://dxr.mozilla.org/mozilla-central/source/testing/config/mozharness/
> marionette.py
> 
> That means it will be tricky to land because we need to land this and the
> m-c patch at the same time. What we should actually do is:
> 
> 1) Land your original patch and wait for it to merge
> 2) Remove --emulator from in-tree configs
> 3) Remove --emulator in the mozharness script
> 
> I'll help out.

I did 2) and 3) on test cluster. It gave me "output_timeout 1000" and "must specify --binary, --emulator or --address"[1].

[1]https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/2tNCHNVPTbOvHPR7-92Nlg/0/public/logs/live_backing.log

Comment 7

[John Dai[:jdai]]

Created attachment 8605135 [details] [diff] [review]
Marionette-webapi detects the emulator type implicitly. (V3)

Hi Andrew,

I addressed the review comment #5. Could you take a look again? Thank you.

Test cluster result:
https://treeherder.allizom.org/#/jobs?repo=try&revision=0f12da4f2973

Mozharness detects the emulator 'x86' type log:
https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/CcCzcKm1RmOkGIgXp43EZg/0/public/logs/live_backing.log

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 8605135 [details] [diff] [review]
Marionette-webapi detects the emulator type implicitly. (V3)

Review of attachment 8605135 [details] [diff] [review]:
-----------------------------------------------------------------

Great looks good!

Yeah, in order to remove the --emulator parameter from the in-tree configs, this first needs to land on mozharness, then the in-tree pointer needs to get updated to a revision that includes this change. It's a multi-step process. Luckily as soon as this happens the mozharness scripts will be doing what you want. The rest is just follow up cleanup.

Comment 9

[Josh Cheng [:josh]]

Hi John,
Do you have any update? Is the patch good for checkin? Thanks!

Comment 10

[John Dai[:jdai]]

Hi Ryan,
Could you help me to checkin? Thanks!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

For future reference, just setting checkin-needed is enough :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/build/mozharness/rev/8bc2ecf8ad09

Comment 13

[Release Engineering SlaveAPI Service]

mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/production

Comment 14

[Release Engineering SlaveAPI Service]

mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/production

Comment 15

[Andrew Halberstadt [:ahal]]

I'll be bumping the mozharness revision in m-c soon for another bug anyway. Will close this out when that happens.

Comment 16

[Andrew Halberstadt [:ahal]]

Mozharness version bump recently landed on central, this should be live.

Comment 17

[Pete Moore [:pmoore][:pete]]

Resetting Version and Target Milestone that accidentally got changed...