921596 - mozharness should use in-tree mozbase for panda android 4.0

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Jeff Hammel]

See across-the-board Android 4.0 Opt on
https://tbpl.mozilla.org/?tree=Try&rev=c20c67ab4e69 when mirroring
modern mozbase -> m-c.

In all cases, failure occurs due to an inability to import `tree` from
mozfile:
"""
16:02:01     INFO -    File
"/builds/panda-0057/test/build/tests/mochitest/runtestsremote.py",
line 17, in <module>
16:02:01     INFO -      from automation import Automation
16:02:01     INFO -    File
"/builds/panda-0057/test/build/tests/mochitest/automation.py", line
34, in <module>
16:02:01     INFO -      from mozprofile import Profile, Preferences
16:02:01     INFO -    File
"/builds/panda-0057/test/build/tests/mozbase/mozprofile/mozprofile/__init__.py",
line 14, in <module>
16:02:01     INFO -      from cli import *
16:02:01     INFO -    File
"/builds/panda-0057/test/build/tests/mozbase/mozprofile/mozprofile/cli.py",
line 17, in <module>
16:02:01     INFO -      from profile import FirefoxProfile
16:02:01     INFO -    File
"/builds/panda-0057/test/build/tests/mozbase/mozprofile/mozprofile/profile.py",
line 17, in <module>
16:02:01     INFO -      from mozfile import tree
16:02:01     INFO -  ImportError: cannot import name tree
"""

As I understand it, this is because mozharness does not use the
in-tree modules except as specified here:
http://hg.mozilla.org/build/mozharness/file/d080fb4225a3/scripts/android_panda.py#l131

mozfile should be added to this list, if my assessment is accurate.

Comment 1

[Kim Moir [:kmoir] ET]

Attachment: bug912249.patch

I think you have to specify the version to include it from the tests versus the packages.  The downside of this approach is that you have to update the mozharness scripts every time the version changes.  Is there a reason not to include this version on the mozilla repos?

Comment 2

[Jeff Hammel]

I'm not sure, but I believe our strategy mozbase-side is to use the versions internal to mozilla-central.  I'll ask Andrew for comment since he's heading up mozbase work for Q4

Comment 3

[Dan Minor [:dminor]]

I'm going to try adding an option in mozharness to call pip with --ignore-installed which will hopefully force an install from the test package without having to specify a version number.

Comment 4

[Andrew Halberstadt [:ahal]]

