1068989 - Talos should use mozversion for getting application version

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[William Lachance (:wlach)]

Right now we use a combination of (1) loading the browser and (2) some handrolled code which is very similar to mozversion for this.

We have the opportunity to consolidate code, reduce duplication, and make things a smidgen faster. This will also solve an issue we have with getting the information out of the browser directly in bug 1050706 (since that's not as easy within e10s).

Comment 1

[William Lachance (:wlach)]

Attachment: Use mozversion

I've tested this. At least for the desktop case it works quite well and reduces the possibilities for failure.

Unfortunately we can not land this as-is, as it adds a new android-specific argument called --apkPath to get the version information from the android .apk (instead of trying to get it off the device). 

I believe the path to landing this looks something like:

1. Add just the --apkPath argument, but don't have it do anything
2. Update mozharness scripts to pass the apk via --apkPath:

https://github.com/mozilla/build-mozharness/blob/master/scripts/android_panda_talos.py
https://github.com/mozilla/build-mozharness/blob/master/configs/android/android_panda_talos_releng.py

3. Land the rest of this patch

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8491130 [details] [diff] [review]
Use mozversion

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

this is really cool, I think it is a win.

concerns:
* do we have mozversion available in the talos venv?
* do we have mozversion setup for the talos.zip we use for android?

we should test this on try

Comment 3

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #2)
> Comment on attachment 8491130 [details] [diff] [review]
> Use mozversion
> 
> Review of attachment 8491130 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this is really cool, I think it is a win.
> 
> concerns:
> * do we have mozversion available in the talos venv?
>
> * do we have mozversion setup for the talos.zip we use for android?

No to both, will add these before posting final patch.

> we should test this on try

Yes we should! :)

Comment 4

[William Lachance (:wlach)]

Attachment: Patch to add --apkPath option

This is just so we can add the code to mozharness to pass this parameter to talos, without breaking talos. I'll land the rest of this patch later.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8491698 [details] [diff] [review]
Patch to add --apkPath option

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

this is simple- we can uplift this to aurora probably, maybe even beta :)

Comment 6

[William Lachance (:wlach)]

Attachment: Make mozharness use the apkPath argument if set in talos.json

This lets us specify extra arguments in talos.json (inside the tree) which we can use inside mozharness for passing extra arguments. This will allow us to pass the new argument to talos while preserving compatibility with older versions of the tree (which will be using a version of talos which do not support the --apkPath option).

Comment 7

[William Lachance (:wlach)]

Attachment: Add extra_options to talos.json

This adds the extra option to talos.json. This should not go in before we land support for the --apkPath option in talos itself.

Comment 8

[Kim Moir [:kmoir] ET]

Comment on attachment 8491787 [details] [diff] [review]
Make mozharness use the apkPath argument if set in talos.json

I'm a bit confused by the use of device_talosrunner.py. The android talos tests run using android_panda_talos.py

Comment 9

[William Lachance (:wlach)]

Attachment: Actual patch against Android talos

Eugh, I was working against an older version of mozharness. I think this patch should work, though it's totally untested. Unfortunately it duplicates a lot of code from `mozharness/mozilla/testing/talos.py`, but I wasn't sure what the best strategy for code sharing would be between those two modules. Feedback welcome.

Comment 10

[Kim Moir [:kmoir] ET]

Comment on attachment 8492456 [details] [diff] [review]
Actual patch against Android talos

To use talos.json takes a lot of testing (I have been tried to land it many times in bug 920757 :-)).  This is the error is you try to your patch in staging

Traceback (most recent call last):
  File "scripts/scripts/android_panda_talos.py", line 32, in <module>
    class PandaTalosTest(TestingMixin, MercurialScript, BlobUploadMixin, Talos, MozpoolMixin, BuildbotMixin, SUTDeviceMozdeviceMixin):
TypeError: Error when calling the metaclass bases
    Cannot create a consistent method resolution
order (MRO) for bases SUTDeviceMozdeviceMixin, MercurialScript, TestingMixin, BuildbotMixin, MozpoolMixin, BlobUploadMixin, Talos

I didn't import Talos when I tried this before.  The reason I didn't land the talos.json stuff before was because it didn't work on one branch but I think most of those issues should be resolved now. I'll have to try to rework some of those patches once I finish some of my current priorities.

Comment 11

[William Lachance (:wlach)]

I just landed the --apkPath changes to talos, they don't do any harm and I think it should make testing easier.

https://hg.mozilla.org/build/talos/rev/bc955b57ddd1

Can always just back out if it causes huge problems, but that seems unlikely at this point.

Comment 12

[William Lachance (:wlach)]

Attachment: Working patch to allow specification of custom talos options in talos.json

It took a few tries, but this one actually works. :) See the one green 'rck' job against push d4b2f4409a6c:

https://tbpl.mozilla.org/?tree=Ash

Comment 13

[Kim Moir [:kmoir] ET]

Comment on attachment 8493848 [details] [diff] [review]
Working patch to allow specification of custom talos options in talos.json

lgtm :-)

Comment 14

[William Lachance (:wlach)]

Pushed mozharness change: https://hg.mozilla.org/build/mozharness/rev/2f7dc3cdef1d

I think we might as well roll the talos.json change in after we've actually applied the patch to make talos use mozversion.

Comment 15

[William Lachance (:wlach)]

Made a few minor modifications to original patch:

