609333 - mozrunner uses external ilbraries to parse extension information when it should use xml.dom

Status: VERIFIED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

currently, mozrunner uses elementtree to install addons:

https://github.com/mozautomation/mozmill/blob/hotfix-1.5.1/mozrunner/mozrunner/__init__.py#L200

This causes complications when supporting multiple versions:

https://github.com/mozautomation/mozmill/blob/hotfix-1.5.1/mozrunner/mozrunner/__init__.py#L200

Talos uses borrowed elements of mozrunner for its profile code that does not require external dependencies in any python version > 2:

http://hg.mozilla.org/build/talos/file/ce9b0413fbf1/ffsetup.py#l119

http://docs.python.org/library/xml.dom.minidom.html

Since we support 2.4 and up, we should use this solution

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/02 - 10/06)]

Personally I think we should take care of it in Mozmill 1.5.2. We don't have tests for this area and changing this code could be risky. IMO we are too late in the cycle for such a change. The current solution works and is not a regression.

Comment 2

[Jeff Hammel]

(In reply to comment #1)
> Personally I think we should take care of it in Mozmill 1.5.2. We don't have
> tests for this area and changing this code could be risky. IMO we are too late
> in the cycle for such a change. The current solution works and is not a
> regression.

This is required for buildbot -- see e.g. http://tools-staging-master.mv.mozilla.com:8010/builders/Rev3%20MacOSX%20Leopard%2010.5.8%20mozilla-central%20debug%20test%20mozmill-all/builds/6/steps/install%20mozmill/logs/stdio

I have no idea why this wasn't spotted before.  I've been told that the windows slaves still use python 2.4, so pip was probably illegally hitting net.  Again, no clue how this ever worked on buildbot + python 2.4

Comment 3

[Jeff Hammel]

Attachment: use xml.minidom to find addon ids

This uses xml.minidom ala talos (http://hg.mozilla.org/build/talos/raw-file/ce9b0413fbf1/ffsetup.py#l119) ; i've restructured addon_id to be its own method, so that it is easier to test.  Currently, I've only tested by hand:  both mozmill and jsbridge work and the current addons i have for FF and thunderbird, but this is a set of about 8 out of millions. So I'm not sure how general purpose this is, but it should be as good as what talos does.  We might want to do more rigorous testing down the line

I'm not married to addon_id being a classmethod, though I'd rather see it be a classmethod, staticmethod, or standalone entirely, as it doesn't need any class data

Comment 4

[cmtalbert]

Comment on attachment 487994 [details] [diff] [review]
use xml.minidom to find addon ids

Looks good.  This is exactly the code we use on Talos which has downloaded and tested all available addons for talos addon testing, so I'm pretty confident it will work.
R+
Clint

Comment 5

[Jeff Hammel]

pushed to hotfix-1.5.1: https://github.com/mozautomation/mozmill/commit/e3faa5d70bf85f15c9af78f004d2219dd1cf2458

needs to go to master and m-c; will update here

Comment 6

[Jeff Hammel]

pushed to master:

https://github.com/mozautomation/mozmill/commit/5dfade5bb88db77fc261f03a1d370086de7f8bb4

Comment 7

[Jeff Hammel]

Attachment: adds xml.minidom fix to m-c which is (probably) necessary for buildbot

this ports the bug to an m-c patch;  also, it corrects installmozmill.py to display python version and exit with the correct code on failure

Comment 8

[cmtalbert]

Landed no m-c http://hg.mozilla.org/mozilla-central/rev/126e28ef841d

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/02 - 10/06)]

Works great. Marking as verified fixed.