658165 - The extension name field in package.json cannot contain periods

Status: RESOLVED FIXED

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

Description

[Jeff Griffiths]

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.

Comment 1

[Jeff Griffiths]

This is either a docs bug or a pretty quick regex fix. Possible patches to follow.

Comment 2

[Jeff Griffiths]

Attachment: 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.

Comment 3

[Jeff Griffiths]

Attachment: 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.

Comment 4

[Jeff Griffiths]

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 ;).

Comment 5

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

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?

Comment 6

[Jeff Griffiths]

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.

Comment 7

[Brian Warner [:warner :bwarner]]

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.

Comment 8

[Jeff Griffiths]

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.

Comment 9

[Jeff Griffiths]

Attachment: Patch to fix the docs ( as previous ) and improve error messages from packaging.py

Comment 10

[Jeff Griffiths]

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.

Comment 11

[Brian Warner [:warner :bwarner]]

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!

Comment 12

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

Changing assignee to Jeff, as he contributed most of the patch, even though it was landed with some modifications by Brian.