The extension name field in package.json cannot contain periods

RESOLVED FIXED in 1.0

Status

Add-on SDK
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Jeff Griffiths, Assigned: Jeff Griffiths)

Tracking

unspecified

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_7) AppleWebKit/534.24 (KHTML, like Gecko) Chrome/11.0.696.68 Safari/534.24
Build Identifier: 

If the extension name in package.json contains a period character ( eg 'fire.fox' ) cfx run crashes.



Reproducible: Always

Steps to Reproduce:
1. mkdir fire.fox && cd ./fire.fox
2. cfx init
3. cfx run

Actual Results:  
Traceback (most recent call last):
  File "/Users/jeff/code/moz/jetpack/addon-sdk-1.0b5/bin/cfx", line 29, in <module>
    cuddlefish.run()
  File "/Users/jeff/code/moz/jetpack/addon-sdk-1.0b5/python-lib/cuddlefish/__init__.py", line 636, in run
    include_dep_tests=options.dep_tests
  File "/Users/jeff/code/moz/jetpack/addon-sdk-1.0b5/python-lib/cuddlefish/packaging.py", line 283, in generate_build_for_target
    add_section_to_build(target_cfg, "tests", is_code=True)
  File "/Users/jeff/code/moz/jetpack/addon-sdk-1.0b5/python-lib/cuddlefish/packaging.py", line 258, in add_section_to_build
    validate_resource_hostname(name)
  File "/Users/jeff/code/moz/jetpack/addon-sdk-1.0b5/python-lib/cuddlefish/packaging.py", line 82, in validate_resource_hostname
    raise ValueError('invalid resource hostname: %s' % name)
ValueError: invalid resource hostname: jid1-gwb55aaeefl6yq-goo.gl_shortener-test

Expected Results:  
Firefox starts up with the extension enabled.
(Assignee)

Comment 1

7 years ago
This is either a docs bug or a pretty quick regex fix. Possible patches to follow.
(Assignee)

Comment 2

7 years ago
Created attachment 533535 [details] [diff] [review]
Patch to the docs stating the name field should not contain periods.

Just patch the docs to clarify that the field in question cannot contain periods.
(Assignee)

Comment 3

7 years ago
Created attachment 533536 [details] [diff] [review]
Alternate patch that actually adds periods to the regex.

Dashes and underscores are already there, periods seem innocuous but I'm unsure what larger spec governs these values in install.rdf. With this patch applied, a simple extension with a period in the name field seems to work correctly.
(Assignee)

Comment 4

7 years ago
It's conceivable that a developer just starting with Jetpack could create a folder with a dot in it, follow the tutorial and then get an obscure error message when running the example extension - admittedly an edge case ;).
This sounds like a bug we already have filed, although I wasn't able to find it in a quick search.

Irakli, Brian: do you know if this is a duplicate of another bug?
Whiteboard: [triage:followup]
(Assignee)

Comment 6

7 years ago
It's similar to this one, but not exactly the same:

https://bugzilla.mozilla.org/show_bug.cgi?id=568131

I searched first! Really! I also did a quick look for a spec on what should or shouldn't be allowed in that field and didn't see any comments on whether periods are inherently bad - not an exhaustive search though, there was a hockey game on.
At this point, I'm in favor of adding docs and assertions rather than allowing more characters (and being surprised by something that doesn't like them). Addon names (and package names) turn into resource: URI fields that are treated like DNS names, so I wouldn't be surprised that periods cause trouble. So I'll go for the docs patch rather than the regex one (which, incidentally, appears to be reversed).

If folks agree I'll land it tomorrow morning.
Assignee: nobody → warner-bugzilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 8

7 years ago
If you're going to dis-allow periods, it might be better to provide a better error message? I've got a new patch that a) reverses the last diff ( d'oh ) and b) detect periods and spaces in the same statement and return a better error message for that case.
(Assignee)

Comment 9

7 years ago
Created attachment 533888 [details] [diff] [review]
Patch to fix the docs ( as previous ) and improve error messages from packaging.py
Attachment #533535 - Attachment is obsolete: true
Attachment #533536 - Attachment is obsolete: true
(Assignee)

Comment 10

7 years ago
ps This is all committed to my fork:

https://github.com/canuckistani/addon-sdk

Let me know if you want a pull request, or further changes.
Landed in https://github.com/mozilla/addon-sdk/commit/ad0498464ab774dba4b410460dc51ea85f94ff37 . I made a few small changes to fix one of the doctests (use "cfx testcfx" to run those).

Thanks for the patch!
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Changing assignee to Jeff, as he contributed most of the patch, even though it was landed with some modifications by Brian.
Assignee: warner-bugzilla → jeff
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [triage:followup]
You need to log in before you can comment on or make changes to this bug.