1091269 - Replace automationutils:ZipFileReader with zipfile.extractall

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jonathan Griffin (:jgriffin)]

automationutils:ZipFileReader is used in only one file, automation.py.in.  We should move it there.

Comment 1

[William Lachance (:wlach)]

I believe the zipfilereader class was mainly used because extractall wasn't in older versions of python that we were using a few years ago. So we might just be able to chuck it wholesale by using extractall directly:

https://docs.python.org/2/library/zipfile.html#zipfile.ZipFile.extractall

Comment 2

[Jonathan Griffin (:jgriffin)]

Even better!

Comment 3

[Prashant Baisla]

I'd like to do this.
Can someone tell me how to get started?

Comment 4

[Amit Tripathi[:amit001]]

I want to work on this bug.
Please assign it to me.

Comment 5

[Jonathan Griffin (:jgriffin)]

At http://dxr.mozilla.org/mozilla-central/source/build/automation.py.in#936, we currently use ZipFileReader from automationutils.py.  Instead, we'd prefer to use Python's built-in ZipFile class.

Comment 6

[Andrew Halberstadt [:ahal]]

Hi Prashant and Amit, thanks for the interest! We can't have you both working on this at the same time, and since Prashant asked first I'll assign it to them.

Prashant, please let jgriffin or I know if you have any questions!

Amit, take a look at bug 1091280 and add a comment over there if you are interested in trying it.

Comment 7

[Prashant Baisla]

Thanks Andrew!
Could you tell me how to set up a development environment for this? And how I should submit the patch? I've never used mercurial before.

Comment 8

[Prashant Baisla]

I need some help in getting started. Can someone tell me how to begin?

Comment 9

[Andrew Halberstadt [:ahal]]

Hi Prashant, sorry for missing your comment last month. One trick you can do to make sure a person sees a comment is to flag them for needinfo. To do that you can check the 'Need more information from' box below and enter in the person's name.

For this bug you'll need the main repo called 'mozilla-central'. To clone it see:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial

Once you have the source, the two files you'll need to modify are 'build/automationutils.py' and 'build/automation.py.in' (see the user story above for more details).

In terms of working with mercurial, there are many different workflows that people use. Some people use mq, others use bookmarks. Probably the simplest way of managing patches to start out with is mq (though bookmarks are the newer way of working):
https://developer.mozilla.org/en-US/docs/Mercurial_Queues

Comment 10

[William Lachance (:wlach)]

I'm pretty sure we should be able to just use zipfile.extractall here, we should definitely try that first, at least.

Comment 11

[Martijn Wargers (dead)]

(In reply to Andrew Halberstadt [:ahal] from comment #9)
> Hi Prashant, sorry for missing your comment last month. One trick you can do
> to make sure a person sees a comment is to flag them for needinfo. To do
> that you can check the 'Need more information from' box below and enter in
> the person's name.

That's really something I don't like about the needinfo? flag, it creates another barrier of entry for newcomers, who ask questions in bugs and don't get any response, because developers don't look at regular bugmail anymore (because they are overwhelmed, I understand).

Comment 12

[Prashant Baisla]

Attachment: bug-1091269-fix.patch

I've removed the ZipFileReader class from automationutils.py and changed automation.py.in to use zipfile.extractall

Comment 13

[Jonathan Griffin (:jgriffin)]

Pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7282258c0e8d

Comment 14

[Jonathan Griffin (:jgriffin)]

Second try run with a different test selection:  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ed7a300112dd

Comment 15

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8539686 [details] [diff] [review]
bug-1091269-fix.patch

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

try runs look good, thanks for the patch!

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ba4af8531c9

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ba4af8531c9