Closed
Bug 794656
Opened 12 years ago
Closed 12 years ago
mozinstall should use mozfile.extract
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: k0scist, Assigned: ekw)
Details
(Whiteboard: [good first bug][mentor=jhammel][lang=py])
Attachments
(1 file, 2 obsolete files)
3.10 KB,
patch
|
k0scist
:
review+
|
Details | Diff | Splinter Review |
https://github.com/mozilla/mozbase/blob/master/mozinstall/mozinstall/mozinstall.py#L210 This function is unrelated to installing. Instead, mozfile.extract should be used: https://github.com/mozilla/mozbase/blob/master/mozfile/mozfile/mozfile.py#L53
Reporter | ||
Updated•12 years ago
|
Whiteboard: [good first bug][mentor=jhammel][lang=py]
Assignee | ||
Comment 1•12 years ago
|
||
This is my first patch to auto-tools. I followed directions on this page to create patch: https://wiki.mozilla.org/Auto-tools/Projects/MozBase Not sure how to test this patch (or even use mozbase as a whole), but this bug seems simple enough. Would be glad to test this patch if someone tells me how.
Reporter | ||
Comment 2•12 years ago
|
||
Comment on attachment 665800 [details] [diff] [review] patch This looks good. For the final patch, we'll want to add mozfile as a dependency to mozinstall (and bump mozinstall's version). I'm not sure about tests. I filed bug 796017 about mozinstall needing tests. However, I can't look at the issue right at the moment, but would encourage others to do so.
Attachment #665800 -
Flags: feedback?(jhammel) → feedback+
Assignee | ||
Comment 3•12 years ago
|
||
Is this how you add mozfile as a dependency to mozinstall and bump mozinstall's version?
Attachment #665800 -
Attachment is obsolete: true
Attachment #667323 -
Flags: review?(jhammel)
Reporter | ||
Comment 4•12 years ago
|
||
Comment on attachment 667323 [details] [diff] [review] patch It all looks good except changing the package version, see https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Versioning Undo that and I can do the version bump
Attachment #667323 -
Flags: review?(jhammel) → review-
Assignee | ||
Comment 5•12 years ago
|
||
Patch updated as specified in comment 4.
Attachment #667323 -
Attachment is obsolete: true
Attachment #667814 -
Flags: review?(jhammel)
Comment 6•12 years ago
|
||
Comment on attachment 667814 [details] [diff] [review] patch Review of attachment 667814 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozinstall/setup.py @@ +13,5 @@ > > PACKAGE_VERSION = '1.2' > > +deps = ['mozinfo==0.3.3', > + 'mozfile' You should specify a fixed version for mozfile here, similar to what we have the line above for mozinfo.
Reporter | ||
Comment 7•12 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #6) > Comment on attachment 667814 [details] [diff] [review] > patch > > Review of attachment 667814 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mozinstall/setup.py > @@ +13,5 @@ > > > > PACKAGE_VERSION = '1.2' > > > > +deps = ['mozinfo==0.3.3', > > + 'mozfile' > > You should specify a fixed version for mozfile here, similar to what we have > the line above for mozinfo. Since there is only one version of mozfile on pypi currently, and since its version is 0.0, and since no known API changes for mozfile.extract are planned at this time, and since we don't have a versioning policy for mozbase yet, this is probably not necessary
Reporter | ||
Comment 8•12 years ago
|
||
Comment on attachment 667814 [details] [diff] [review] patch lgtm; we can file a bug to bump the mozinstall version on landing; ewong3, do you have permission to land or should i push?
Attachment #667814 -
Flags: review?(jhammel) → review+
Assignee | ||
Comment 9•12 years ago
|
||
I don't have permission to land. Please push.
Reporter | ||
Comment 10•12 years ago
|
||
pushed: https://github.com/mozilla/mozbase/commit/6d3692a663081f30174390b53cdb18e8029b328c Thanks for the patch!
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•