730776 - Do not include locales.json and locale files unless the add-on is really localized

Status: RESOLVED FIXED

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

Description

[Kohei Yoshino [:kohei]]

Recently I've noticed that many add-ons include en-GB.json, eo.json, fr-FR.json and locales.json though those are not localized. That makes it difficult to detect which add-on is really localized. Please stop.

Comment 1

[Kohei Yoshino [:kohei]]

This makes my work sticky as I have manually analyzed every updated add-on to find localized ones. Please fix it ASAP.

Comment 2

[Alexandre Poirot [:ochameau]]

Agreed. This is only unit test strings that shouldn't end up in the xpi.
But we may have in future some strings related to SDK APIs.
So that you won't be able to distinguish translated addons and partially translated ones (only SDK API using translated strings).
I'm wondering what is your usecase ? I think you need something else.

I'll try to get rid of these files for 1.6 release.

Comment 3

[Dave Townsend [:mossop]]

Let's get this for the next release if possible.

Comment 4

[Kohei Yoshino [:kohei]]

(In reply to Alexandre Poirot (:ochameau) from comment #2)
> I'm wondering what is your usecase ?

We've collected localized add-ons to expand our own add-on library. 
See https://wiki.mozilla.org/Japan/AMJ for details.

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to Kohei Yoshino from comment #4)
> We've collected localized add-ons to expand our own add-on library. 
> See https://wiki.mozilla.org/Japan/AMJ for details.

Even if I'm able to fix this now, you will still it similar issue in long term. It will be really hard to distinguish partially and fully localized addons. And all of them are most likely going to be partilly localized, i.e. strings from the SDK itself will be localized.

Can you give me a link to your current code, I'm wondering how you do this for regular addons?

Comment 6

[Kohei Yoshino [:kohei]]

(In reply to Alexandre Poirot (:ochameau) from comment #5)
> It will be really hard to distinguish partially and fully localized addons.

Yeah I do know. Since 2006 I have downloaded and unzipped every updated add-ons on AMO, then *manually* detected Japanese-localized ones and localizeless ones, almost every day. This bug is just annoying.

Comment 7

[Alexandre Poirot [:ochameau]]

Brian, Irakli: I'm trying to figured out how to fix this properly without having to wait for packageless, nor any big incoming tasks. (Want to fix this in 1.6)

We currently have some issue with: cfx xpi.
We are shipping all files from api-utils/data and addon-kit/data. Similar thing is done for locales. The problem is that in most cases, these files are only there for unit tests. We really do not want them in users XPIs!!

What do you think about adding a new package at the same level than api-utils, addon-kit, test-harness ? This package is going to depend on addon-kit and contains only these unit tests that pollutes data folder and locales?
(I may just move localization tests first for 1.6)

One alternative would be to use a undocumented cfx feature. cfx supports `packages` folder in packages root folder. This folder can contain sub-packages. So that we can create a sub package in /packages/addon-kit/packages/tests/ and move l10n tests there.



In the long term, I would like to implement a built-in locale parser, that will fetch locale keys, like what we are doing with require. That will allow us to ship only used keys. (And provide other usefull features for localizers and developers)

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

> We currently have some issue with: cfx xpi.
> We are shipping all files from api-utils/data and addon-kit/data. Similar thing is > done for locales. The problem is that in most cases, these files are only there for > unit tests. We really do not want them in users XPIs!!

That's a good point and I think test assets should live in tests folder as they do in other projects.

> What do you think about adding a new package at the same level than api-utils,
> addon-kit, test-harness ? This package is going to depend on addon-kit and
> contains only these unit tests that pollutes data folder and locales?
> (I may just move localization tests first for 1.6)
> 
> One alternative would be to use a undocumented cfx feature. cfx supports
> `packages` folder in packages root folder. This folder can contain sub-packages. > So that we can create a sub package in /packages/addon-kit/packages/tests/ and
> move l10n tests there.


I'm not sure how this solves an issues, but it surely adds lot's complexity and will
raise lots of questions. I would rather go for property solution and putting test assets into test folder (or alternatively into a special folder) feels more adequate.

Comment 9

[Alexandre Poirot [:ochameau]]

The critical part here is that we really need files in `data` or `locale` folder, as we intend to test end-to-end feature.
In this particular case, I want to ensure that properties files are correctly parsed by cfx and that at the end, the high-level API returns valid strings.

We can't do what you describe here, because we can't execute an addon from JS.
i.e. create a "test-l10n" package, with a locale folder with some properties files, and just one unit test file that directly verify localized strings. We can't execute such "test package" from a JS unit test.

That brings me to another alternative. Move this localization unit test to python side. I've added some helpers in order to test some addons from python side. I introduced this in order to ensure that console.* works properly, and that firefox quits when we close the last window:
https://github.com/mozilla/addon-sdk/tree/master/python-lib/cuddlefish/tests/addons
I can add a second addon in order to test l10n features.

Would it makes more sense over adding a "test-package" next to api-utils/addon-kit?

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

>
> Would it makes more sense over adding a "test-package" next to
> api-utils/addon-kit?
>

Yeah I think that would be a better option.

Alternatively I'd suggest to just ship data folders of addon-kit and api-utils only in cfx test. This would require some changes in how we ship `content-proxy.js` and `worker.js` but I think that's ok as we need to do it either way. I'd suggest on of the following options:

# Option 1

1. Move worker.js & content-proxy.js to modules and have some kind of condition like:
`if (typeof(exports) === 'object') exports.uri = module.uri`. That would allow loading these modules into contest scripts without self but with `require('...').uri`.
2. Change cfx so that `data` folders of `addon-kit` and `api-utils` are only included
in `cfx test`.

# Option 2

1. Teach cfx linker how to handle `require('term!./foo')` like requires so it will just ignore part before `term!`.
2. Move `worker.js` and `content-proxy.js` to modules.
3. Teach loader to return `module.uri` on `require('asset!./foo')` and use that.
4. Include `data` folders only in `cfx test`.

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #10)
>
> Yeah I think that would be a better option.
> 
> Alternatively I'd suggest to just ship data folders of addon-kit and
> api-utils only in cfx test. This would require some changes in how we ship
> `content-proxy.js` and `worker.js` but I think that's ok as we need to do it
> either way.

These proposals only fix `data` folder. Not `locale` one. For now, localizations are not analysed as modules are. We don't know which module uses which localization key. (Like dependency graph of modules but for l10n) So, these two proposals around `require` will fix data files, but not l10n ones.
The second issue I faced while trying to implement option 1 is that it introduce major difference between cfx xpi and other commands so that we can very easily have regression happening only on cfx xpi. That would be really hard to debug/maintain. The third issue with Option 1 is that we will waste memory for two sandboxes, just to retrieve module URI.

Here is one concrete way to address locale file issues:
Add a new folder in our repository, that contains packages. We run tests for all of these packages, like `examples` folder:
  https://github.com/ochameau/addon-sdk/compare/auxiliary-packages
We would be able to put various packages there in order to test cfx itself. For example do various test against package.json content, ... 
The name may not be the best one. We might name this folder "unit-tests-packages", "tests-packages" or whatever makes more sense.
Here I just moved l10n tests, but we may create additional packages for tests that use data files.

Otherwise, I haven't thought about blacklisting addon-kit/api-utils in cfx. There is a simple stupid patch to fix locale issue, until a proper fix:
--- a/python-lib/cuddlefish/packaging.py
+++ b/python-lib/cuddlefish/packaging.py
@@ -316,6 +316,11 @@ def generate_build_for_target(pkg_cfg, target, deps,
                 build.packages[cfg.name][section] = dirname

     def add_locale_to_build(cfg):
+        # Bug 730776: Ignore locales for addon-kit, that currently are only for
+        # unit tests
+        if cfg.name == "addon-kit":
+            return
+
         path = resolve_dir(cfg, cfg['locale'])

Your thoughts? Do you like one of these options? Do you have any other one?

Comment 12

[Mingyi Liu]

I second the removal of empty en-GB, eo, fr-FR in the 'cfx xpi'-packed addons without further delay. I just can't understand why it took several months of discussions and these locales are still included, even in SDK 1.7? Sorry for sounding annoyed, but this issue just bit me big time:

I localized and released Fastest Search v2.21. I specifically left only en-US.properties in my locale directory.  Yet unbeknownst to me, after I ran 'cfx xpi', empty .json files for the above 3 locales were included AND the locales.json file also included them. This totally ruined the interface of my addon for users of en-GB, eo, and fr locales!  I didn't realize this, naturally - as I can't possibly test every locale that I DIDN'T include in my addon - until users started complaining.

Please, just remove those locales. Right now all developers have to manually unpack the xpi each time cfx xpi is run, change the locales.json and remove the 3 locales, then repackage and remove.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 452

Sorry guys for missing releases ... I'm just working on too many things at same time. I'll do my best to not miss yet another one!

Irakli: We discussed a lot about this, let try to get something for 1.8 ASAP.
So I'm suggesting to include this very simple fix in 1.8/stabilization branch.
i.e. pull request 452. I've just added some tests in cfx-py in order to avoid taking care of these files except during tests.
I'm suggesting here to land this simple fix now in order to be able to land whatever we think is best option whenever it is ready. Such simple fix can easily be pulled in stabilization branch!

Otherwise, I think you suggested me to do that during the work week:
# "Move l10n test and data file to examples"
https://github.com/ochameau/addon-sdk/compare/bug/730776-move-l10n-test-to-example
Here I moved it to an existing example, but I don't think it is a good idea, even if we create a dedicated example for it. Developers expect to find examples, nor internal unit tests.

# Move l10n tests to a folder containing tests addons:
https://github.com/ochameau/addon-sdk/compare/auxiliary-packages
That's for me the best solution I can think about. We create a new folder similar to examples or packages ones where we execute tests for each package.
We may even improve this later on by running main module instead.
So that we can start having a way to run tests without having to handle a very special case for tests. 
Obviously, I'd wait for cfx-js to be ready before doing this this improvement.

Comment 14

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/c5663854ce0c7de4d3f319c46e0a4313a6b7606d
Bug 730776: Ignore locales from addon-kit package in order to avoid shipping them in xpi.

https://github.com/mozilla/addon-sdk/commit/0cb1c873b59a5d7e77a5150dfaee5bdeb3472193
Merge pull request #452 from ochameau/bug/730776-ignore-addon-kit-locales

Bug 730776: Ignore locales from addon-kit package r=@gozala

Comment 15

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/57b5d7db645a881692584dfcca225570a272fe1c
Bug 730776: Ignore locales from addon-kit package in order to avoid shipping them in xpi.
(cherry picked from commit c5663854ce0c7de4d3f319c46e0a4313a6b7606d)

Comment 16

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

This is now included in 1.8b3, set to go out in 1.8 final in four weeks. If you want to test out the fix, details on how to get the beta builds are here: https://groups.google.com/forum/#!topic/mozilla-labs-jetpack/xYIdrlUwyb8