797832 - [mozprofile] AddonManager.addon_details(path) should support XPI/JAR files

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

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

Given that none of our extensions will be unpacked anymore but used directly as XPI (except for those the author really doesn't want that) the addon_details() method should support XPI files directly.

That way the consumer code can directly specify the XPI file or path and both will work.

Comment 1

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

Code which would help us here could be:

addon_path = tempfile.mktemp('.addon')
zipfile.ZipFile(self.testrun.target_addon, "r").extractall(addon_path)
AddonManager.addon_details(addon_path)

But I do not think we really have to extract the XPI to get the install.rdf details.

Comment 2

[Johannes Vogel (:nebelhom)]

Hi,

I would like to try my hand on this. Before I add anything to the existing code though, how would I best test the code for correct functionality once I have done the changes?

thanks

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Thanks for volunteering to contribute! You will need to ensure that the existing tests still pass once you've made your changes. You will find a test that checks the addon ID [1], and you could modify this to also check when the addon provided is an xpi.

If you have any questions, you can find us on irc.mozilla.org in #ateam and #automation

[1] https://github.com/mozilla/mozbase/blob/master/mozprofile/tests/addonid.py

Comment 4

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

Sorry, I was away for a couple of days. So thank you Dave for stepping in. For the right repository and file see the URL field. Beside the existing tests we would have to also add one which exercises the new code path to ensure it works as expected.

Comment 5

[Johannes Vogel (:nebelhom)]

Attachment: First try

Ok,Here is my first try. It does what you described to me in IRC, so hopefully the direction is right.

This is a very rough first draft and I am sure it needs revising. In particular, I randomly chose the DuckDuckGo for firefox xpi and left it the way it is.

If you could tell me which parts are essential, I can strip it down to the bare bones, but I wanted to make sure, I got the entire idea right.

thanks for the assistance.

Comment 6

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

Comment on attachment 678786 [details] [diff] [review]
First try

Review of attachment 678786 [details] [diff] [review]:
-----------------------------------------------------------------

In general this already looks fantastic. Great work Johannes! Comments to the code I made inline. Beside that, I kinda would like to change the RDF content to the minimal amount of data we need. Some good test examples you can find here: https://ssl-ov.mozqa.com/data/firefox/addons/extensions/.

You get my f+ on that.

::: mozprofile/mozprofile/addons.py
@@ +136,4 @@
>                  if node.nodeType == node.TEXT_NODE:
>                      rc.append(node.data)
>              return ''.join(rc).strip()
> +        

nit: looks like you have introduced a whitespace issue here.

@@ +137,5 @@
>                      rc.append(node.data)
>              return ''.join(rc).strip()
> +        
> +        if addon_path.endswith('.xpi'):
> +            with zipfile.ZipFile(addon_path, 'r') as compressed_file:

In case of .jar files for themes we would fail here. Can we make use of zipfile.is_zipfile() instead?

::: mozprofile/tests/addonid.py
@@ +15,4 @@
>          f.write(filecontents)
>          f.close()
>          return path
> +    

nit: white-space

@@ +26,1 @@
>                  self.assertTrue(addon_id == "winning", "We got the addon id")

Would you mind to change this to assertEqual?

@@ +26,4 @@
>                  self.assertTrue(addon_id == "winning", "We got the addon id")
>              finally:
>                  shutil.rmtree(p)
> +              

nit: white-space again

@@ +29,5 @@
> +              
> +    def test_xip_unpack(self):
> +        a = addons.AddonManager("profile")
> +        addon = a.addon_details(os.path.join(os.getcwd(), "test.xpi"))
> +        self.assertNotEqual(addon['id'], None)

I would name the test method similar to the other test, e.g. test_addonID_xpi. Otherwise you will also have to create the xpi again because it get removed by the other test.

Comment 7

[Johannes Vogel (:nebelhom)]

Attachment: First revision

Ok, here is the revision to it. I've been wrestling with git for quite some time, but I am still not 100% certain that it did the right thing, therefore there may be errors.

A couple of things to mention in particular:

- whitespace nit:
It shows up in my patch again, although I made sure to remove any automatic indentation that has been automatically added by my editor. I am stumped to the two that you see in this patch.

- xpi file
I used the empty.xpi file from the link you provided ( https://ssl-ov.mozqa.com/data/firefox/addons/extensions/ ) and didn't bother to change the name. I hope that is alright. It is a fairly descriptive name :P
Furthermore, this time I 'really' hope that the patch noticed the file correctly. If not, I really don't know anymore (Oh, Git, you constant guest in my nightmares) ;)

I hope that helps.

Johannes

Comment 8

[Johannes Vogel (:nebelhom)]

Attachment: First revision

Made a mistake. sorry

Comment 9

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

Comment on attachment 680725 [details] [diff] [review]
First revision

Review of attachment 680725 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, everything alright that way. There are two things why you can't get my r+. See below.

::: mozprofile/tests/addonid.py
@@ +28,5 @@
>                  shutil.rmtree(p)
>  
> +    def test_addonID_xpi(self):
> +        a = addons.AddonManager("profile")
> +        addon = a.addon_details(os.path.join(os.getcwd(), "empty.xpi"))

You missed to add this file to your branch. Do it via 'git add %path%'.

@@ +29,5 @@
>  
> +    def test_addonID_xpi(self):
> +        a = addons.AddonManager("profile")
> +        addon = a.addon_details(os.path.join(os.getcwd(), "empty.xpi"))
> +        self.assertNotEqual(addon['id'], None)

You should use assertEqual() and check for the id which you can find in the install.rdf of the empty extension.

Comment 10

[Johannes Vogel (:nebelhom)]

Attachment: Next Revision

this time used the command.

git diff master --binary > %patchfilepath%

if this still doesn't work, then I'm out of ideas why my setup doesn't want to do what it is supposed to do :(

Anyways, I hope this is ok, now.

Comment 11

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

Comment on attachment 683055 [details] [diff] [review]
Next Revision

Review of attachment 683055 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like you have attached the wrong patch.

Comment 12

[Johannes Vogel (:nebelhom)]

Attachment: Next Revision

Comment 13

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

Comment on attachment 684504 [details] [diff] [review]
Next Revision

Review of attachment 684504 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Next time I would like to see a commit message too. But don't worry about it for now. I will get this landed in a bit.

Comment 14

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

Johannes, I have problems applying your patch because of the binary diff for empty.xpi. Would you mind to attach that file to that bug instead? Thanks!

Comment 15

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

Comment on attachment 684504 [details] [diff] [review]
Next Revision

Review of attachment 684504 [details] [diff] [review]:
-----------------------------------------------------------------

The reason why I can't apply it is that the binary file has been changed and is not an addition. Lets talk on IRC how we can best create a diff!

Comment 16

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

Johannes, I hope we can figure out the remaining problem soon so we can get this landed and released. Thanks.

Comment 17

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

Attachment: patch v2

Given that Johannes wasn't able to complete the patch he sent me all modifications. This is all what he implemented and some additional tweaks from myself to make everything pass.

Comment 18

[Jeff Hammel]

Comment on attachment 690408 [details] [diff] [review]
patch v2

I very much hesitate to allow the use of `with` in mozprofile or any of mozbase:  while we want to be on python 2.7, we're not.  That said, the rest of the patch looks good.  If it causes problem in production, we can fix.

Comment 19

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

Johannes, would you have time to fix this last remaining issue so we can get this checked-in?

Comment 20

[Johannes Vogel (:nebelhom)]

Attachment: Trial number bazillion

Hi Henrik,

here is another try. I did it now using a fork and creating the diff from the branch, as you suggested. If this works, I have finally managed to attach a file correctly to a patch using git (!!!).

Let's hope so. This way of doing it seemed a bit more logical to me ...and github has lovely documentation.

P.S. Sorry for the rebase stupidity... Well, you should be used to it from me by now (unfortunately)

Comment 21

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

(In reply to Jeff Hammel [:jhammel] from comment #18)
> I very much hesitate to allow the use of `with` in mozprofile or any of
> mozbase:  while we want to be on python 2.7, we're not.  That said, the rest
> of the patch looks good.  If it causes problem in production, we can fix.

Wait. What is the lowest version of Python we have to support? Is it still v2.4? If not the 'with' statement has been added with Python 2.5.

I wanted to ask that first before asking Johannes to update the patch. Sorry that I missed that.

Comment 22

[Jeff Hammel]

IMHO, we should support python 2.4 until we don't. However, our official policy is at https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Overview : "Mozbase requires python 2.5, 2.6, or 2.7 however, Talos still requires compatibility with python 2.4 (bug 734466), so mozbase dependencies in http://hg.mozilla.org/build/talos/file/tip/setup.py *must* be kept compatible with python 2.4 ".

To unwrap: mozprofile is not (yet) used by Talos.  This means that, technically, you can break python 2.4 compatability although I would discourage doing so.

Comment 23

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

Comment on attachment 698733 [details] [diff] [review]
Trial number bazillion

Review of attachment 698733 [details] [diff] [review]:
-----------------------------------------------------------------

If you compare my latest patch and yours you can see that this patch misses some files. You might have forgotten to fully rebase against master? Shall I finish it up Johannes?

::: mozprofile/mozprofile/addons.py
@@ +104,1 @@
>          

nit: would you mind to get this white-space issue fixed? Looks like an additional tab is in here.

@@ +140,5 @@
> +        if zipfile.is_zipfile(addon_path):
> +            compressed_file = zipfile.ZipFile(addon_path, 'r')
> +            parseable = compressed_file.read('install.rdf')
> +            doc = minidom.parseString(parseable)
> +            compressed_file.close()

How do we handle the failure case? Do we close the XPI file if parseString() or read() throwing an error? I suspect we don't do it.

::: mozprofile/tests/addonid.py
@@ +13,3 @@
>  class AddonIDTest(unittest.TestCase):
>      """ Test finding the addon id in a variety of install.rdf styles """
>      

nit: another white-space issue to fix

Comment 24

[Johannes Vogel (:nebelhom)]

> Shall I finish it up Johannes?

If it isn't too much hassle, please do so. This is becoming a never-ending story that is not funny anymore. It would be very annoying if it bitrotted in the end.

Regarding the whitespace: I have no idea where this one could come from as I did it last time from scratch, when I transferred the changes to the fork that I created.

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Updated patch, with all suggested changes.

Taking this as it's been a while since we had an update.

(In reply to Johannes Vogel (:nebelhom) from comment #24)
> > Shall I finish it up Johannes?
> 
> If it isn't too much hassle, please do so. This is becoming a never-ending
> story that is not funny anymore. It would be very annoying if it bitrotted
> in the end.

Sorry for the difficulties you have been experiencing Johannes. Your contributions are very appreciated, and I hope they continue. :)

> Regarding the whitespace: I have no idea where this one could come from as I
> did it last time from scratch, when I transferred the changes to the fork
> that I created.

I suspect these whitespace issues pre-existed, and you didn't introduce them yourself. When reviewing via Bugzilla, trailing whitespaces are highlighted so it's easy to pick these up during review.

Comment 26

[Johannes Vogel (:nebelhom)]

@davehunt: 

> Sorry for the difficulties you have been experiencing Johannes. Your contributions are very 
> appreciated, and I hope they continue. :)

No need to apologise. I just don't want it to go to waste. If that means that I step aside in that case, then so be it ;) I just left it as is, because I thought I would make it worse :P I apologise if I misread the intention. and thanks for finishing it off.

Comment 27

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

Comment on attachment 714165 [details] [diff] [review]
Updated patch, with all suggested changes.

Review of attachment 714165 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. There is only one nit we should fix before landing. r=me with the change.

::: mozprofile/tests/addonid.py
@@ +33,5 @@
>  
> +    def test_addonID_xpi(self):
> +        a = addons.AddonManager("profile")
> +        addon = a.addon_details(os.path.join(here, "addons", "empty.xpi"))
> +        self.assertNotEqual(addon['id'], None)

Please modify so we check for the correct id of the addon similar to the above test.

Comment 28

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Updated patch, with all suggested changes.

Updated with the id check, also fixed a couple of conflicts so re-requesting review.

Comment 29

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

Comment on attachment 725361 [details] [diff] [review]
Updated patch, with all suggested changes.

Review of attachment 725361 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Please land it so we can continue with the remaining addons testrun. Thanks!

Comment 30

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as:
https://github.com/mozilla/mozbase/commit/a84eb3b09c375f5e3b83301e1dc0ae72c3ef6988