964216 - Use build_stage to build keyboard app

Status: RESOLVED FIXED

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

Description

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

+++ This bug was initially created as a clone of Bug #956184 +++

Before Bug #956184 comment 1 could come true, I can make some contribution first by having the current keyboard app using build_stage feature of the Gaia build script.

Comment 1

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

WIP https://github.com/timdream/gaia/compare/keyboard-build?expand=1

Comment 2

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

Update: I've successfully build keyboard app with build_stage, but the manifest change will not be updated by DEBUG=1 profile.

Also, CONFIGURE/README files need some update too.

I will ask for review when all of these are fixed.

Comment 3

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

I am now stuck on having build script pick up the built manifest.webapp. It always includes the original one. It's useless to redirect file reference of manifest.webapp in utils-xpc.js because the file does not exist yet :'(.

I am working with yuren to figure out whether or not it's possible to break the dependency and make app Makefiles run first.

Comment 4

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

With Yuren's help, we are using another build step to overwrite the one out-of-zip manifest with build_stage/manifest.webapp. 

I am trying to verify if the manifest will be correctly localized in this case.

Comment 5

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

OK, everything magically works.

local.mk:

GAIA_DEV_PIXELS_PER_PX=1.5
#PRODUCTION=1
MOZILLA_OFFICIAL=1
GAIA_KEYBOARD_LAYOUTS=en,zh-Hant-Zhuyin,el,de,fr,pl,ru,zh-Hans-Pinyin,ko
LOCALE_BASEDIR=locales/
LOCALES_FILE=locales/languages_mine.json

commands:

git checkout master
make clean
make
rm -rf /tmp/keyboard-before
mv profile/webapps/keyboard.gaiamobile.org /tmp/keyboard-before
open /tmp/keyboard-before/application.zip # Unzip the file in to ./application/
make clean
DEBUG=1 make
rm -rf /tmp/keyboard-before-debug
mv profile-debug/webapps/keyboard.gaiamobile.org /tmp/keyboard-before-debug

git checkout keyboard-build
make clean
make
rm -rf /tmp/keyboard-after
mv profile/webapps/keyboard.gaiamobile.org /tmp/keyboard-after
open /tmp/keyboard-after/application.zip
make clean
DEBUG=1 make
rm -rf /tmp/keyboard-after-debug
mv profile-debug/webapps/keyboard.gaiamobile.org /tmp/keyboard-after-debug

and the results are:

$ diff -r /tmp/keyboard-before-debug /tmp/keyboard-after-debug
(nothing changed)
$ diff -r /tmp/keyboard-before /tmp/keyboard-after
Only in /tmp/keyboard-before/application: CONFIGURE
Only in /tmp/keyboard-before/application/locales-obj: ar.json
Only in /tmp/keyboard-before/application/locales-obj: fr.json
Binary files /tmp/keyboard-before/application.zip and /tmp/keyboard-after/application.zip differ

The ar.json and fr.json are files gitignore'd that I didn't clean up. Not copying CONFIGURE is intentional.

It looks like keyboard-config.js modification to in-zip manifest will always be overwritten by mutlilocale.js, and the out-of-zip manifest will always remain unlocalized, with keyboard-config.js modifications. I am ended up simply working against a bug in the build system to ensure what's localized is kept localized, and vise versa.

Yuren, could you take a look of the patch?

Jan, David, I would like to make sure this gets worked on and landed by the time Taipei returns from holiday. The only nits to be done would be squash the patches and amend some comments like CONFIGURE and README.

Comment 6

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

"The out-of-zip manifest will always remain unlocalized, with keyboard-config.js modifications." This should be addressed as an i18n bug. :(

Comment 7

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

I squash 5 commit to read and it looks good, there are some suggestion:
1. JS_RUN_ENVIRONMENT is never used, we should remove it. (Makefile:L1-3)
2. GAIA_BUILD_DIR need to start by file:///, so it doesn't work in Makefile:L6

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch

I squashed the commits and applied what Yuren said in this pull req.

Built the keyboard using the keyboard makefile and couldn't find anything that went different than on current master.

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

Oh and changed CONFIGURE to reflect the changes. Let's see if Travis passes.

Comment 10

[Jan Jongboom [:janjongboom] (inactive)]

Travis was green, I had to fix some jshint/jslint configs due to the file moving. Plus fix one broken test that assumed things about the filesystem. https://github.com/mozilla-b2g/gaia/commit/a77ae31a900f562b2ba3086336dc76168ad8414d

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Pointer to Github PR https://github.com/mozilla-b2g/gaia/pull/15839

This PR fix the following issue found at build stage.

run-js-command  install-gaia
Exception: ReferenceError: env is not defined
getEnvPath@file:///home/jaoo/dev/gaia/build/utils-xpc.js:692
@file:///home/jaoo/dev/gaia/build/install-gaia.js:7
@-e:1

make: *** [install-gaia] Error 3

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8368010 [details] [review]
Pointer to Github PR https://github.com/mozilla-b2g/gaia/pull/15839

r=me for a quick fix

NI Yuren to see if it's possible to unit test this

Comment 13

[Julien Wajsberg [:julienw]]

landed the fix in master: d93ca888cab7882e53433c21d09b3d0c579580b4

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

PRODUCTION=1 make builds fail to show the keyboard.

Only in application-master: CONFIGURE
Only in application-master: WD..
Only in application-master: build_stage
diff -r application-new/index.html application-master/index.html
12c12
<   <!-- <script defer="" type="text/javascript" src="js/keyboard.js"></script> --><script type="text/javascript" defer="" src="/Users/janjongboom/repos/gaia/build_stage/keyboard/gaia_build_defer_index.js"></script>
---
>   <!-- <script defer="" type="text/javascript" src="js/keyboard.js"></script> --><script type="text/javascript" defer="" src="./gaia_build_defer_index.js"></script>
diff -r application-new/settings.html application-master/settings.html
14c14
<   <!-- <script defer="" src="js/settings/settings.js"></script> --><script type="" defer="" src="/Users/janjongboom/repos/gaia/build_stage/keyboard/gaia_build_defer_settings.js"></script>
---
>   <!-- <script defer="" src="js/settings/settings.js"></script> --><script type="" defer="" src="./gaia_build_defer_settings.js"></script>

I'll undo the patch and look into it tomorrow

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

Reverted https://github.com/mozilla-b2g/gaia/commit/45decfe73113ffc604981bd6056922fd59ed7f8f

Comment 16

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

Thanks for taking care of this.

Comment 17

[Jan Jongboom [:janjongboom] (inactive)]

Found a problem in webapp-optimize with using both the JS optimize step and a build dir specified in gaia_build.json on production builds. Just never bubbled up because all apps that use the build_stage don't have defer'ed javascripts.

Tested debug build in FF nightly and production builds in B2G desktop and Keon and keyboard works on all of them. Also clean diff on production profile now. Waiting for Travis to run at https://github.com/mozilla-b2g/gaia/pull/15857.

Comment 18

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch v2

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

Relanded https://github.com/mozilla-b2g/gaia/commit/d8bdd6e84ccbb9027f811e42373cbdd53977980a

Comment 20

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

Jan, I build a user build a few hours ago with localization, layouts other than Number are all gone. I think we should still back this out and find a way to make sure the out-of-zip manifest is properly fills with keyboard layout data.

Comment 21

[Jan Jongboom [:janjongboom] (inactive)]

What is your make command?

Comment 22

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

(In reply to Jan Jongboom [:janjongboom] from comment #21)
> What is your make command?

I am using https://gist.github.com/timdream/7716684 and only flashes system partition.

The file /system/b2g/webapps/keyboard.gaiamobile.org/manifest.webapp does not include any keyboard layout information. I am not sure if that's the cause of the bug (and whether or not if it's caused by this patch.)

Comment 23

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

master revert: 33eb812eadb61f76744103105e099972a8255843

People have reported the same problem on dev-gaia so I am backing this out.

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

Installing the 2/1 Peak nightly from Geeksphone I get no keyboard besides the numeric one, even after I choose to overwrite the userdata partition.

Comment 25

[Jan Jongboom [:janjongboom] (inactive)]

Hmpf, looks similar to what I have. I'll sort it out this week.

Comment 26

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

After backing out the patch and rebuilt, I can see layout config present in
out/target/product/peak/system/b2g/webapps/keyboard.gaiamobile.org/manifest.webapp

So this is indeed to patch that cause the problem and we do need layout config in out-of-zip manifest.

Comment 29

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

Stealing it back; just came back from holiday in Taipei.

Comment 30

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

yes we can do unit tests if add app path into rule |build-test-unit|, I filed bug 968052 for it.

Comment 31

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

It is unclear to me why B2G failed to generate a correct profile, and the config it sent to Gaia build script. I am going do investigate that and hopefully re-land this today.

Comment 32

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

I can now confirm the issue only happen when we run make with -j > 1, which is what B2G build system do by default.

I am going to try to see if it's possible to set a dependency in which make will understand.

Comment 33

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

Fixed with the following amendment.

diff --git a/Makefile b/Makefile
index 0840900..f07c772 100644
--- a/Makefile
+++ b/Makefile
@@ -1006,7 +1006,7 @@ adb-remount:
 # Generally we got manifest from webapp-manifest.js which is execute in
 # applications-data.js unless manifest is generated from Makefile of app.
 # so we will copy manifest.webapp if it's avaiable in build_stage/
-copy-build-stage-manifest:
+copy-build-stage-manifest: app-makefiles
        @$(call run-js-command, copy-build-stage-manifest)
 
 build-test-unit: $(NPM_INSTALLED_PROGRAMS)

Comment 34

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

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

Comment 35

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

On a second thought, it doesn't make sense for me to wait for Travis CI tests because they didn't catch the problem at first place.

master: https://github.com/mozilla-b2g/gaia/commit/ce212ba54f36284db84068f82af0c790ceb2c3ff

Comment 36

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

I just realized localization of out-of-zip manifests of all apps in the build_stage are gone. I will file and fix that as a follow-up bug in hours.