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).

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 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

(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! :)

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 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 :)

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).

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 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

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 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.

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.

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 on attachment 8493848 [details] [diff] [review] Working patch to allow specification of custom talos options in talos.json lgtm :-)

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.

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

(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.

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 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

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

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.

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

Updated patch to use mozversion 0.8, carrying forward r+ Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f2ce22a2562c

(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

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

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

Ok, that worked well. Committed patch updated for bitrot + breakage: https://hg.mozilla.org/build/talos/rev/e2ca7449db07

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.

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

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.

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