Closed Bug 922463 Opened 11 years ago Closed 10 years ago

rewrite build/multilocale.py in javascript

Categories

(Firefox OS Graveyard :: Gaia, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.3 C2/1.4 S2(17jan)

People

(Reporter: yurenju, Assigned: yurenju)

References

Details

(Whiteboard: [ft:system-platform])

Attachments

(3 files)

to build gaia localized build by firefox extension, we need to re-write multilocale.py in javascript.
No longer blocks: 922540
WIP
https://github.com/yurenju/gaia/commit/b89405631c79b59b36f5e5e577431b7a88aa0c0d

I would like to add file into zip in webapp-zip.js instead of modify gaia tree, but i need to rewrite in javascript first :)

maybe I can done this on the plane to santa clara LOL
I found we used webapp-optimize.js to generate localized HTML and l10n json file into gaia source tree, need to clear up code if we want to add all l10n files into zip without modifying or adding files in gaia source tree.
Agreed. Still would love to catch up with you in SC. I did a little project on the flight, too, doing pseudo localization, which mostly works right now, https://twitter.com/axelhecht/status/385917042903097344.
A few comments:

Generally, let vs var? I could use some comments to follow what's happening.

You're adding findFiles, but I don't see it used.

The serializeIni function seems to rely on order when iterating over the object that the first added is returned first ('default'), not sure if that's OK to assume. Also, should that produce a string or really an array?

I'd probably be more restrictive in which ini files you process.

I've tripped over used.locales in webapp-zip.js, but couldn't figure out what it's actually doing :-/

It'd be nice if we could use just one .properties parser, too. Right now, there's one in l10n.js with two variants, and the old-dumb version of it in multilocale.*. And then I copied a single-code-path one into my pseudo-l10n code :-/. Not necessarily food for this bug, though.

The localeProps code path is confusing me right now, it seems to be generated as array and used as object?

That's just what I found by glancing at the current patch, hope that helps.

