968666 - multilocale.js should be a standalone build script which can be executed by macro |run-js-command|

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Build, defect)

Description

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

after bug 897352 land, we can refactoring webapp-zip.js and migrate l10n feature from webapp-zip to standalone multilocale.js to get clearer build process.

after refactoring, webapp-zip.js should remove all l10n features, and multilocale.js should be a standalone build script which can be executed by macro |run-js-command|

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Yuren, did you get a change to work on this bug last week? Please give out and update here since this bug was listed on your weekly report.

Comment 2

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

no we are trying to land bug 897352 since sheriff said it broke windows build, I'm negotiating with sheriffs for this issue.

Comment 3

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

I'm still working on bug 897352

Comment 4

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

WIP here, this patch still has some problem for communications app, others looks good.

https://github.com/yurenju/gaia/commit/becc013d951afb7610c5415cbf2ed7dca60b8b94

Comment 5

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Attachment: github PR: https://github.com/mozilla-b2g/gaia/pull/18556

Alex, could you review this pr?

and Pike, could you give some feedback for this pr?

BTW, this pr cannot be merged to master because commit of bug 968661 was backout, but I think we still can review it first since commit of bug 968661 shouldn't be modified a lot.

Comment 6

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

try server:
https://tbpl.mozilla.org/?tree=Try&rev=a29cc88cdcb6

Comment 7

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

bug 968661 has been resolved.

Comment 8

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Comment on attachment 8410751 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/18556

Looks good to me, but I have some questions in the PR.
Multilocale works fine here but I would like to hear from Pike or any l10n expert to ensure everything is still in place.

From what I understand of current implementation of httpd,
it shouldn't change much to DEBUG=1 support as it seems to still fetch files from source directories.

Comment 9

[Axel Hecht [:Pike]]

I'm utterly slow to review this, sorry. I have a really hard time to wrap my head around what's actually supposed to happen in which stage. Could we add comments to that extent?

Then I could just check if things do what they should, instead of reverse engineering what things are doing, and then seeing if they're doing it right :-/ . Comments in both the Makefile and the js run scripts would help greatly.

Comment 10

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

There is a document for gaia build process
https://developer.mozilla.org/en-US/Firefox_OS/Platform/Gaia/Build_System_Primer#Build_process

multilocale should be execute between app-makefile and webapp-optimize, and our goal for this issue is move all build process for multilocale to multilocale.js instead of using l10nManager everywhere.

and find me on IRC if you have any question!

Comment 11

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

l10n for DEBUG=1 deosn't work since bug 929172 landed.

Comment 12

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

and I feel a little bit sad because it took a lot of time to implement and no one found this issue in 3 months...

Comment 13

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

filed bug 1006950 for DEBUG=1 test case

Comment 14

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

try server for gecko + gaia

https://tbpl.mozilla.org/?tree=Try&rev=1fab5e42364a

Comment 15

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

travis-ci:

https://travis-ci.org/mozilla-b2g/gaia/builds/24607693

Comment 16

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Hi Pike,

today I fix the multilocale issue for DEBUG=1 mode and I also verified it with/without environment variable GAIA_INLINE_LOCALES=0 GAIA_CONCAT_LOCALES=0

so please let me know if you have any question, I really hope this commit can be landed in this week

thanks!

Comment 17

[Axel Hecht [:Pike]]

Comment on attachment 8410751 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/18556

I ended up doing a full code review, it seems to be almost a rewrite. So some of this might not be regressions or changes.

As in the PR, it'd be nice if the order of dependencies were consistent in Makefile.

In multilocale.js, it looks like localizeManifest would overwrite the title and description for en-US with the value in en-US, if that was locally present? Doesn't sound right. Can you check what happens if a localizer has a partial translation of a manifest.properties? Like, description is there, but name isn't.

In webapp-zip.js, I think you can remove localesFile, and the check for it?

That's the stuff I had, I confess I stopped looking in-depth at the point where the PR reached httpd.js :_/

I think this is a feedback+ with nits, I don't think I need another pass over this.

Comment 18

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Pike, I didn't change that part of code in this pull request so I think that is another bug, I filed bug 1007485 and let's fix on there.

Comment 19

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

merged, thanks!

https://github.com/mozilla-b2g/gaia/commit/a29259475596c3080199aa54985c177d4f21199c