Closed Bug 708309 Opened 8 years ago Closed 8 years ago

Don't use zipfile.extract() in mozinstall

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ahal, Assigned: ahal)

References

Details

(Whiteboard: [mozbase])

Attachments

(2 files, 2 obsolete files)

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
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
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.
Attachment #580048 - Flags: review?(jhammel)
Sorry, just noticed I was doing:
if not os.path.exists(extdir):
    os.makedirs(extdir)
assert os.path.isdir(extdir)

which is silly.
Attachment #580048 - Attachment is obsolete: true
Attachment #580048 - Flags: review?(jhammel)
Attachment #580051 - Flags: review?(jhammel)
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
Attachment #580051 - Flags: review?(jhammel) → review-
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.
Assignee: ahalberstadt → nobody
Component: Mozmill → Mozbase
QA Contact: mozmill → mozbase
Whiteboard: [mozbase]
(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
Assignee: nobody → ahalberstadt
Addresses comments from last review
Attachment #580051 - Attachment is obsolete: true
Attachment #580417 - Flags: review?(jhammel)
Comment on attachment 580417 [details] [diff] [review]
Patch 1.2 - Compatible with Python 2.4

Looks good, thanks for fixing!
Attachment #580417 - Flags: review?(jhammel) → review+
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.
Patch to update mozbase and peptest in mozilla central. Needed to test slaves with Python 2.5 can run the mozinstall code.
Does this still need to land on m-c so that Aki can test his fixes to get peptest active on try?
(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.
Landed on m-c: https://hg.mozilla.org/mozilla-central/rev/300849c3dd10
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.