1. Added support for adding mozversion to talos.zip (necessitated bumping a few other dependencies)
2. Added mozversion to dependencies for code talos (again, necessitated bumping a few other dependencies)
3. Made talos abort if apkPath is not specified in non-development mode (don't want people accidentally reporting results without version info)

Doing a try run here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8ad365200a54

Comment 16

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #15)

> Doing a try run here:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8ad365200a54

Well that didn't work very well. The desktop talos jobs failed because we haven't uploaded the new deps to pypi. The mobile ones failed because we haven't had a mozharness reconfig yet (and thus --apkPath wasn't been used, and we failed out as we should). I'll file a bug to take care of the first, the second should be solvable just by retriggering the jobs later after the reconfig.

Comment 17

[William Lachance (:wlach)]

Attachment: Updated patch

Ok I think this one can actually go in after the mozharness reconfig and uploading some new deps to pypi. This one has the following changes from previous:

1. Added support for adding mozversion to talos.zip (necessitated bumping a few other dependencies)
2. Added mozversion to dependencies for code talos (again, necessitated bumping a few other dependencies)
3. Made talos abort if apkPath is not specified in non-development mode (don't want people accidentally reporting results without version info)

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8494759 [details] [diff] [review]
Updated patch

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

please test on try:
* android: all tests
* windows: other, tsvg, xperf
* osx, linux: other, tsvg

Comment 19

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

Something here landed in production today: https://wiki.mozilla.org/ReleaseEngineering/Maintenance#Reconfigs_.2F_Deployments

Comment 20

[William Lachance (:wlach)]

With the mozharness changes, this *almost* works. Android, Linux, and Mac are now fine. We're still running into a slight problem on windows because mozversion can't find the binary if we don't specify the full path to it (including the .exe extension, which we deliberately strip for talos). Filed bug 1073697 to fix mozversion to allow this.

Comment 21

[William Lachance (:wlach)]

This hasn't landed yet, still need to get bug 1073697 in order and upload a new internal mozversion dep to internal pypi.

Comment 22

[William Lachance (:wlach)]

Attachment: Updated patch to use mozversion 0.8

Updated patch to use mozversion 0.8, carrying forward r+

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f2ce22a2562c

Comment 23

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #22)
> 
> Try run:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f2ce22a2562c

Forgot to specify --apkPath on Android, doing another try run just for that platform:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6cac64a62f8f

Comment 24

[William Lachance (:wlach)]

Ok, this bug is totally non-urgent but if I don't finish it off now we probably never will (and I think the cleanup is worthwhile). It's already bitrotted somewhat.

There was a problem in the android logic, which I think I've fixed. I've kicked off a new try run with an updated patch. If that goes through, I'll commit:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=380169a58401

Comment 25

[William Lachance (:wlach)]

Ok that one worked on android, but not desktop due to a logic error (doing foo.get('baz', foo.get('bar')) will always return the value of 'baz' if 'baz' is defined, even it's None). Fixed that, doing a hopefully last try run:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=69899cce5ef7

Comment 26

[William Lachance (:wlach)]

Ok, that worked well. Committed patch updated for bitrot + breakage:

https://hg.mozilla.org/build/talos/rev/e2ca7449db07

Comment 27

[Joel Maher ( :jmaher ) (UTC -8)]

oh no, I overlooked something obvious.  Android fails in this case, the modifications to getInfo.html were not being tested on try because we use a static webserver, not the files from talos.zip.

In testing stuff locally, I see this:
Traceback (most recent call last):
  File "run_tests.py", line 292, in <module>
    main()
  File "run_tests.py", line 289, in main
    sys.exit(run_tests(parser))
  File "run_tests.py", line 229, in run_tests
    talos_results.add(mytest.runTest(browser_config, test))
  File "/home/jmaher/mozilla/talos/talos/ttest.py", line 346, in runTest
    self.initializeProfile(profile_dir, browser_config)
  File "/home/jmaher/mozilla/talos/talos/ttest.py", line 107, in initializeProfile
    if not self._ffsetup.InitializeNewProfile(profile_dir, browser_config):
  File "/home/jmaher/mozilla/talos/talos/ffsetup.py", line 272, in InitializeNewProfile
    version_info = mozversion.get_version(binary=binary)
  File "/home/jmaher/mozilla/talos/local/lib/python2.7/site-packages/mozversion-0.8-py2.7.egg/mozversion/mozversion.py", line 265, in get_version
    version = LocalVersion(binary)
  File "/home/jmaher/mozilla/talos/local/lib/python2.7/site-packages/mozversion-0.8-py2.7.egg/mozversion/mozversion.py", line 99, in __init__
    raise IOError('Binary path does not exist: %s' % binary)
IOError: Binary path does not exist: org.mozilla.fennec


this makes sense since we pass in org.mozilla.fennec and not a path to a .apk for the binary name.  mozversion doesn't work with that.  

Is there something simple we can do to remediate this?  I am not familiar enough with mozversion to know how it works.  The only realistic way to solve this is to query the device to get the info since it isn't guaranteed we will have the .apk in a known location.  Maybe there is something I am overlooking.

Comment 28

[William Lachance (:wlach)]

Attachment: Always require --apkPath on Android

This makes the error when running locally without --apkPath more clear. Let's just always require this to be specified, even with --develop

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8510476 [details] [diff] [review]
Always require --apkPath on Android

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

my only concern here is our android mozharness talos doesn't do this.  We could add it in as part of talos.json, assuming we know the location of the .apk.  Please land this along with a comment about how best to enable this.

Comment 30

[William Lachance (:wlach)]

Pushed extra fix: https://hg.mozilla.org/build/talos/rev/ae356cdd2e37