Closed Bug 921596 Opened 11 years ago Closed 11 years ago

mozharness should use in-tree mozbase for panda android 4.0

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: k0scist, Assigned: dminor)

References

Details

(Whiteboard: [mozharness])

Attachments

(3 files, 1 obsolete file)

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.
Blocks: 917750
Whiteboard: [mozharness]
Assignee: nobody → kmoir
Attached patch bug912249.patch (deleted) — Splinter Review
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?
Flags: needinfo?(jhammel)
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
Flags: needinfo?(jhammel) → needinfo?(ahalberstadt)
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.
(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).
(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?
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.
That sounds like this module should be placed before the other one in the ordered virtualenv_modules list, then.
(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
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
(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?
No objections
Summary: mozharness should use in-tree mozfile for panda android 4.0 → mozharness should use in-tree mozbase for panda android 4.0
Assignee: kmoir → dminor
Status: NEW → ASSIGNED
Blocks: 892569
Attached patch WIP Mozbase mixin (obsolete) (deleted) — Splinter Review
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.
Attached patch Patch to add Mozbase Mixin (deleted) — Splinter Review
Attachment #815047 - Attachment is obsolete: true
Attachment #815782 - Flags: review?(kmoir)
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 on attachment 815782 [details] [diff] [review] Patch to add Mozbase Mixin looks good in staging with tests run on cedar branch
Attachment #815782 - Flags: review?(kmoir) → review+
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?
Any way we can freeze the API for mozdevice so this isn't an issue?
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
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
(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
Attached patch mozbase_pep8 (deleted) — Splinter Review
Just a quick cleanup -- removal of unused imports, and whitespace fixes to remove pyflakes&pep8 warnings.
Attachment #825661 - Flags: review?(dminor)
Comment on attachment 825661 [details] [diff] [review] mozbase_pep8 Review of attachment 825661 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for cleaning it up.
Attachment #825661 - Flags: review?(dminor) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Component: General Automation → Mozharness
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: