707976 - Remove manifestdestiny from its old location in m-c and use that in testing/mozbase

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

Now that mozbase has been checked into m-c (mozdevice on inbound, should land tomorrow), we should remove the old one off versions of these packages that are lying around and modify the things that are using them.

http://mxr.mozilla.org/mozilla-central/source/testing/firebug/
http://mxr.mozilla.org/mozilla-central/source/build/manifestparser.py
http://mxr.mozilla.org/mozilla-central/find?string=mobile%2Fdevicemanager[^-]*.py&tree=mozilla-central

Comment 1

[Jeff Hammel]

mozinfo also has a duplicate: http://mxr.mozilla.org/mozilla-central/source/build/tests/

Comment 2

[Jeff Hammel]

The manifest destiny tests are also a duplicate:

http://mxr.mozilla.org/mozilla-central/source/build/tests/

Comment 3

[Jeff Hammel]

Its also worth noting that each of these issues may be done separately.  Each package will have different issues and different scopes of problems and its probably better to tackle them one at a time versus as a monolith.  We can do this here or we can file blocking bugs

Comment 4

[William Lachance (:wlach)]

Note that the decision was to keep devicemanager and friends in the tree for now:

https://groups.google.com/d/topic/mozilla.tools/nKQKe1ZiDmU/discussion

I personally think we should consider revisiting that decision soon now that firefox for android is out, but for now devicemanager (mozdevice) should stay in the tree. I will continue to mirror things over to the mozbase github repo as needed for Eideticker and other projects.

Comment 5

[cmtalbert]

