892098 - add argument to include updateKey in install.rdf

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect, P3)

Description

[johndoe_fakestreet]

Attachment: 0001-add-argument-to-include-updateKey-in-install.rdf.patch

User Agent: Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130511120803

Steps to reproduce:

if you hosting the update.rdf yourself, you have to put the updateKey into the install.rdf of the firefox addon. with this patch, you can provide the update-key as an argument and it's include in the install.rdf ( like --update-url):
cfx --update-key=MIGf.... xpi

Comment 1

[johndoe_fakestreet]

Attachment: add argument "--update-key" to include the updateKey into the install.rdf (without useless comments)

remove my useless comments from the patch

Comment 2

[Dave Townsend [:mossop]]

Is doing this through the command line the right option? Seems like it might be better put into package.json

Comment 3

[johndoe_fakestreet]

yeah, you're right. The updateKey isn't changed very often. I will submit a new patch for that.
But doesn't the same hold for the update-url? It's also a command line argument.

Comment 4

[Dave Townsend [:mossop]]

(In reply to johndoe_fakestreet from comment #3)
> yeah, you're right. The updateKey isn't changed very often. I will submit a
> new patch for that.
> But doesn't the same hold for the update-url? It's also a command line
> argument.

I think it does, I'm not sure why it was added to the command line but I think it makes more sense in package.json too.

Irakli does this make sense to you?

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Putting both in `package.json` makes sense. Unfortunately I can't recall reasons why cli argument was chosen in first place, probably that decision predates me.

Comment 6

[johndoe_fakestreet]

Attachment: 0001-add-keys-updateURL-and-updateKey-to-package.json.patch

Comment 7

[johndoe_fakestreet]

the patch removes the argument '--update-url' and adds the keys 'updateKey' and 'updateURL' to the package.json. I've also insert some docs to 'package-spec.md'.

Comment 8

[johndoe_fakestreet]

do you include my patch? Can I help in any way, so this will happen?

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 780562 [details] [diff] [review]
0001-add-keys-updateURL-and-updateKey-to-package.json.patch

Erik can you please take this one, trying to make sure I finish all pending task before my upcoming PTO

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 780562 [details] [diff] [review]
0001-add-keys-updateURL-and-updateKey-to-package.json.patch

The patch needs tests.  Let us know if you need/want help with that.

Comment 11

[johndoe_fakestreet]

Sure. I add tests, but it will take time until the end of next week, because I'm on holiday for some days.

Comment 12

[johndoe_fakestreet]

Attachment: adds a test for updateKey and updateURL to test_rdf.py

If you have comments or notes about things which I got wrong totally, please let me know.

Comment 13

[johndoe_fakestreet]

*ping, someone working on it?

Comment 14

[Dave Townsend [:mossop]]

Erik, what is the status here?

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend (:Mossop) from comment #14)
> Erik, what is the status here?

Oh I didn't know there was a change here, it looks the new patch needs a review.

Comment 16

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 804945 [details] [diff] [review]
adds a test for updateKey and updateURL to test_rdf.py

Ok it looks like this test is merely testing that our python function which generates the install.rdf will include the desired xml string (if the xml valid or not).  We need some more complete testing here, such that if the `updateURL` and `updateKey` keys are found in the package.json file then the generate install.rdf has the correct nodes.

One way to do this would be to check that the Addon.permissions https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/Add-on_Manager/Addon from the AddonManager https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/Add-on_Manager/AddonManager says that the addon is able to upgrade, or parse the install.rdf in the generated xpi for the updateURL/updateKey values.

I would suggest using a test addon like those found here https://github.com/mozilla/addon-sdk/tree/master/test/addons

Comment 17

[johndoe_fakestreet]

Attachment: 0001-add-unit-tests-for-keys-updateKey-and-updateURL-in-p.patch

here is my unit test patch for updateKey and updateURL.

First I tried to implement your suggestion and use AddonManger in firefox, but I noticed that the (permissions & AddonManager.PERM_CAN_UPGRADE) is always true, whether or not there is an em:updatKey and/or em:updateURL in the install.rdf. The same thing holds for 'providesUpdatesSecurely'.

So in the end I added an unit test to python-lib/cuddlefish/tests/test_linker.py. It seems to be the right place for tests which builds and checks the xpi.

Comment 18

[johndoe_fakestreet]

*ping

Comment 19

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to johndoe_fakestreet from comment #17)
> Created attachment 8341274 [details] [diff] [review]
> 0001-add-unit-tests-for-keys-updateKey-and-updateURL-in-p.patch
> 
> here is my unit test patch for updateKey and updateURL.
> 
> First I tried to implement your suggestion and use AddonManger in firefox,
> but I noticed that the (permissions & AddonManager.PERM_CAN_UPGRADE) is
> always true, whether or not there is an em:updatKey and/or em:updateURL in
> the install.rdf. The same thing holds for 'providesUpdatesSecurely'.
> 
> So in the end I added an unit test to
> python-lib/cuddlefish/tests/test_linker.py. It seems to be the right place
> for tests which builds and checks the xpi.

Hmm well there is news since last November, which is that we are working on killing as much python code as possible now.  So we'll need an addon test like the one I mentioned in comment 16.  I see the issue you mentioned in comment 17 now though, so I'm not sure how best to resolve this at the moment..

The good news is that in bug 915376 this bug will be resolved for free, so I think it'd be best to either close this bug as a dup of bug 915376 or make sure we resolve this bug with tests that will work after bug 915376 is resolved.

I'm sorry about the confusion here, it was difficult to get agreement on bug 915376 for sometime..

Comment 20

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8341274 [details] [diff] [review]
0001-add-unit-tests-for-keys-updateKey-and-updateURL-in-p.patch

So it looks like we'll need a test addon which will work after bug 915376 is resolved (see comment 19 for more info).  I think that this may work for a test addon:

Components.utils.import("resource://gre/modules/AddonManager.jsm");
AddonManager.getAddonByID(require('self').id, function(aAddon) {
  assert.ok(aAddon.updateKey);
});

Comment 21

[Erik Vold [:erikvold] (please needinfo? me)]

Sorry we won't be releasing any new versions of cfx, jpm is the replacement https://www.npmjs.com/package/jpm

Comment 22

[joscha]

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #21)
> Sorry we won't be releasing any new versions of cfx, jpm is the replacement
> https://www.npmjs.com/package/jpm

The release of a new version is not needed I think, but to ease the time until jpm is ready, could be put one of the patches in HEAD?