Closed
Bug 922463
Opened 11 years ago
Closed 11 years ago
rewrite build/multilocale.py in javascript
Categories
(Firefox OS Graveyard :: Gaia, defect)
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.
Assignee | ||
Comment 1•11 years ago
|
||
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
Assignee | ||
Comment 2•11 years ago
|
||
more function coming. https://github.com/yurenju/gaia/commit/cffa97be99557bcedff1b64bbbcac0880ae599f4
Assignee | ||
Comment 3•11 years ago
|
||
WIP is updated. https://github.com/yurenju/gaia/commit/f6a3eb7776178b4554886bf30716ece0fc424f98 hope I can finish it during summit.
Assignee | ||
Comment 4•11 years ago
|
||
WIP with unit tests. https://github.com/yurenju/gaia/commit/cb0cb8d3b770ac33ad9ab18f7935e46414b7a5f4
Assignee | ||
Comment 5•11 years ago
|
||
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.
Comment 6•11 years ago
|
||
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.
Assignee | ||
Comment 7•11 years ago
|
||
WIP today! almost finish :D hope we can land it this week! https://github.com/yurenju/gaia/commit/5d95591a91d70cbe6d8d8952d8333ca02c5853c5
Comment 8•11 years ago
|
||
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?
Comment 9•11 years ago
|
||
Yuren: while working on l10n.js refactor we encountered bug 927635. Do you plan to fix it in your rewrite?
Assignee | ||
Comment 10•11 years ago
|
||
(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!
Assignee | ||
Comment 12•11 years ago
|
||
first workable WIP, woooow! https://github.com/yurenju/gaia/commit/ba34b747357c1128621892f3ac6060f6a329b909 I'm still working for shared/*.ini
Comment 13•11 years ago
|
||
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.
Assignee | ||
Comment 14•11 years ago
|
||
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
Assignee | ||
Comment 15•11 years ago
|
||
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)
Comment 16•11 years ago
|
||
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)
Assignee | ||
Comment 17•11 years ago
|
||
default of GAIA_CONCAT_LOCALES is 1, hummmm... https://github.com/mozilla-b2g/gaia/blob/master/Makefile#L280-L285
Assignee | ||
Comment 18•11 years ago
|
||
I got it. after setting GAIA_INLINE_LOCALES=0 GAIA_CONCAT_LOCALES=0 I got locales/* in zip file.
Assignee | ||
Comment 19•11 years ago
|
||
WIP with localized manifest, ini and properties files. https://github.com/yurenju/gaia/commit/e673ac662524e6a9b8a0a73e48f8e46f19fb059c now I'm working on locales-obj.
Assignee | ||
Comment 20•11 years ago
|
||
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.
Assignee | ||
Comment 21•11 years ago
|
||
major work is done. https://github.com/yurenju/gaia/commit/4a5a6614fbf82ff899b8e4e1abb5e565e1b6e2ff but the code need to be cleaned.
Comment 22•11 years ago
|
||
Do we need to touch webapp-manifests.js, too? Also, there seems to be a bunch of linting going into this patch, too?
Assignee | ||
Comment 23•11 years ago
|
||
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 :-)
Assignee | ||
Comment 24•11 years ago
|
||
Today WIP https://github.com/yurenju/gaia/commit/5cc8a7ae166369b00d588d50c9c0bd9aafa93c15#diff-3 hopely I can have a pr tomorrow.
Assignee | ||
Comment 25•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
blocking-b2g: --- → 1.3?
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Comment 26•11 years ago
|
||
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.
Assignee | ||
Comment 27•11 years ago
|
||
(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
Assignee | ||
Comment 28•11 years ago
|
||
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 → ---
Assignee | ||
Updated•11 years ago
|
blocking-b2g: --- → 1.3?
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Updated•11 years ago
|
blocking-b2g: 1.3? → 1.3+
Whiteboard: [ft:system-platform]
Assignee | ||
Comment 29•11 years ago
|
||
WIP 4 Fixed manifest-webapp issue. https://github.com/yurenju/gaia/commit/376a7cb2ef3d89c3eb704880015172ca11fdf20d
Assignee | ||
Comment 30•11 years ago
|
||
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)
Assignee | ||
Comment 31•11 years ago
|
||
WIP-6 - Add JSDoc comment for multilocale.js - Move localizeIni() and localize() from webapp-zip to multilocale https://github.com/yurenju/gaia/commit/ab25df81a34e70eb54b7693cb9fb89033a5359c6
Assignee | ||
Comment 32•11 years ago
|
||
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.
Assignee | ||
Comment 34•11 years ago
|
||
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)
Comment 36•11 years ago
|
||
(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 37•11 years ago
|
||
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+
Comment 38•11 years ago
|
||
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 39•11 years ago
|
||
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)
Assignee | ||
Comment 40•11 years ago
|
||
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.
Assignee | ||
Comment 41•11 years ago
|
||
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
Assignee | ||
Comment 42•11 years ago
|
||
Brijesh is interested in this issue, assign to him.
Assignee: yurenju.mozilla → brijesh.ks
Updated•11 years ago
|
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
Assignee | ||
Comment 44•11 years ago
|
||
Take it back since we need finish it in this week.
Assignee: brijesh.ks → yurenju.mozilla
Status: NEW → ASSIGNED
Comment 45•11 years ago
|
||
This is not a committed feature for 1.3, so this should not block the release.
blocking-b2g: 1.3+ → 1.3?
Assignee | ||
Comment 46•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(l10n)
Assignee | ||
Comment 47•11 years ago
|
||
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)
Assignee | ||
Comment 48•11 years ago
|
||
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 49•11 years ago
|
||
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+
Comment 50•11 years ago
|
||
This feature engineering work has been moved to v1.4.
blocking-b2g: 1.3? → 1.4?
Assignee | ||
Comment 51•11 years ago
|
||
fixed all issues which Alex mentioned on github :D https://github.com/mozilla-b2g/gaia/pull/13135 I'm working on DEBUG=1 support.
Comment 52•11 years ago
|
||
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)
Assignee | ||
Comment 53•11 years ago
|
||
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
Updated•11 years ago
|
blocking-b2g: 1.4? → ---
Target Milestone: 1.3 Sprint 6 - 12/6 → 1.3 C2/1.4 S2(17jan)
Assignee | ||
Comment 54•11 years ago
|
||
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 55•11 years ago
|
||
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+
Assignee | ||
Comment 56•11 years ago
|
||
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 57•11 years ago
|
||
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+
Comment 58•11 years ago
|
||
(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.
Assignee | ||
Comment 59•11 years ago
|
||
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/
Assignee | ||
Updated•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 60•11 years ago
|
||
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)
Comment 61•11 years ago
|
||
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)
Assignee | ||
Comment 62•11 years ago
|
||
since I got error from TBPL, back out these two commits first. https://github.com/mozilla-b2g/gaia/commit/30d9648e8b442b421116d250d079a051d9299de4 https://github.com/mozilla-b2g/gaia/commit/2a5c0464b3dc15bdcfcd12d44c2039215fe02a99
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 63•11 years ago
|
||
TBPL log: https://tbpl.mozilla.org/php/getParsedLog.php?id=32344549&tree=B2g-Inbound#error0
Assignee | ||
Comment 64•11 years ago
|
||
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
Assignee | ||
Comment 65•11 years ago
|
||
mreged again. https://github.com/mozilla-b2g/gaia/commit/7f6af5bda6ae2d02ffe61774f13850574b9dc661 I'll keep an eye on TBPL.
Assignee | ||
Updated•11 years ago
|
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 66•11 years ago
|
||
TBPL passed. https://tbpl.mozilla.org/php/getParsedLog.php?id=32350242&tree=B2g-Inbound
You need to log in
before you can comment on or make changes to this bug.
Description
•