Did you find out what we actually need to do for DEBUG=1 profiles since we talked at summit?
Yuren: while working on l10n.js refactor we encountered bug 927635. Do you plan to fix it in your rewrite?
(In reply to Axel Hecht [:Pike] from comment #8)
> A few comments:
> 
> Generally, let vs var? I could use some comments to follow what's happening.
actually I have plan to support node.js so it would be great if use var replace all let.

> You're adding findFiles, but I don't see it used.
it's the rewrite version from find_ini in multilocale.py, but I will remove it since we won't use it anymore.

> The serializeIni function seems to rely on order when iterating over the
> object that the first added is returned first ('default'), not sure if
> that's OK to assume. Also, should that produce a string or really an array?
seems we set properties files as default if it isn't in a section, e.g.,

GAIA_DIR/apps/homescreen/locales/locales.ini
============================================

@import url(homescreen.en-US.properties)
@import url(collections.en-US.properties)

[ar]
@import url(homescreen.ar.properties)

[fr]
@import url(homescreen.fr.properties)
@import url(collections.fr.properties)

[zh-TW]
@import url(homescreen.zh-TW.properties)

we use the first two lines as default.

and yes we should use string instead of return a array.

> I'd probably be more restrictive in which ini files you process.
that's a good idea, how about locales/*.ini?

> I've tripped over used.locales in webapp-zip.js, but couldn't figure out
> what it's actually doing :-/
I didn't implment that part yet, you will see it in next WIP!

> It'd be nice if we could use just one .properties parser, too. Right now,
> there's one in l10n.js with two variants, and the old-dumb version of it in
> multilocale.*. And then I copied a single-code-path one into my pseudo-l10n
> code :-/. Not necessarily food for this bug, though.
> 
> The localeProps code path is confusing me right now, it seems to be
> generated as array and used as object?
localesProps is an array with manifest object.

> That's just what I found by glancing at the current patch, hope that helps.
> 
> Did you find out what we actually need to do for DEBUG=1 profiles since we
> talked at summit?
no, but we may hack httpd.js for this.

thank you for your comments!
Zbigniew, 

yes, I'll study your patch and fix it on this bug.
Nice. Few more comments on webapp-zip.js:

I'm trying to read the getLocaleProperties() function, that hardcodes /apps/browser/.... as the two significant levels of directory structure? Is that an OK constraint?

Also, I don't find the place in the code that drops the /locales/ path segment for the l10n files. I.e.

apps/browser/locales/browser.en-US.properties is localized by
de/apps/browser/browser.properties.

Also, to repeat my comment from the branding bug, we don't localized the official branding, just the unofficial branding.
I updated pull request and modified some code for coping correct file from LOCALE_BASEDIR.

https://github.com/yurenju/gaia/blob/8e2e77f772deed91413c959c48f7cfdeb0fa4fd9/build/webapp-zip.js#L217-L218
I found we didn't copy any properties files into zip for master, e.g., I downloaded Polish l10n and use below language.json:

> {
>   "en-US" : "English (US)",
>   "pl" : "Polish"
> } 

then I extracted profile/webapps/browser.gaiamobile.org/applications.zip and I found we only have locales-obj:

> /gaia/profile/webapps/browser.gaiamobile.org/application
> ├── index.html
> ├── js
> │   └── ...
> ├── locales-obj
> │   ├── ar.json
> │   ├── en-US.json
> │   ├── fr.json
> │   ├── pl.json
> │   └── zh-TW.json
> ├── manifest.webapp
> ├── shared
> │   ├── js
> │   │   └── ...
> │   ├── resources
> │   │   └── branding
> │   │       ├── Browser_60.png
> │   │       └── about_logo.png
> │   └── style
> │       └── ...
> └── style
> └── ...

is it correct?
Flags: needinfo?(l10n)
I'm afraid that depends on the various optimize settings. At least GAIA_CONCAT_LOCALES seems to be involved, in webapp-optimize.js. I'm not deeply involved in those myself, I'd log at git blame to figure out folks in the know.
Flags: needinfo?(l10n)
I got it. after setting GAIA_INLINE_LOCALES=0 GAIA_CONCAT_LOCALES=0 I got locales/* in zip file.
WIP with localized manifest, ini and properties files.
https://github.com/yurenju/gaia/commit/e673ac662524e6a9b8a0a73e48f8e46f19fb059c

now I'm working on locales-obj.
because I didn't copy properties files and ini file to gaia source tree, in webapp-optimize.js I need to hack optimize_getFileContent for getting properties files in LOCALE_BASEDIR.
Do we need to touch webapp-manifests.js, too?

Also, there seems to be a bunch of linting going into this patch, too?
Why should we touch webapp-manifest.js?
this patch comes huge, I thought we need some follow up bugs if needed.

and yes, I'm refactoring to get more clear code :-)
WIP updated, now we can get exactly same result as original multilocale.py but won't mess up source tree, use below environment for testing:
- download Polish l10n zip and assign a new language.json only with english and polish
- test for locales-obj: |make|
- test for locales: |make make GAIA_INLINE_LOCALES=0 GAIA_CONCAT_LOCALES=0|

https://github.com/yurenju/gaia/compare/multilocale.js-2

- remove multilocale.copyProperty and integate to getPropertiesFile
- support utf-8 for addEntryStringWithTime
- add locales/* into zip only if INLINE_LOCALES or CONCAT_LOCALES = 1
- minor fixes

still working on testing Windows
blocking-b2g: --- → 1.3?
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Testing this locally now.

First thing I hit is that DEBUG=1 aka desktop needs to be fixed still.

Also, LOCALES_FILE needs to be an absolute path, that used to be OK with a relative path.

Only the communications.g.o manifest.webapp file is localized. I guess that goes back to whether or not we actually need or use those for anything.

Not sure if this is a regression, but I came across contacts/locales/fb/facebook.ar.properties and friends in the application.zip of the comms app.
(In reply to Axel Hecht [:Pike] from comment #26)
> Testing this locally now.
> 
> First thing I hit is that DEBUG=1 aka desktop needs to be fixed still.
yeah i know. What do you think if we create a follow bug for it?

> Also, LOCALES_FILE needs to be an absolute path, that used to be OK with a
> relative path.
Oh I fixed it on today WIP!

> Only the communications.g.o manifest.webapp file is localized. I guess that
> goes back to whether or not we actually need or use those for anything.
not sure what's the problem is, do you mean manifest.webapp in zip or in profile/webapps/communications.g.o/manifest.webapp

I haven't modified webapp-manifest.js yet

> Not sure if this is a regression, but I came across
> contacts/locales/fb/facebook.ar.properties and friends in the
> application.zip of the comms app.


and this is WIP which support Windows and relative path for LOCALES_FILE and LOCALE_BASEDIR!
https://github.com/yurenju/gaia/commit/229f2441db0b3d1f443de7eea90397c8956dab25
BTW, you can use this command to extract all zip and diff directories:

> find profile -name 'application.zip' -exec sh -c "unzip {} -d {}-dir" \;

for Mac, you need install findutils from homebrew and use gfind instead of find.
blocking-b2g: 1.3? → ---
Target Milestone: 1.3 Sprint 4 - 11/8 → ---
blocking-b2g: --- → 1.3?
Target Milestone: --- → 1.3 Sprint 4 - 11/8
blocking-b2g: 1.3? → 1.3+
Whiteboard: [ft:system-platform]
Blocks: 931457
Gandalf,

should I copy branding properties file from unofficial/*.en-US.properties to unofficial/*.properties in zip file?

for my pull request, properties files for localization will be copied into zip if available, e.g., we have branding.properties file for Polish, so this file will be added into zip.
Flags: needinfo?(gandalf)
WIP-6

- Add JSDoc comment for multilocale.js
- Move localizeIni() and localize() from webapp-zip to multilocale

https://github.com/yurenju/gaia/commit/ab25df81a34e70eb54b7693cb9fb89033a5359c6
I will do some tests and hopely i can have a pull request tomorrow and I would like to file a follow up bug for DEBUG=1 mode localization.

below are some test scenario:

without LOCALES_FILE and LOCALE_BASEDIR
- make reset-gaia
- make production
- DEBUG=1 make

with absolute path
- make reset-gaia
- make production
- DEBUG=1 make

with relative path such as ".."
- make reset-gaia
- make production
- DEBUG=1 make 

and for sure, testing on Windows.
Oh yeah, everything looks good on Windows ;-)
Finally, we have the first pull request :-)

I will squash all commits when get r+.
Attachment #823254 - Flags: review?(timdream)
Attachment #823254 - Flags: review?(poirot.alex)
Attachment #823254 - Flags: review?(l10n)
(In reply to Yuren Ju [:yurenju] from comment #30)
> Gandalf,
> 
> should I copy branding properties file from unofficial/*.en-US.properties to
> unofficial/*.properties in zip file?
> 
> for my pull request, properties files for localization will be copied into
> zip if available, e.g., we have branding.properties file for Polish, so this
> file will be added into zip.

We should copy unofficial branding from the locale folder, but official from en-US.  That is:

if MOZILLA_OFFICIAL is 1:

  $GAIA_DIR/shared/locales/branding/official/branding.en-US.properties should be copied into
  shared/locales/branding/branding.$LOCALE.properties in the zip


if MOZILLA_OFFICIAL is 0:

  $LOCALE_BASEDIR/$LOCALE/shared/branding/unofficial/branding.properties should be copied into
  shared/locales/branding/branding.$LOCALE.properties in the zip

This is what the patch in bug 927635 attempts to do, but in Python :)
Flags: needinfo?(gandalf)
Comment on attachment 823254 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/13135

Skim-read, look good!
Attachment #823254 - Flags: review?(timdream) → feedback+
I see a lot of good comments from Alex on the PR.

One thing I didn't figure out is, are you getting the localized webapp.manifest into the application.zip, too? I find it really confusing that we have both, and I'd rather have you check.

Also, I'm feeling really uneasy about landing this without DEBUG=1 support. I started looking at that a bit, so I might be able to help.
Comment on attachment 823254 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/13135

That's an amazing work you provided!
I'm so happy to see multilocale no longer pollute workdir... and that without even having an objdir. And all that in JS \o/

I'd need another round to understand the zip step and what looks like a duplicated job, but I most likely missed something.
Attachment #823254 - Flags: review?(poirot.alex)
I had a WIP to address Alex and Pike's review, but not finish yet. and I'm going to focus on bug 897325 in this week.
the milestone date is incorrect, we should fix in before 11/22 (sprint 5)
Target Milestone: 1.3 Sprint 4 - 11/8 → 1.3 Sprint 5 - 11/22
Brijesh is interested in this issue, assign to him.
Assignee: yurenju.mozilla → brijesh.ks
we should fix some issue bug 940897 in this bug.
See Also: → 940897
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
Take it back since we need finish it in this week.
Assignee: brijesh.ks → yurenju.mozilla
Status: NEW → ASSIGNED
This is not a committed feature for 1.3, so this should not block the release.
blocking-b2g: 1.3+ → 1.3?
Comment on attachment 823254 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/13135

Hi Alex, I modified a lot of code accroding your comments, and now we only export a constrocutor for L10nManager, then we can do some locationzation things without give a lot of arguments again and again such as gaiadir, shareddir, localesFile & LOCALE_BASEDIR.

and I'll try to make DEBUG=1 build work tomorrow, so I will make another pull request for that.

Pike, if you find a good way to hack httpd.js for DEBUG=1 build support, please let me know.
Attachment #823254 - Flags: review?(poirot.alex)
Flags: needinfo?(l10n)
Comment on attachment 823254 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/13135

Hi Alex, I have canceled review because I found some issues, I'll set it again if I fixed it.
Attachment #823254 - Flags: review?(poirot.alex)
Attachment #823254 - Flags: review?(l10n)
Comment on attachment 823254 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/13135

okay, fixed, that is a stupid mistake because I use Function.call but append a array as args.

and I found we have _findPropertiesPath in httpd.js for localization, this is a good start.
Attachment #823254 - Flags: review?(poirot.alex)
Attachment #823254 - Flags: review?(l10n)
Comment on attachment 823254 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/13135

Looks good to me, seems to work fine but I don't really know what kind of edge cases we should test.
I mainly suggested various doc improvements.
Attachment #823254 - Flags: review?(poirot.alex) → review+
This feature engineering work has been moved to v1.4.
blocking-b2g: 1.3? → 1.4?
fixed all issues which Alex mentioned on github :D

https://github.com/mozilla-b2g/gaia/pull/13135

I'm working on DEBUG=1 support.
status update, I won't get to this review request until mid of next week. I've traveled last week, and this week. sorry for the delay.

Re httpd.js, I was wondering if it'd make sense to add our customizations to that in a middleware or something to make it easier to upstream updates from mozilla-central. bug 393063 sounds related, but I didn't get feedback from Waldo there yet. For the manifests, it sounds like explicitly registering path handlers for those might work.
Flags: needinfo?(l10n)
okay I compare original[1] httpd.js and ours, looks not so different, it would be great if we can have a middleware to handle it.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/httpd.js
blocking-b2g: 1.4? → ---
Target Milestone: 1.3 Sprint 6 - 12/6 → 1.3 C2/1.4 S2(17jan)
Comment on attachment 823254 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/13135

aha, DEBUG=1 works again :-)

Tim, could you help to review part 2 commit which is modified httpd extension to support DEBUG=1 mode?

https://github.com/yurenju/gaia/commit/8bb05767bad41390bfe5408a5ee94c5e4567a2e8

and nightly looks has some trouble for now, but we still can drag notification to bottom and click settings to switch language, and go to http://<APP>.gaiamobile.org:8080 to test apps.
Attachment #823254 - Flags: review?(timdream)
Comment on attachment 823254 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/13135

Looks fine if you have tested all scenarios.

(BTW, is it possible to write automation tests to run build script with these scenarios?)
Attachment #823254 - Flags: review?(timdream) → review+
we have basic build tests with scenarios for multilocale, but we can add more to verify our build result.

https://travis-ci.org/mozilla-b2g/gaia/jobs/15501408
Comment on attachment 823254 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/13135

Sweeeet, r=me.

Perhaps as a follow-up, should the default value of GAIA_CONCAT_LOCALES be !DEBUG?

One thing I noticed is that changes to manifest.properties require a remake.

I also wished that "reload application" worked in the devtools. Right now, one has to kill the app via long-home-press and then start it again to get strings to update.

All of these are follow-ups, though, this is so much better than what we have.
Attachment #823254 - Flags: review?(l10n) → review+
(In reply to Axel Hecht [:Pike] from comment #57)
> Comment on attachment 823254 [details] [review]
> github PR: https://github.com/mozilla-b2g/gaia/pull/13135
> 
> Sweeeet, r=me.
> 
> Perhaps as a follow-up, should the default value of GAIA_CONCAT_LOCALES be
> !DEBUG?
> 
> One thing I noticed is that changes to manifest.properties require a remake.
> 
> I also wished that "reload application" worked in the devtools. Right now,
> one has to kill the app via long-home-press and then start it again to get
> strings to update.

Note that with the app manager, when you click on Update button, the app is reloaded, so whatever is updated in application.zip should be updated. But I'd imagine you update it via the command line with APP=foo make install-gaia.
Bug 912903 is meant to allow plugging the gaia build system (or any app specific build scripts) into the app manager.
Merged.

https://github.com/mozilla-b2g/gaia/commit/3f5e44888df7407c8ef7995233925d43ba1a763d

Thank you Pike, Alex and Tim spent a lot of time on this bug! and this is the last bug for py2js, so we have no python build scripts in GAIA_DIR/build/ now!

\o/
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Hi sheriffs,

I set needinfo flag to you because this pull request has high potential to break TBPL but we have no way to push this to try server before I land it.

if you found something break TBPL please check this bug first.
Flags: needinfo?(sheriffs)
Due to the holiday, no sheriffs are watching the tree right now. You're best waiting until after January 2 if you want assistance watching this post-landing.
Flags: needinfo?(sheriffs)
found the problem, TBPL use this path for LOCALE_BASEDIR:

> c:/builds/moz2_slave/b2g-in-w32_g-00000000000000000/build-gaia-l10n

and we expect getting:

> c:\builds\moz2_slave\b2g-in-w32_g-00000000000000000\build-gaia-l10n
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 955887
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: