(addon-sdk)│cfx xpi Error: the name of your package contains upper-case letters. Package names can contain only lower-case letters, numbers, underscores, and dashes. Current package name: 0-0 (Pdb) name u'0-0' (Pdb) name.islower() False (Pdb) '12345'.islower() False (Pdb) 'a12345'.islower() True So I'm not sure if this should be an allowable package name or not, but in any case the error message is misleading as '0-0' contains nothing but numbers and a dash
Hrm. This page says that "It return true if all cased characters in the string are lowercase and there is at least one cased character, false otherwise"... http://www.tutorialspoint.com/python/string_islower.htm If that's true, then you HAVE to have at least one letter in there to pass that test...
From my (really quick) testing (generating the xpi with the package name "m0-0", then replacing every instance of "m0-0" with "0-0" within the XPI), everything seems to work correctly. The extension is installed, and the code that's generated via cfx init all appears to work. So I'd guess this is just a weird edge case where islower() is being too strict?
Yep. I just happened across it naming addons '%d-%d' % (0, 0) for testing. No big deal, now I'm naming them 'a%d-%d' % (0,0) but thought the error message should be fixed.
The real test should probably be name.lower()==name, since the reason for the restriction is that the name gets used in the hostname-ish part of a resource:// URI, and (inappropriately, in my opinion) it gets downcased when used. (The "authority" portion of a URL maps to a DNS name for many protocols, but resource:// doesn't need to follow that lead. Also, DNS lookups are case-insensitive, but that doesn't mean you have to downcase the string before handing it to your resolver library). I vaguely recall doing some experiments that showed upper-case letters in the addon name getting used to register the resource:// protocol handler, but then get downcased any time the loader attempted to fetch anything, resulting in the XPI's contents being unretrievable. The secondary concern was name collisions between addons that only differ in case. So forbidding uppercase seemed like the easiest answer.
Created attachment 593480 [details] [diff] [review] name.lower() == name So, something like this?
Attachment #593480 - Flags: feedback?(warner-bugzilla)
Actually, looking at the code, we can delete that whole clause. There's a RESOURCE_HOSTNAME_RE.match() just afterwards which will only match on lower-case letters, so the extra islower() check is redundant. And the doctests just before it should provide the right test coverage. So let's just delete that first "if" statement.
Comment on attachment 593480 [details] [diff] [review] name.lower() == name Ah, Wes pointed out that the two checks have different error messages: the first one specifically mentions upper-case characters, while the regexp one just says "invalid character". Given that upper-case characters are pretty likely (it's not particularly surprising that "&" is illegal, but there's no good reason for "A" to be disallowed), having the separate message is a good idea. So yeah, that patch looks great. The only thing I'd change is to make it "if name.lower() != name", as that reads a bit better.
Attachment #593480 - Flags: feedback?(warner-bugzilla) → feedback+
Comment on attachment 593480 [details] [diff] [review] name.lower() == name looks good! I'd still suggest using "if name.lower() != name" instead of "if not name.lower() == name", though.
Attachment #593480 - Flags: review?(warner-bugzilla) → review+
Wes did this ever land?
Doesn't look like it did. https://github.com/mozilla/addon-sdk/blob/master/python-lib/cuddlefish/packaging.py#L87
Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/d25827ff7cbfa61426ebac7ef4843a3f4256b97c Bug 722989 - 0-0 (etc) not a valid package name? if so the message is misleading r=@warner
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Created attachment 8536658 [details] [review] Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1753
Attachment #8536658 - Flags: review?(rFobic)
This needs a test case to make sure we don't regress.
Assignee: nobody → evold
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8536658 [details] [review] Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1753 I don't think we should support package names that are not supported by npm, which is the case here as far as I know.
Attachment #8536658 - Flags: review?(rFobic) → review-
Comment on attachment 8536658 [details] [review] Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1753 There is a package called 5 on npm https://www.npmjs.com/package/5
Attachment #8536658 - Flags: review- → review?(rFobic)
Attachment #8536658 - Flags: review?(rFobic) → review?(jsantell)
Comment on attachment 8536658 [details] [review] Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1753 Looks like a valid name
Attachment #8536658 - Flags: review?(jsantell) → review+
Commits pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/4374a99cd1c1ddc5ed37487411a6b1e2c1fe0b51 Bug 722989 adding tests for names without letters https://github.com/mozilla/addon-sdk/commit/46029f09b2c4bd079b0c51bc967bbc69c5bee961 Merge pull request #1753 from erikvold/722989 Bug 722989 adding tests for names without letters r=jsantell
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago → 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.