Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Talos should use mozversion for getting application version

RESOLVED FIXED

Status

Testing
Talos
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: wlach, Assigned: wlach)

Tracking

Trunk
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 4 obsolete attachments)

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).
Created attachment 8491130 [details] [diff] [review]
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
Attachment #8491130 - Flags: feedback?(jmaher)
Blocks: 1050706
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
Attachment #8491130 - Flags: feedback?(jmaher) → feedback+
Depends on: 1069429
(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! :)
Created attachment 8491698 [details] [diff] [review]
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.
Attachment #8491698 - Flags: review?(jmaher)
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 :)
Attachment #8491698 - Flags: review?(jmaher) → review+
Created attachment 8491787 [details] [diff] [review]
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).
Attachment #8491787 - Flags: review?(kmoir)
Created attachment 8491789 [details] [diff] [review]
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.
Attachment #8491789 - Flags: review?(kmoir)
Attachment #8491789 - Attachment is patch: true
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
Created attachment 8492456 [details] [diff] [review]
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.
Attachment #8491787 - Attachment is obsolete: true
Attachment #8491787 - Flags: review?(kmoir)
Attachment #8492456 - Flags: review?(kmoir)
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.
Attachment #8492456 - Flags: review?(kmoir) → review-
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.
Created attachment 8493848 [details] [diff] [review]
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
Attachment #8492456 - Attachment is obsolete: true
Attachment #8493848 - Flags: review?(kmoir)
Comment on attachment 8493848 [details] [diff] [review]
Working patch to allow specification of custom talos options in talos.json

lgtm :-)
Attachment #8493848 - Flags: review?(kmoir) → review+
Attachment #8491789 - Flags: review?(kmoir) → review+
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.
Created attachment 8494759 [details] [diff] [review]
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)
Attachment #8491130 - Attachment is obsolete: true
Attachment #8494759 - Flags: review?(jmaher)
Depends on: 1072580
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
Attachment #8494759 - Flags: review?(jmaher) → review+
Something here landed in production today: https://wiki.mozilla.org/ReleaseEngineering/Maintenance#Reconfigs_.2F_Deployments
No longer blocks: 1050706
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.
Depends on: 1073697
Blocks: 1076990
This hasn't landed yet, still need to get bug 1073697 in order and upload a new internal mozversion dep to internal pypi.
No longer blocks: 1076990
Created attachment 8499737 [details] [diff] [review]
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
Attachment #8494759 - Attachment is obsolete: true
Attachment #8499737 - Flags: review+
(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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(wlachance)
Resolution: FIXED → ---
Created attachment 8510476 [details] [diff] [review]
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
Flags: needinfo?(wlachance)
Attachment #8510476 - Flags: review?(jmaher)
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.
Attachment #8510476 - Flags: review?(jmaher) → review+
Pushed extra fix: https://hg.mozilla.org/build/talos/rev/ae356cdd2e37
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.