561944 - Give clearer/cleaner errors when validate_resource_hostname() throws.

Status: RESOLVED FIXED

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

Description

[Gomita]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.3a5pre) Gecko/20100426 Minefield/3.7a5pre
Build Identifier: 

The XPI file exported by 'cfx xpi' command is named after the value of the "name" property in package.json.
If the value of the "name" property contains some invalid characters (\ / * : ? " < > |) which we cannot use as a part of file name, 'cfx xpi' fails to export XPI.

Reproducible: Always

Steps to Reproduce:
1. set the value of the "name" property in package.json as "Hello * World".
2. try to export XPI by 'cfx xpi' command.
Actual Results:  
Fails to export XPI file with the following error message.
IOError: [Errno 22] invalid mode ('wb') or filename: u'Hello * World.xpi'

Expected Results:  
Succcessfully exports XPI file.

Comment 1

[Gomita]

I think the XPI file name should be named after the package directory name like: 'hello-world.xpi' (assuming that the name of the package root directory is 'hello-world').

Comment 2

[Atul Varma [:atul]]

Oh, so theoretically long-ish names with special characters is what the 'fullName' property in the package.json is for:

  https://jetpack.mozillalabs.com/sdk/0.2/docs/#guide/package-spec

'name' is actually not meant to be something with those funky characters, and it actually defaults to the name of the package's directory if it's not in package.json--though this may conflict with the CommonJS package specification, I'm not sure.

In any case, thanks for the report--we should definitely not raise a silly IOError in this instance, at the very least!

Comment 3

[Gomita]

Sorry, I missed to read the spec of package.json.
Now I understand that I should use "fullName" property instead of "name" to display a funcy name in the Extensions Manager.
Should I change the status of the bug to "RESOLVED"?

I found another related bug.
If the value of "name" property in package.json and the root directory name both contain a white-space such like "hello world",
'cfx run' command fails with the following error message:

(C:\jetpack-sdk-0.3) C:\jetpack-sdk-0.3\packages\hello world>cfx run -a firefox
[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.import]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///c:
/docume~1/xxx/locals~1/temp/xxx.mozrunner/extensions/[e-mail]/components/harness.js :: buildLoader :: line 163"  data: no] (undefined:163)
FAIL

Should I file this as a separated bug?

Comment 4

[Gomita]

Sorry again, the package spec says that: "This name cannot contain spaces."

Comment 5

[Atul Varma [:atul]]

No, it's not your fault! We're providing terrible error feedback here--we should detect for these kinds of cases in cfx and give more helpful warnings, like:

"Sorry, but thee 'name' property of packages/foo/package.json cannot be 'foo bar' because it contains illegal characters. You may want to use the 'fullName' property instead. For more information, see the package.json documentation at static-files/md/dev-guide/package-spec.md."

How does something like that sound?

Comment 6

[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".

Comment 7

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

It sounds like we should:

* validate that the name field only contains valid characters
* strip out invalid characters when creating a XPI

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Is this still happening?

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Myk: I never got confirmation that this was still happening after setting the triage flag a month ago. Do you know if we already do the things you listed in comment 7?

Comment 11

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

Yes, this is still happening.  To test this, I created an addon using "cfx init", changed its name in package.json to "foo/bar", and called "cfx xpi" to try to package it, whereupon cfx printed the following error with traceback:

Traceback (most recent call last):
  File "c:/Users/myk/Dropbox/addon-sdk/bin/cfx", line 29, in <module>
    cuddlefish.run()
  File "c:\Users\myk\Dropbox\addon-sdk\python-lib\cuddlefish\__init__.py", line 637, in run
    include_dep_tests=options.dep_tests
  File "c:\Users\myk\Dropbox\addon-sdk\python-lib\cuddlefish\packaging.py", line 322, in generate_build_for_target
    add_section_to_build(target_cfg, "tests", is_code=True)
  File "c:\Users\myk\Dropbox\addon-sdk\python-lib\cuddlefish\packaging.py", line 297, in add_section_to_build
    validate_resource_hostname(name)
  File "c:\Users\myk\Dropbox\addon-sdk\python-lib\cuddlefish\packaging.py", line 85, in validate_resource_hostname
    raise ValueError('invalid resource hostname: %s' % name)
ValueError: invalid resource hostname: jid1-qwouchluzbmukq-at-jetpack-foo/bar-tests

cfx should instead tell me without traceback that my addon's name contains invalid characters, if indeed "/" is an invalid character in addon names.  Otherwise, it should strip out that character when using the addon name as a resource hostname, XPI filename, etc.

(And of course it should do the same for other such characters.)

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Myk Melez [:myk] from comment #11)
So basically, in https://github.com/mozilla/addon-sdk/blob/3eb964d2c25963a0516c59cd072a08b33ab29013/python-lib/cuddlefish/packaging.py#L76 it should catch the errors raised in those three ifs? (Or just not raise any errors but just modify 'name'?)

Comment 13

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

(In reply to Wes Kocher (:KWierso) (Jetpack Bugmaster) from comment #12)
> (In reply to Myk Melez [:myk] from comment #11)
> So basically, in
> https://github.com/mozilla/addon-sdk/blob/
> 3eb964d2c25963a0516c59cd072a08b33ab29013/python-lib/cuddlefish/packaging.
> py#L76 it should catch the errors raised in those three ifs? (Or just not
> raise any errors but just modify 'name'?)

Hmm, yes, I think that's right.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Pointer to pull request 278

So this pull request just changes the wording of the error being raised when invalid stuff is detected. It now mentions that the name must be only letters, numbers, dashes and underscores, instead of just "invalid resource hostname".


Also, as of some recent commit, it no longer shows the jid-style URI in the error, it just shows the package name's value.

The pull request doesn't strip out invalid characters or hide the traceback, but it's at least an improvement over what was there.

Comment 15

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

Comment on attachment 577802 [details]
Pointer to pull request 278

Comments in pull request.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 577802 [details]
Pointer to pull request 278

Ready for another review, Myk.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 577802 [details]
Pointer to pull request 278

Passing review request to markh.

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 577802 [details]
Pointer to pull request 278

looks good!

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Renaming bug to what the patch actually does.

Comment 20

[[github robot]]

Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/95f03637db95a53c1169804e19e54e9cdb43f7b4
Bug 561944 - Give clearer/cleaner errors when validate_resource_hostname() throws.  r=@mhammond