(In reply to William Lachance (:wlach) from comment #4)
> Note that the decision was to keep devicemanager and friends in the tree for
> now:
> 
> https://groups.google.com/d/topic/mozilla.tools/nKQKe1ZiDmU/discussion
> 
> I personally think we should consider revisiting that decision soon now that
> firefox for android is out, but for now devicemanager (mozdevice) should
> stay in the tree. I will continue to mirror things over to the mozbase
> github repo as needed for Eideticker and other projects.

I think this is a fine time to revisit that decision. We just have to change everything that uses it. (assuming we decide to put mozdevice together and use it as a package).  I think the mozdevice change is worthy of its own bug, btu the other issues can just be patches on this bug, separately handled and pushed.

Comment 6

[Jeff Hammel]

(In reply to Clint Talbert ( :ctalbert ) from comment #5)
> (In reply to William Lachance (:wlach) from comment #4)
> > Note that the decision was to keep devicemanager and friends in the tree for
> > now:
> > 
> > https://groups.google.com/d/topic/mozilla.tools/nKQKe1ZiDmU/discussion
> > 
> > I personally think we should consider revisiting that decision soon now that
> > firefox for android is out, but for now devicemanager (mozdevice) should
> > stay in the tree. I will continue to mirror things over to the mozbase
> > github repo as needed for Eideticker and other projects.
> 
> I think this is a fine time to revisit that decision. We just have to change
> everything that uses it. (assuming we decide to put mozdevice together and
> use it as a package).  I think the mozdevice change is worthy of its own
> bug, btu the other issues can just be patches on this bug, separately
> handled and pushed.

FWIW, I don't see a problem using devicemanager from the testing/mozbase location in m-c vs build/mobile (outside of ensuring that the stakeholders are aware of the change).  In general, when mozbase is merged to m-c, we *should* *very closely* look at the diffs and port changes there back to mozbase.  I did this the last time and there were changes, albeit not major ones.  Ideally we would merge back per-change, but while there is more work merging back after/during/before a mozbase -> m-c merge, I don't think that changes the mechanics of anything

Comment 7

[Jeff Hammel]

Some of this, ABICT, has been done. Changing summary to note what is left

Comment 9

[Jeff Hammel]

It looks like, much to my surprise, that most if not all of the usage of mozinfo points to the testing/mozbase directory (that is, the one we want): http://mxr.mozilla.org/mozilla-central/search?string=mozinfo . I *think* (maybe, try will tell) that removing the old file should be fine.  We also don't need the `pythonpath.py` invocations there, but that shouldn't matter for present purposes (and its already ticketed, bug 774911)

I haven't checked on manifestdestiny.  At the very least the tests, http://mxr.mozilla.org/mozilla-central/source/build/tests/, should be done something with (like, deleted and all references to them deleted)

Comment 10

[Jeff Hammel]

mozinfo was removed in a separate bug: bug 746545 , I think!  The dates are all strange, as A. build/mozinfo.py is still in mxr and B. I friggin swear it has existed since I've filed this bug.  

I have no clue.  In any case, since both locally in m-c and hgweb show it gone, i'm going to accept that and move forward.

Comment 12

[Jeff Hammel]

Attachment: remove manifestdestiny

Still needs to go to try....not sure what all platforms/tests i should use (guessing at least xpcshell? all platforms?) `make` and `make check` work for me locally

Comment 13

[Jeff Hammel]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=817aa9cb5581

try: -b do -p all -u xpcshell,marionette,mochitests -t none --post-to-bugzilla Bug 707976

Not sure if this is sufficient or overkill.  In any case, if I'm missing something, lemme know

Comment 14

[Jeff Hammel]

This is a go except on android:

========= Started 'python2.7 xpcshell/remotexpcshelltests.py ...' warnings (results: 1, elapsed: 0 secs) (at 2013-05-23 20:06:25.515720) =========
python2.7 xpcshell/remotexpcshelltests.py --deviceIP 10.250.49.68 --xre-path ../hostutils/xre --manifest xpcshell/tests/xpcshell_android.ini --build-info-json xpcshell/mozinfo.json --testing-modules-dir modules --local-lib-dir ../fennec --apk ../fennec-24.0a1.en-US.android-arm-armv6.apk --no-logfiles --symbols-path=http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-817aa9cb5581/try-android-armv6/fennec-24.0a1.en-US.android-arm-armv6.crashreporter-symbols.zip
 in dir /builds/tegra-080/test/build/tests (timeout 2400 secs) (maxTime 14400 secs)
 watching logfiles {}
 argv: ['python2.7', 'xpcshell/remotexpcshelltests.py', '--deviceIP', '10.250.49.68', '--xre-path', '../hostutils/xre', '--manifest', 'xpcshell/tests/xpcshell_android.ini', '--build-info-json', 'xpcshell/mozinfo.json', '--testing-modules-dir', 'modules', '--local-lib-dir', '../fennec', '--apk', u'../fennec-24.0a1.en-US.android-arm-armv6.apk', '--no-logfiles', u'--symbols-path=http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-817aa9cb5581/try-android-armv6/fennec-24.0a1.en-US.android-arm-armv6.crashreporter-symbols.zip']
 environment:
  HOME=/home/cltbld
  LOGNAME=cltbld
  MINIDUMP_SAVE_PATH=/builds/tegra-080/test/minidumps
  MINIDUMP_STACKWALK=/builds/minidump_stackwalk
  OLDPWD=/home/cltbld
  PATH=/usr/local/bin:/usr/local/bin:/usr/local/bin:/bin:/usr/bin:/usr/local/sbin:/usr/sbin:/sbin:/home/cltbld/bin
  PWD=/builds/tegra-080/test/build/tests
  PYTHONPATH=/builds/sut_tools
  SHELL=/bin/sh
  SHLVL=4
  SUT_IP=10.250.49.68
  SUT_NAME=tegra-080
  USER=cltbld
  _=/tools/buildbot/bin/python2.7
 using PTY: False
Traceback (most recent call last):
  File "xpcshell/remotexpcshelltests.py", line 9, in <module>
    import runxpcshelltests as xpcshell
  File "/builds/tegra-080/test/build/tests/xpcshell/runxpcshelltests.py", line 14, in <module>
    import manifestparser
ImportError: No module named manifestparser
program finished with exit code 1
elapsedTime=0.180305
TinderboxPrint: xpcshell<br/><em class="testfail">T-FAIL</em>
Unknown Error: command finished with exit code: 1

I'm not sure how this is wired up...but suffice it to say, it should be wired up differently.

Comment 15

[Jeff Hammel]

Looking at this from https://tbpl.mozilla.org/php/getParsedLog.php?id=23342735&tree=Try&full=1#error0 , it doesn't look like PYTHONPATH etc gets set for any of the mozbase modules.  We'll need to somehow get manifestparser on the path; I'm not sure the current going way of doing this to get this to work for android/fennec .  Does anyone happen to know?

Comment 16

[Jeff Hammel]

Hmm....there's also this lovely:

25 # --------------------------------------------------------------
26 # TODO: this is a hack for mozbase without virtualenv, remove with bug 849900
27 #
28 here = os.path.dirname(__file__)
29 mozbase = os.path.realpath(os.path.join(os.path.dirname(here), 'mozbase'))
30 
31 try:
32     import mozcrash
33 except:
34     deps = ['mozcrash',
35             'mozfile',
36             'mozlog']
37     for dep in deps:
38         module = os.path.join(mozbase, dep)
39         if module not in sys.path:
40             sys.path.append(module)
41     import mozcrash

(from http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#25 )

We could leverage this. :shrug:  Not sure about strategic plans here o_O

Comment 17

[Jonathan Griffin (:jgriffin)]

I think that's a fine approach for now.  Eventually, we'll have fennec tests run via a mozharness script, and then we won't have this issue.

Comment 18

[Andrew Halberstadt [:ahal]]

Comment on attachment 753536 [details] [diff] [review]
remove manifestdestiny

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

Sorry, must have missed this review. Cool though! :)

Comment 19

[Jeff Hammel]

(In reply to Andrew Halberstadt [:ahal] from comment #18)
> Comment on attachment 753536 [details] [diff] [review]
> remove manifestdestiny
> 
> Review of attachment 753536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, must have missed this review. Cool though! :)

Sorry, I meant to deflag for review considering the failures.  I may carry the r+ forward if the follow up passes try (to be posted)

Comment 20

[Jeff Hammel]

Attachment: as above, but with importing

Comment 21

[Jeff Hammel]

pushed to android try...lets see what happens

Comment 22

[Jeff Hammel]

https://tbpl.mozilla.org/?tree=Try&rev=8d42311614b0

Comment 23

[Jeff Hammel]

Attachment: forgot to delete original import

I still don't understand why the `import mozinfo` line works.  'Twould be nice to figure out what is being used from where...

Comment 24

[Jeff Hammel]

So ABICT the try run fails because of the mismatch of the directory name vs the module name of manifestdestiny/manifestparser.  I've done a quick hack to fix it.  Trying again

Comment 25

[Jeff Hammel]

Attachment: + quick hack

Comment 26

[Jeff Hammel]

Comment on attachment 758241 [details] [diff] [review]
+ quick hack

android only try works.... While I can't say I'm a fan (at all) of this pattern....its a hack o_O (That, Ideally, I would Love to Fix Soon.)

Any idea of what would give decent try coverage?

Comment 27

[Jonathan Griffin (:jgriffin)]

(In reply to Jeff Hammel [:jhammel] from comment #26)
> Comment on attachment 758241 [details] [diff] [review]
> + quick hack
> 
> android only try works.... While I can't say I'm a fan (at all) of this
> pattern....its a hack o_O (That, Ideally, I would Love to Fix Soon.)
> 
> Any idea of what would give decent try coverage?

To be safe, I'd run xpcshell tests on all platforms.

Comment 28

[Jeff Hammel]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=e7fc6c739fd7

Comment 29

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #25)
> Created attachment 758241 [details] [diff] [review]
> + quick hack

:ahal, btw, only the runxpcshelltests.py has been modified since your last review

Comment 30

[Andrew Halberstadt [:ahal]]

Comment on attachment 758241 [details] [diff] [review]
+ quick hack

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

Looks kind of weird, but r+ if it works. One thing I don't understand though.. Aki switched desktop unittests to use mozharness awhile back, so for the in production case this hack shouldn't be needed. For the running locally case aren't the modules already installed into a virtualenv? Can't mach just activate it before running the tests?

Comment 31

[Jeff Hammel]

(In reply to Andrew Halberstadt [:ahal] from comment #30)
> Comment on attachment 758241 [details] [diff] [review]
> + quick hack
> 
> Review of attachment 758241 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks kind of weird, but r+ if it works. One thing I don't understand
> though.. Aki switched desktop unittests to use mozharness awhile back, so
> for the in production case this hack shouldn't be needed. For the running
> locally case aren't the modules already installed into a virtualenv? Can't
> mach just activate it before running the tests?

The failures are on android, not desktop, hence the change.  See the try pushes and failures when tbpl becomes viewable again

Comment 32

[Jeff Hammel]

(P.S. I'm not particularly a fan of these changes either; I'd much rather use typical python practices as you suggest in comment 30 vs home-brewed roll-your-own craziness)

Comment 33

[Jeff Hammel]

From https://bugzilla.mozilla.org/show_bug.cgi?id=877733#c28 :

::: testing/xpcshell/runxpcshelltests.py
@@ +39,5 @@
> +            module_path = os.path.join(mozbase, dep)
> +            if module_path not in sys.path:
> +                sys.path.append(module_path)
> +        globals()[module] = __import__(module)
> +

there is a high chance we will append the same paths to the sys path if we allow the modules to grow.  Is there a need for this vs the original method?  Right now we have the original code in automation.py.in as well, it would be nice to keep the hack uniform across all instances.  I am fine fixing that in a separate bug though.

Attachment #759399 [details] [diff] - Flags: review?([e-mail]) → review+

Comment 34

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #33)
> From https://bugzilla.mozilla.org/show_bug.cgi?id=877733#c28 :
> 
> ::: testing/xpcshell/runxpcshelltests.py
> @@ +39,5 @@
> > +            module_path = os.path.join(mozbase, dep)
> > +            if module_path not in sys.path:
> > +                sys.path.append(module_path)
> > +        globals()[module] = __import__(module)
> > +
> 
> there is a high chance we will append the same paths to the sys path if we
> allow the modules to grow.  Is there a need for this vs the original method?
> Right now we have the original code in automation.py.in as well, it would be
> nice to keep the hack uniform across all instances.  I am fine fixing that
> in a separate bug though.
> 
> Attachment #759399 [details] [diff] [diff] - Flags: review?([e-mail]) →
> review+

So the reason I introduced the new way of doing things is due to the addition of the new need for an import of manifestdestiny based on locality rather than the singular import of just mozcrash. I could have copy-pasted code and used the same pattern, but the method i introduced, while hacky (although the whole thing is a hack), is extensible and shorter and more intent made manifest IMHO for those that care for such things.

Regarding the concern of appending the same paths to sys.path, it should not be an issue since I check for `if module_path not in sys.path`.

I'll file a follow-up to consolidate this hack, as per IRC discussion.

Comment 35

[Jeff Hammel]

pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d48e817041

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b7d48e817041