mozinstall should use mozfile.extract

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jeff Hammel, Assigned: ekw)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=jhammel][lang=py])

Attachments

(1 attachment, 2 obsolete attachments)

3.10 KB, patch
Jeff Hammel
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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

5 years ago
Whiteboard: [good first bug][mentor=jhammel][lang=py]
(Assignee)

Comment 1

5 years ago
Created attachment 665800 [details] [diff] [review]
patch

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.
Assignee: nobody → ewong3
Status: NEW → ASSIGNED
Attachment #665800 - Flags: feedback?(jhammel)
(Reporter)

Comment 2

5 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

5 years ago
Created attachment 667323 [details] [diff] [review]
patch

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

5 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

5 years ago
Created attachment 667814 [details] [diff] [review]
patch

Patch updated as specified in comment 4.
Attachment #667323 - Attachment is obsolete: true
Attachment #667814 - Flags: review?(jhammel)
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

5 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

5 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

5 years ago
I don't have permission to land.  Please push.
(Reporter)

Comment 10

5 years ago
pushed: https://github.com/mozilla/mozbase/commit/6d3692a663081f30174390b53cdb18e8029b328c

Thanks for the patch!
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.