561503 - cfx-generated XPI rejected by AMO

Status: RESOLVED FIXED

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

Description

[Myk Melez [:myk] [@mykmelez]]

A XPI generated by cfx is rejected by AMO with the following errors:

--------------------------------------------------------------------------------
- "The add-on failed a validation test: Add-ons cannot use an external
updateURL. Please remove this from install.rdf and try again."
- "The add-on failed a validation test: The following errors were
found in install.rdf:
   3.7.* is not a valid version for Firefox<br>1.* is not a valid
version for Fennec
   3.1.* is not a valid version for Thunderbird
   Please see <a href="https://addons.mozilla.org/en-US/firefox/pages/
appversions">this page</a> for reference."
- "Please check that the following fields are valid and not blank:
homepage"
--------------------------------------------------------------------------------

Since the point of the SDK is to make it possible to create a XPI and distribute it on AMO, we should make sure that the XPIs created by cfx are accepted by AMO.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Erik Vold encountered this when he tried to upload his View Page Source in Tab addon <https://addons.mozilla.org/en-US/firefox/addon/151809> to AMO.  He fixed it via the following changes to the XPI's install.rdf file:

--------------------------------------------------------------------------------15,33c15
-         <em:maxVersion>3.7.*</em:maxVersion>
-       </Description>
-     </em:targetApplication>
-
-     <!-- Fennec -->
-     <em:targetApplication>
-       <Description>
-         <em:id>{a23983c0-fd0e-11dc-95ff-0800200c9a66}</em:id>
-         <em:minVersion>0.1</em:minVersion>
-         <em:maxVersion>1.*</em:maxVersion>
-       </Description>
-     </em:targetApplication>
-
-     <!-- Thunderbird -->
-     <em:targetApplication>
-       <Description>
-         <em:id>{3550f703-e582-4d05-9a08-453d09bdfdc6}</em:id>
-         <em:minVersion>3.0b3</em:minVersion>
-         <em:maxVersion>3.1.*</em:maxVersion>
---
+         <em:maxVersion>3.6.*</em:maxVersion>
42d23
-     <em:homepageURL/>
44d24
-     <em:updateURL/>
--------------------------------------------------------------------------------

Given that the SDK works on Firefox 3.7 pre-release builds, and early Jetpack users are likely to be using those builds, we should probably specify compatibility with them by setting maxVersion to the latest version that AMO accepts (currently 3.7a5pre).

I'm not sure what we should do about Thunderbird and Fennec, however, perhaps remove them until we have better support for determining compatibility across multiple applications?

Atul: what do you think?

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Atul, Myk: what do you think to remove updateURL from install.rdf patching cuddlefish/rdf.py?

I'm attaching my little proposed patch on cuddlefish/rdf.py.

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: remove em:updateURL element from install.rdf by default (leave/set it if specified)

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: remove em:updateURL and em:homepageURL elements from install.rdf if not specified

updated to handle homepageURL too

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: temporarily comments out Fennec/Thunderbird compatibility spec