(In reply to Jeff Hammel [:jhammel] from comment #2)
> I'm not sure, but I believe our strategy mozbase-side is to use the versions
> internal to mozilla-central.  I'll ask Andrew for comment since he's heading
> up mozbase work for Q4

Yes, ideally we just like to use whatever is in mozilla-central. If it can't find anything there, it will just grab whatever the latest version is on the puppetagain server (http://puppetagain.pub.build.mozilla.org/data/python/packages/), but nowhere do we currently peg mozharness scripts to specific mozbase versions.

(In reply to Dan Minor [:dminor] from comment #3)
> I'm going to try adding an option in mozharness to call pip with
> --ignore-installed which will hopefully force an install from the test
> package without having to specify a version number.

See also bug 879765 and bug 908356. Jgriffin already implemented a two-pass install system, but it only works if installing from requirements.txt (bug 879765 is for getting it working in the general sense).

Comment 5

[Aki Sasaki (not active)]

(In reply to Dan Minor [:dminor] from comment #3)
> I'm going to try adding an option in mozharness to call pip with
> --ignore-installed which will hopefully force an install from the test
> package without having to specify a version number.

Is the virtualenv not getting clobbered?

Comment 6

[Dan Minor [:dminor]]

When I spoke to Kim on irc, she said that yes the virtualenv is being clobbered, but that it looked like an non-test package module was being pulled into satisfy a dependency earlier on, causing the test package module to not be installed, since we don't specify a version.

Comment 7

[Aki Sasaki (not active)]

That sounds like this module should be placed before the other one in the ordered virtualenv_modules list, then.

Comment 8

[Andrew Halberstadt [:ahal]]

(In reply to Dan Minor [:dminor] from comment #6)
> When I spoke to Kim on irc, she said that yes the virtualenv is being
> clobbered, but that it looked like an non-test package module was being
> pulled into satisfy a dependency earlier on, causing the test package module
> to not be installed, since we don't specify a version.

Yeah, so that's exactly what bug 915865 and bug 921596 are about. If you are using a requirements.txt file you can pass in two_pass=True to the register_virtualenv_module function [1]. This will install everything in the requirements.txt file first with --no-deps and then a second time without --no-deps to get missed dependencies the first time around.

But you can't do this (yet) if you are installing via a list of modules. Your options are either to re-order them like Aki suggested, or else fix bug 915865 for us ;)

[1] http://mxr.mozilla.org/build/source/mozharness/scripts/desktop_unittest.py#181

Comment 9

[Andrew Halberstadt [:ahal]]

Note having said that, I'd highly recommend just installing *all* of mozbase from in-tree sources whether you need the modules or not as there are a lot of inter-dependencies that could bite you if some of them are coming from in-tree and some are coming from puppetagain.

The easiest way to do this is to use the in-tree mozbase requirements.txt [1] with two_pass=True. You'll need to have a fallback in-case this file doesn't exist so things continue to work on aurora, beta and release. Use [2] as a guideline.

Jeff suggested we create a MozbaseMixin that inherits from VirtualenvMixin which sets up mozbase correctly for you. I think this is a great idea.

[1] http://mxr.mozilla.org/mozilla-central/source/testing/config/mozbase_requirements.txt
[2] http://mxr.mozilla.org/build/source/mozharness/scripts/b2g_emulator_unittest.py#197

Comment 10

[Dan Minor [:dminor]]

(In reply to Andrew Halberstadt [:ahal] from comment #9)
> Note having said that, I'd highly recommend just installing *all* of mozbase
> from in-tree sources whether you need the modules or not as there are a lot
> of inter-dependencies that could bite you if some of them are coming from
> in-tree and some are coming from puppetagain.

This makes a lot of sense to me. Any objections to rescoping the bug to cover all of mozbase?

Comment 11

[Kim Moir [:kmoir] ET]

No objections

Comment 12

[Dan Minor [:dminor]]

Attachment: WIP Mozbase mixin

This is a work in progress that seems ok locally. I'm planning to test it on Ash tomorrow, but thought I would put it here first in case anyone had any comments.

Comment 13

[Dan Minor [:dminor]]

Attachment: Patch to add Mozbase Mixin

Ash run is here: https://tbpl.mozilla.org/?tree=Ash&rev=67fd469f8a73

Comment 14

[Ed Morley [:emorley]]

Do we need to update here as well?
https://hg.mozilla.org/build/mozharness/file/5db3cb3f5159/scripts/android_panda_talos.py#l88

Comment 15

[Kim Moir [:kmoir] ET]

dminor I tested this in staging tonight but had some issues, I'm going to look at it again in the morning with better eyes :-) and see what's happening.

Comment 16

[Kim Moir [:kmoir] ET]

Comment on attachment 815782 [details] [diff] [review]
Patch to add Mozbase Mixin

looks good in staging with tests run on cedar branch

Comment 17

[Justin Wood (:Callek)]

Copy/pasting from IRC chat...

[11:57:18]	Callek	kmoir: sooo re: Bug 921596 -- I'm actually against doing that as specified.... let me state my reason(s) and then get your opinion
[11:57:19]	bugbot	Bug https://bugzilla.mozilla.org/show_bug.cgi?id=921596 normal, --, ---, dminor, ASSIGNED , mozharness should use in-tree mozbase for panda android 4.0
[11:57:52]	kmoir	Callek: okay listening
[11:58:37]	Callek	kmoir: so basically since mozharness code/rev is not based on in-tree code/revs if in-tree mozdevice changes in an incompatible way with mozharness we'll have a chicken and egg problem that I don't think is worth having. It also means that core functionality of mozharness is dependant on exactly how the in-tree code looks, and can be modified in weird ways by a malicious try push.
[11:59:27]	Callek	kmoir: I'd much rather let mozharness use the mozdevice on our pypi mirror directly (a static version) and then allow the actual running tests to use the in-tree version for their test runs
[11:59:55]	Callek	kmoir: I agree though that using [the] mozdevice [currently] on foopies for mozharness is probably not useful though
[12:00:29]	Callek	kmoir: I'm not considering my objection a hard-blocker here, I'll let you make the call on that, I just wanted to raise my objection to you directly
[12:00:58]	Callek	(sorry I didn't glance at that bug before now though)
[12:02:39]	kmoir	Callek: I understand your concerns. I don't understand why we just don't push new packages to pypi more often. Maybe record your comments on the bug so dminor can respond when he gets back, it is his bug

...so dminor, what do you think re: all that?

Comment 18

[Aki Sasaki (not active)]

Any way we can freeze the API for mozdevice so this isn't an issue?

Comment 19

[Dan Minor [:dminor]]

From my point of view, and others I've talked to on the ateam, tests should always be running using the version of mozbase present in the test package. The approach in this patch is already being done for b2g [1] and desktop [2] and the current code for pandas [3] is moving in the that direction.

I can see how you would like to have mozharness run off a stable version of mozdevice. If an in-tree change breaks mozharness it will break the build (hopefully on try) and be caught at that point.

Malicious try pushes are certainly possible, but some degree of trust is already implicit with access to try, since people must be vouched for and sign an agreement prior to access.

I guess ideally we would have the tests run in a separate virtualenv from mozharness, but that seems to me like a larger issue than this bug, since I'm following what is being done for other test targets. I would argue that landing this patch would be a step in the right direction, as we could then remove the duplicated code from [1] and [2] and work towards "doing the right thing".

[1] http://hg.mozilla.org/build/mozharness/file/646e52346be8/scripts/b2g_emulator_unittest.py#l197
[2] http://hg.mozilla.org/build/mozharness/file/646e52346be8/scripts/desktop_unittest.py#l172
[3] http://hg.mozilla.org/build/mozharness/file/646e52346be8/scripts/android_panda.py#l134

Comment 20

[Dan Minor [:dminor]]

Followed up with Callek in irc, agreement was that running tests in a separate venv is out of scope for this bug.

https://hg.mozilla.org/build/mozharness/rev/28399b92470f

Comment 21

[Jonathan Griffin (:jgriffin)]

(In reply to Dan Minor [:dminor] from comment #20)
> Followed up with Callek in irc, agreement was that running tests in a
> separate venv is out of scope for this bug.
> 
> https://hg.mozilla.org/build/mozharness/rev/28399b92470f

pushed to production

Comment 22

[Aki Sasaki (not active)]

Attachment: mozbase_pep8

Just a quick cleanup -- removal of unused imports, and whitespace fixes to remove pyflakes&pep8 warnings.

Comment 23

[Dan Minor [:dminor]]

Comment on attachment 825661 [details] [diff] [review]
mozbase_pep8

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

Thanks for cleaning it up.

Comment 24

[Aki Sasaki (not active)]

Comment on attachment 825661 [details] [diff] [review]
mozbase_pep8

http://hg.mozilla.org/build/mozharness/rev/c0a269d8b6eb