794656 - mozinstall should use mozfile.extract

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

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

Comment 1

[Eric Wong (:ekw)]

Attachment: 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.

Comment 2

[Jeff Hammel]

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.

Comment 3

[Eric Wong (:ekw)]

Attachment: patch

Is this how you add mozfile as a dependency to mozinstall and bump mozinstall's version?

Comment 4

[Jeff Hammel]

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

Comment 5

[Eric Wong (:ekw)]

Attachment: patch

Patch updated as specified in comment 4.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 7

[Jeff Hammel]

(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

Comment 8

[Jeff Hammel]

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?

Comment 9

[Eric Wong (:ekw)]

I don't have permission to land.  Please push.

Comment 10

[Jeff Hammel]

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

Thanks for the patch!