Closed
Bug 722989
Opened 12 years ago
Closed 9 years ago
0-0 (etc) not a valid package name? if so the message is misleading
Categories
(Add-on SDK Graveyard :: General, defect, P3)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: k0scist, Assigned: evold)
Details
Attachments
(2 files)
(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?
Reporter | ||
Comment 3•12 years ago
|
||
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.
Comment 4•12 years ago
|
||
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.
So, something like this?
Attachment #593480 -
Flags: feedback?(warner-bugzilla)
Comment 6•12 years ago
|
||
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 7•12 years ago
|
||
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+
Attachment #593480 -
Flags: review?(warner-bugzilla)
Priority: -- → P3
Comment 8•12 years ago
|
||
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+
Doesn't look like it did. https://github.com/mozilla/addon-sdk/blob/master/python-lib/cuddlefish/packaging.py#L87
Flags: needinfo?(kwierso)
Comment 11•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8536658 -
Flags: review?(rFobic)
Assignee | ||
Comment 13•9 years ago
|
||
This needs a test case to make sure we don't regress.
Assignee: nobody → evold
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 14•9 years ago
|
||
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-
Assignee | ||
Comment 15•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8536658 -
Flags: review?(rFobic) → review?(jsantell)
Comment 16•9 years ago
|
||
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+
Comment 17•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•