Closed Bug 609333 Opened 12 years ago Closed 12 years ago
mozrunner uses external ilbraries to parse extension information when it should use xml
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
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.
(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
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
Assignee: nobody → jhammel
Attachment #487994 - Flags: review?(ctalbert)
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
Attachment #487994 - Flags: review?(ctalbert) → review+
pushed to hotfix-1.5.1: https://github.com/mozautomation/mozmill/commit/e3faa5d70bf85f15c9af78f004d2219dd1cf2458 needs to go to master and m-c; will update here
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
Attachment #488203 - Flags: review?(ctalbert)
Landed no m-c http://hg.mozilla.org/mozilla-central/rev/126e28ef841d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Works great. Marking as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.