708309 - Don't use zipfile.extract() in mozinstall

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

This is only available in python 2.6+ and can't be run on some of the test slaves that are still running 2.5.

https://github.com/mozilla/mozbase/blob/master/mozinstall/mozinstall/mozinstall.py#L125

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - Don't depend on extractall

Note: tarfile.extractall does exist in Python 2.5

I was debating breaking this method up into a class, but it's still only 40 lines and the ZipFile class in automationutils.py seems kind of redundant to me.

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.1 - Don't depend on extractall

Sorry, just noticed I was doing:
if not os.path.exists(extdir):
    os.makedirs(extdir)
assert os.path.isdir(extdir)

which is silly.

Comment 3

[Jeff Hammel]

Comment on attachment 580051 [details] [diff] [review]
Patch 1.1 - Don't depend on extractall

+    elif not os.path.isdir(extdir):
+        os.makedirs(extdir)

What if extdir is a file?

+                    with open(filename, "wb") as dest:
i would prefer not to use with.  it does not exist in 2.4

     elif tarfile.is_tarfile(path):
         bundle = tarfile.open(path)
         namelist = bundle.getnames()
+        bundle.extractall(path=extdir)
This also doesn't exist in 2.4
http://docs.python.org/library/tarfile.html#tarfile.TarFile.extractall

r+ if you can address these issues.  I've been laboring under the assumption that mozbase supports python >= 2.4, but admittedly we have never decided this.  r-ing for now until these are addressed/decided upon

Comment 4

[Andrew Halberstadt [:ahal]]

So correct me if I'm wrong, but some of the Talos slaves are the only things running 2.4, and that's just by default (they also have 2.5 installed). Aki is setting up mozharness to point to all the 2.5 binaries when they aren't default.

This means that the only reason we would want to support 2.4 is to move Talos to mozbase. However, this in itself is a fairly substantial refactoring job that will likely disrupt the status quo. Wouldn't it make more sense to just update the build slaves when we update Talos?

If we still need to support 2.4, can we at least file a follow on? I'd really like to see peptest landed on try by the end of the quarter (to meet our goals) and seeing as there are only two weeks left, I don't want to block on 2.4 support.

Comment 5

[Jeff Hammel]

(In reply to Andrew Halberstadt [:ahal] from comment #4)
> So correct me if I'm wrong, but some of the Talos slaves are the only things
> running 2.4, and that's just by default (they also have 2.5 installed). Aki
> is setting up mozharness to point to all the 2.5 binaries when they aren't
> default.
> 
> This means that the only reason we would want to support 2.4 is to move
> Talos to mozbase. However, this in itself is a fairly substantial
> refactoring job that will likely disrupt the status quo. Wouldn't it make
> more sense to just update the build slaves when we update Talos?
> 
> If we still need to support 2.4, can we at least file a follow on? I'd
> really like to see peptest landed on try by the end of the quarter (to meet
> our goals) and seeing as there are only two weeks left, I don't want to
> block on 2.4 support.

Talking to ctalbert on IRC, he recommended supporting 2.4+ for Mozbase and I have noted it accordingly in the wiki: https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Overview  . I am fine taking with a followup, if desired, though I would strongly recommend taking out the with statement now. None of this is particularly challenging, just annoying. But that's development for you

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.2 - Compatible with Python 2.4

Addresses comments from last review

Comment 7

[Jeff Hammel]

Comment on attachment 580417 [details] [diff] [review]
Patch 1.2 - Compatible with Python 2.4

Looks good, thanks for fixing!

Comment 8

[Andrew Halberstadt [:ahal]]

master: https://github.com/mozilla/mozbase/commit/691e5627f51a84dbfd4538ee35b52f89e65c418e

I'm leaving this bug open because we'll need to update the mozilla-central copy so that it can run on the test slaves. Patch upcoming.

Comment 9

[Andrew Halberstadt [:ahal]]

Attachment: Patch 2.0 (m-c) - Update mozbase and peptest

Patch to update mozbase and peptest in mozilla central. Needed to test slaves with Python 2.5 can run the mozinstall code.

Comment 10

[cmtalbert]

Does this still need to land on m-c so that Aki can test his fixes to get peptest active on try?

Comment 11

[Aki Sasaki (not active)]

(In reply to Clint Talbert ( :ctalbert ) from comment #10)
> Does this still need to land on m-c so that Aki can test his fixes to get
> peptest active on try?

I can test linux64 (where I'm hitting https://bugzilla.mozilla.org/show_bug.cgi?id=700415#c32 ), but not the other platforms. It would be certainly easier to test if this were landed.

Comment 12

[cmtalbert]

Landed on m-c: https://hg.mozilla.org/mozilla-central/rev/300849c3dd10