And here's a patch that comments out the Fennec/Thunderbird compatibility specification, since we don't currently have a good way to determine whether or not an addon is compatible with those applications (i.e. it doesn't use any Firefox-only APIs like context menu).

Comment 6

[Atul Varma [:atul]]

Hmm, if we're breaking support for Fennec/TB in this release, we should probably remove it from the SDK docs, since the SDK docs says you can use the tutorial with Thunderbird, IIRC.

FWIW, this is one of my concerns w/ putting everything in jetpack-core; a good dilineation strategy for figuring out whether things go in jetpack-core vs. another package, IMO, is that everything in jetpack-core should pass all tests regardless of the host XULRunner app.  But I guess we can sort this out post-0.3, as long as we change the documentation to reflect the current state of things...

Comment 7

[Atul Varma [:atul]]

Also, thanks for the patch Luca!

Also, sorry I didn't mention it in my last comment, but removing the Fennec/TB support from the install.rdf template effectively prevents cfx from running on Fennec/TB--that is, you can't even run 'cfx test -a fennec' or 'cfx test -a thunderbird' on any package anymore. If we do accept this patch, we should revert it as soon as we release 0.3, lest we rot support for those apps.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

> Hmm, if we're breaking support for Fennec/TB in this release, we should
> probably remove it from the SDK docs, since the SDK docs says you can use the
> tutorial with Thunderbird, IIRC.

Would it be preferable (lower-risk, lest breaky) to instead update the max version strings for Fennec and Thunderbird such that they are acceptable by AMO?


> FWIW, this is one of my concerns w/ putting everything in jetpack-core; a good
> dilineation strategy for figuring out whether things go in jetpack-core vs.
> another package, IMO, is that everything in jetpack-core should pass all tests
> regardless of the host XULRunner app.  But I guess we can sort this out
> post-0.3, as long as we change the documentation to reflect the current state
> of things...

Yup, let's sort this out post-0.3.


> Also, sorry I didn't mention it in my last comment, but removing the Fennec/TB
> support from the install.rdf template effectively prevents cfx from running on
> Fennec/TB--that is, you can't even run 'cfx test -a fennec' or 'cfx test -a
> thunderbird' on any package anymore. If we do accept this patch, we should
> revert it as soon as we release 0.3, lest we rot support for those apps.

If we're going to revert the patch right after releasing, then it'd be better to branch and make the change only on the branch.  Alternately, we can instead not make the change in the first place, but then we need an alternate strategy like the one I suggest above.

Comment 9

[Atul Varma [:atul]]

Yeah, it would be much more preferable (IMO) to update the max-version strings for Fennec and TB. That would make this patch something we could keep for the long-term, rather than revert as soon as releasing 0.3 (or branching).

Comment 10

[Atul Varma [:atul]]

Comment on attachment 441407 [details] [diff] [review]
remove em:updateURL and em:homepageURL elements from install.rdf if not specified

Looks great, thanks Luca!

Comment 11

[Atul Varma [:atul]]

Pushed Luca's patch:

Bug 561503 - cfx-generated XPI rejected by AMO
Luca Greco
http://hg.mozilla.org/labs/jetpack-sdk/rev/87212c2367229e09ca59d9819d32da625c2cadab

Myk, can you push another that fixes the TB/Fennec maxVersion issue?

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch that only changes the max version values to AMO-accepted values

Here's a patch that just changes the max version values for the three supported applications to values that AMO accepts.

Comment 13

[Atul Varma [:atul]]

Comment on attachment 441643 [details] [diff] [review]
patch that only changes the max version values to AMO-accepted values

Looks great, thanks Myk!

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Pushed my change: https://hg.mozilla.org/labs/jetpack-sdk/rev/7fe9b842ec2a.

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Submitting an extension I've created with jetpack-sdk for Public Nomination on AMO, it was rejected with the following reason:

"We can't allow your add-on to set global preferences like enabling dump and strict JS warnings. Please remove them from your release version."

I'm attaching a little patch on 'cuddlefish/xpi.py' to remove template's preferences from the generated xpi.

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: remove debugging preferences from the generated xpi

Comment 17

[Atul Varma [:atul]]

Heh, clever fix!

My only concern with this is that when traditional XPI developers are supplying their own templates for extension directory structures, will their legitimate preference defaults be left out of the generated XPI?  It's fine that the default template's prefs are left out, since they're only for development, but it could result in confusing behavior for custom ones.

Since this is a blocker for release of 0.3, maybe we should just push this patch now and deal with the above issue later?

Comment 18

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #17)
> Since this is a blocker for release of 0.3, maybe we should just push this
> patch now and deal with the above issue later?

I'm ok with that as a short term solution if we don't have a better fix yet.

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to comment #17)
> Heh, clever fix!

;-)

> My only concern with this is that when traditional XPI developers are supplying
> their own templates for extension directory structures, will their legitimate
> preference defaults be left out of the generated XPI?  It's fine that the
> default template's prefs are left out, since they're only for development, but
> it could result in confusing behavior for custom ones.
> 
> Since this is a blocker for release of 0.3, maybe we should just push this
> patch now and deal with the above issue later?

I agree... I tried to find a minimal fix 'cause this could block the release,
and for the next release I can work on bug 559306 and copy preference defaults
from the main jetpack dir into the xpi with minimal effort.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Fixed by changeset https://hg.mozilla.org/labs/jetpack-sdk/rev/6884e54fd6a1.

Comment 21

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".