1191770 - Add a way to preprocess apps to enable/disable Firefox Sync at build time

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Sync, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

We need a build time way to remove/add the Sync related code from Gaia just in case we want to ship a release without Sync support.

Comment 1

[Sean Lee [:seanlee][:weilonge]]

We can define a line in Makefile (at Gaia root) like this:
GAIA_DATA_SYNC?=0

The default value given to 0 is to disable DataSync by default.

System app uses a similar way to override source codes 
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/build/build.js#L112

If there is any file has to be changed for disabling/enabling data sync, using string replace technique is a possible method to modify the specific file.

However, it is a pretty hacky way.

Comment 2

[Sean Lee [:seanlee][:weilonge]]

Attachment: patch_build_script_way.diff

In this WIP patch, we can use ENABLE_DATA_SYNC=1 to enable data sync feature in Settings app.

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8647597 [details] [diff] [review]
patch_build_script_way.diff

>From 0bf19e9af42a82eec6976fade26e172a8b562b9c Mon Sep 17 00:00:00 2001
>From: Sean Lee <[e-mail]>
>Date: Thu, 13 Aug 2015 11:25:28 +0800
>Subject: [PATCH] Use ENABLE_DATA_SYNC to enable/disable DataSync feature in
> Settings app.
>
>---
> Makefile                         |  4 ++++
> apps/settings/build/build.js     | 41 ++++++++++++++++++++++++++++++++++++++++
> apps/settings/elements/root.html |  2 ++
> apps/settings/index.html         |  6 ++++++
> 4 files changed, 53 insertions(+)
>
>diff --git a/Makefile b/Makefile
>index 8d8b817..8ac1b4e 100644
>--- a/Makefile
>+++ b/Makefile
>@@ -86,6 +86,9 @@ PRODUCTION?=0
> GAIA_OPTIMIZE?=0
> GAIA_DEV_PIXELS_PER_PX?=1
> 
>+# For Data Sync
>+ENABLE_DATA_SYNC?=0
>+
> # Parallel build for multicores CPU
> P?=1
> 
>@@ -518,6 +521,7 @@ define BUILD_CONFIG
>   "NO_LOCK_SCREEN" : "$(NO_LOCK_SCREEN)", \
>   "SCREEN_TIMEOUT" : "$(SCREEN_TIMEOUT)", \
>   "SYSTEM" : "$(SYSTEM)", \
>+  "ENABLE_DATA_SYNC" : "$(ENABLE_DATA_SYNC)", \
>   "GAIA_PORT" : "$(GAIA_PORT)", \
>   "GAIA_LOCALES_PATH" : "$(GAIA_LOCALES_PATH)", \
>   "GAIA_INSTALL_PARENT" : "$(GAIA_INSTALL_PARENT)", \
>diff --git a/apps/settings/build/build.js b/apps/settings/build/build.js
>index e28e6a5..f054eb5 100644
>--- a/apps/settings/build/build.js
>+++ b/apps/settings/build/build.js
>@@ -172,12 +172,53 @@ SettingsAppBuilder.prototype.writeGitCommit = function(options) {
>   }
> };
> 
>+SettingsAppBuilder.prototype.enableDataSync = function(options) {
>+  function processHtml(enable, content){
>+    if (!enable) {
>+      return ;
>+    }
>+
>+    var replaced = content.
>+      replace('<!--DATASYNC_BLOCK_START', '', 'g').
>+      replace('DATASYNC_BLOCK_END-->', '', 'g');
>+    return replaced;
>+  }
>+
>+  var enable = options.ENABLE_DATA_SYNC === '1';
>+
>+  // Modified File List:
>+  // ./index.html
>+  // ./elements/root.html
>+  var appPath = options.APP_DIR;
>+  var indexHtmlFile = utils.getFile(appPath, 'index.html');
>+  var rootHtmlFile = utils.getFile(appPath, 'elements', 'root.html');
>+
>+  var stagePath = options.STAGE_APP_DIR;
>+  var indexHtmlStageFile = utils.getFile(stagePath, 'index.html');
>+  var rootHtmlStageFile = utils.getFile(stagePath, 'elements', 'root.html');
>+
>+  if (enable) {
>+    utils.log('DATA SYNC: enable');
>+
>+    var indexContent = utils.getFileContent(indexHtmlStageFile);
>+    var replacedIndexContent = processHtml(enable, indexContent);
>+    utils.writeContent(indexHtmlStageFile, replacedIndexContent);
>+
>+    //var rootContent = utils.getFileContent(rootHtmlFile);
>+    //var replacedRootContent = processHtml(enable, rootContent);
>+    //utils.writeContent(rootHtmlStageFile, replacedRootContent);
>+  } else {
>+    utils.log('DATA SYNC: disable');
>+  }
>+};
>+
> SettingsAppBuilder.prototype.execute = function(options) {
>   this.writeGitCommit(options);
>   this.writeDeviceFeaturesJSON(options);
>   this.writeSupportsJSON(options);
>   this.writeFindMyDeviceConfigJSON(options);
>   this.writeEuRoamingJSON(options);
>+  this.enableDataSync(options);
> 
>   return this.executeRjs(options).then(function() {
>     this.executeJsmin(options);
>diff --git a/apps/settings/elements/root.html b/apps/settings/elements/root.html
>index fa61bf5..c0d925d 100644
>--- a/apps/settings/elements/root.html
>+++ b/apps/settings/elements/root.html
>@@ -143,12 +143,14 @@
>             <small id="findmydevice-desc" class="findmydevice-desc menu-item-desc"></small>
>           </a>
>         </li>
>+        <!--DATASYNC_BLOCK_START
>         <li>
>           <a id="menuItem-fxsync" class="menu-item" data-icon="download" href="#fxsync" data-l10n-id="fxsyncSection" aria-describedby="fxsync-desc">
>             <span data-l10n-id="fxsync"></span>
>             <small id="fxsync-desc" class="fxsync-desc menu-item-desc"></small>
>           </a>
>         </li>
>+        DATASYNC_BLOCK_END-->
>       </ul>
> 
>       <header>
>diff --git a/apps/settings/index.html b/apps/settings/index.html
>index b7daa3d..cc64a2f 100644
>--- a/apps/settings/index.html
>+++ b/apps/settings/index.html
>@@ -218,7 +218,9 @@
>     <link rel="import" href="/elements/firefox_accounts.html">
>     <link rel="import" href="/elements/achievements.html">
>     <link rel="import" href="/elements/findmydevice.html">
>+    <!--DATASYNC_BLOCK_START
>     <link rel="import" href="/elements/fxsync.html">
>+    DATASYNC_BLOCK_END-->
>     <link rel="import" href="/elements/frame.html">
>     <link rel="import" href="/elements/full_developer_mode.html">
>     <link rel="import" href="/elements/full_developer_mode_warning.html">
>@@ -365,7 +367,9 @@
>     <section is="findmydevice" role="region" id="findmydevice"></section>
> 
>     <!-- Accounts :: FxSync -->
>+    <!--DATASYNC_BLOCK_START
>     <section is="fxsync" role="region" id="fxsync"></section>
>+    DATASYNC_BLOCK_END-->
> 
>     <!-- Security :: Screen Lock :: PIN Input -->
>     <section is="screenLock-passcode" role="region" id="screenLock-passcode" class="dialog"></section>
>@@ -486,8 +490,10 @@
>     <div class="settings-dialog-overlay" hidden></div>
> 
>     <script src="js/startup.js"></script>
>+    <!--DATASYNC_BLOCK_START
>     <script src="js/datasync/hawk_creds.js"></script>
>     <script src="js/datasync/kinto.dev.js"></script>
>     <script src="js/datasync/synccrypto.js"></script>
>+    DATASYNC_BLOCK_END-->
>   </body>
> </html>
>-- 
>2.2.1
>

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Attachment: patch_settings_way.diff

After the discussion with Evelyn Hung who is the peer of Settings app, she gives me the suggestion of using mozSettings to keep enabling/disabling status of Data Sync.

This is exactly what Developer menu does in Device Information/More Information menu, and enabling Data Sync or not can be configured in customization/settings.json .

Here is the benefits of mozSettings way:
* QA don't have to rebuild Gaia to test Data Sync feature.
* Skip the hacky way to modify build script and replace comment in HTML.
* Leverage the original design in Settings app for the architecture consistency.

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Hi Evelyn,

Could you give us some suggestions about my comment 4? Thank you!

Comment 6

[Evelyn Hung]

As Sean said in comment 4, I didn't see adding a build configuration is a proper way. Although this is just a temporary code to avoid confusion, we could do more. Instead of hiding an incomplete feature at build time, we can make it easier for QA to test it. If we end up miss the train of 2.5, we can hide the entry point in developer menu.

Sean, please also lazy load the whole feature logic after checking the mozSetting, otherwise it may regress load time unintended.

Comment 7

[Michiel de Jong [:michielbdejong]]

OK, I see the advantages of dropping our 'build flag' idea and switch to a mozSettings approach. I do have two questions:
* Do we need to worry about the size of the built code?
* What about the synchronizer (background) app, can we make that entirely invisible, even if it has a subfolder of the apps/ folder?

Comment 8

[Sean Lee [:seanlee][:weilonge]]

Hey Michiel,

Thanks a lot that you moved here. :)

(In reply to Michiel de Jong [:michielbdejong] from comment #7)
> OK, I see the advantages of dropping our 'build flag' idea and switch to a
> mozSettings approach. I do have two questions:
> * Do we need to worry about the size of the built code?
I think I can not give you the exact answer for your questions (yes or no).
But if we really do worry about it, that won't be a problem.

Let's see the patch at attachment 8647823 [details] [diff] [review].

We only have to remove this line to disable Data Sync permanently
apps/settings/elements/root.html
        <li data-show-name="datasync.menu.enabled" hidden>

and remove the code only for Data Sync like this:
apps/settings/elements/root.html
apps/settings/js/fxsync.js
...

> * What about the synchronizer (background) app, can we make that entirely
> invisible, even if it has a subfolder of the apps/ folder?
For synchronizer (background) app, we can delete it directly in apps/synchronizer directory to reduce the size of the built code.

For scheduler (system) app, I think that's probably a similar way to Settings app. However, I have to think about the real design.

Comment 9

[Michiel de Jong [:michielbdejong]]

@seanlee: OK, I agree! Let's make it a config flag instead of a build flag.

Comment 10

[Sean Lee [:seanlee][:weilonge]]

Hey Michiel, Thanks for your comment! I will close this bug based on your comment 9, and the implementation is in bug 1168185.

Comment 11

[Michiel de Jong [:michielbdejong]]

In today's Gaia meeting, Tim Chien mentioned that there are build-time flags already, best example to look at: 'GAIA_KEYBOARD_ENABLE_USER_DICT'. Older examples (probably no longer in the codebase now) include SYSTEM_OFFLINE_BROWSER and ROCKET_BAR.

Comment 12

[Michiel de Jong [:michielbdejong]]

See https://github.com/mozilla-b2g/gaia/search?utf8=%E2%9C%93&q=GAIA_KEYBOARD_ENABLE_USER_DICT&type=Code

I will try to create the Synchronizer skeleton app using this pattern, and if that works, I think this bug can stay closed.

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Thank you Sean and Michiel. I am reopening this bug and moving it to the appropriate sprint.

Comment 14

[Michiel de Jong [:michielbdejong]]

OK, so most of the solution to this bug can be done by reading the 'ENABLE_DATA_SYNC' build option from each app's custom build script (see https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Build_System_Primer#Build_Script_for_apps), as Sean is already doing for the Setting app (see https://bugzilla.mozilla.org/attachment.cgi?id=8647597&action=diff).

That leaves one issue: how can we add/remove the entire Synchronizer app at build time?
option 1: Simply giving it a build script that copies zero files of ENABLE_DATA_SYNC is 0 doesn't work, because the rest of the build process assumes that at least the manifest was copied to build_stage.
option 2: Using preprocessor directives in build/config/phone/apps-engineering.list, is not readily possible, because this file is parsed by build/scan-appdir.js (not by build/utils-rpc.js), and this file is called from the Makefile with only a few environment variables (probably so that make can see what its output depends on). Changing the Makefile so that it passes our ENABLE_DATA_SYNC option to scan-appdirs doesn't scale to other build options that people may want to use from the preprocessor.
option 3: Adding a build option like EXCLUDE_APPS, and passing that to scan-appdirs would be a nice and easy enough solution
option 4: I also noticed that build/config/phone/apps-engineering.list includes apps/* but not dev_apps/*. So  an even easier option (with no need to change the build process at all) is to put our app in dev_apps/ instead of in apps/, and then we can land all our code there without affecting any of the built products. We could even add an apps-experimental.list so you can do 'GAIA_APP_TARGET=experimental make' to build it. I PoC'ed this approach here: https://github.com/michielbdejong/gaia/pull/1/files

What do you think? Which option shall I use for the Synchronizer app skeleton?

Comment 15

[Michiel de Jong [:michielbdejong]]

Oh, I thought of an option 5: Let an app's custom build/build.js script return true or false, where false indicates that the app was not copied to build_stage, and then the rest of the build process can take this into account. If we add that to the documentation on https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Build_System_Primer#Build_Script_for_apps then this would be a nice addition. Let me investigate this option.

Comment 16

[Michiel de Jong [:michielbdejong]]

I implemented a variation on option 3 in bug 1198427.

Comment 17

[Sean Lee [:seanlee][:weilonge]]

Attachment: New Build Script with File List object

Hey Fernando,

The attachment is a better design for customizing the file content for an app.
In fileList object, you can add more files to handle a specific operation like file content modification, removing file, etc.

In this example, when ENABLE_DATA_SYNC flag is not 1, DATA SYNC block in these two html files will be removed:
        utils.getFile(stagePath, 'index.html'),
        utils.getFile(stagePath, 'elements', 'root.html')

And the two files will be removed as well:
        utils.getFile(stagePath, 'js', 'fxsync.js'),
        utils.getFile(stagePath, 'elements', 'fxsync.html')

Please notice that all the files should be modified or removed in build_stage folder (options.STAGE_APP_DIR).

Please give me some suggestions for this patch. Thank you.

Comment 18

[Sean Lee [:seanlee][:weilonge]]

Hey Michiel,

Could you help to give me a feedback? Thank you :)

Comment 19

[Michiel de Jong [:michielbdejong]]

Comment on attachment 8652727 [details]
New Build Script with File List object

It looks to me like this code will change the developer settings view in production, even when ENABLE_DATA_SYNC is 0. Otherwise, looks good! :)

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Sean Lee [:seanlee] from comment #4)
> Created attachment 8647823 [details] [diff] [review]
> patch_settings_way.diff
> 
> After the discussion with Evelyn Hung who is the peer of Settings app, she
> gives me the suggestion of using mozSettings to keep enabling/disabling
> status of Data Sync.
> 
> This is exactly what Developer menu does in Device Information/More
> Information menu, and enabling Data Sync or not can be configured in
> customization/settings.json .
> 

I don't think we need a way to enable/disable Firefox Sync from the developer menu. This is not a feature for developers and in any case it should be enabled by default. The cases where we might need to disable it are:

- We don't meet the 2.5 deadline.
- We identify a last minute security or functionality issue before shipping the product.
- We (or a partner) wants to ship a product without Firefox Sync. For example, the Red-Square (feature phone) or the TV projects may not want to add Firefox Sync to their products. We need to provide them with a way to remove the Firefox Sync feature from their builds.

> Here is the benefits of mozSettings way:
> * QA don't have to rebuild Gaia to test Data Sync feature.

As I said before, this feature should be enabled by default, so QA won't need to do any change on the regular builds.

> * Skip the hacky way to modify build script and replace comment in HTML.
> * Leverage the original design in Settings app for the architecture
> consistency.

This is not specific to Settings only. Firefox Sync is a wider feature. For now we identified that we will be adding code to the Settings app, but also to the System app and potentially the FTU app. We will also be adding a new hidden app (Synchronizer). The mechanism for removing this feature will need to take that into account.


(In reply to Evelyn Hung [:evelyn] from comment #6)
> As Sean said in comment 4, I didn't see adding a build configuration is a
> proper way. Although this is just a temporary code to avoid confusion, we
> could do more. Instead of hiding an incomplete feature at build time, we can
> make it easier for QA to test it. If we end up miss the train of 2.5, we can
> hide the entry point in developer menu.
> 

As I mentioned above, a missed deadline is not the only reason to require a way to remove a feature

> Sean, please also lazy load the whole feature logic after checking the
> mozSetting, otherwise it may regress load time unintended.

I'd like to avoid any runtime check to decide if we need to load or not a specific piece of code if possible. These checks are error prone and affects performance. We already suffered this on Settings with the Firefox Accounts entry in the past.

(In reply to Michiel de Jong [:michielbdejong] from comment #7)
> OK, I see the advantages of dropping our 'build flag' idea and switch to a
> mozSettings approach. I do have two questions:
> * Do we need to worry about the size of the built code?

Lucky for us, this is a minor issue. On user builds, preinstalled apps are stored on the /system partition. The situation would be different if these apps would be stored on the /data partition which is where user data, installed apps, etc. are stored and the one that rapidly grows leaving users with low device storage situations [1].

So removing the Synchronizer app in our case is a nice to have, but not really mandatory.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=861898

> * What about the synchronizer (background) app, can we make that entirely
> invisible, even if it has a subfolder of the apps/ folder?

Yes, we can. https://bugzilla.mozilla.org/show_bug.cgi?id=1182001#c12

Comment 21

[Sean Lee [:seanlee][:weilonge]]

(In reply to Michiel de Jong [:michielbdejong] from comment #19)
> Comment on attachment 8652727 [details]
> New Build Script with File List object
> 
> It looks to me like this code will change the developer settings view in
> production, even when ENABLE_DATA_SYNC is 0. Otherwise, looks good! :)

I agree that Firefox Sync item in Developer menu should be removed, and I will do this in bug 1168185 when the design of build flag has the agreement.

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8652727 [details]
New Build Script with File List object

Thanks for doing this work Sean!

I added a few comments on the PR. It would be great to get a build peer feedback as well. Alexandre could you give us some feedback, please? I added a suggestion for an app preprocessing mechanism to Sean's PR, what do you think about that? Thanks!

Comment 23

[Sean Lee [:seanlee][:weilonge]]

Hi Ricky,

We would like to use build flag to enable or disable a module in Gaia.
When the module is disabled, the module related codes should be removed and won't be able to enable at runtime.

IFDEF_* and ENDIF_* are the common tag in HTML or JS code
How do you think if creating a common build module to pre-process these codes?

You can see Fernando's comment at my patch:
https://github.com/weilonge/gaia/commit/c78272b8b666d7d1fc226ceb94c4e009e5792c63#commitcomment-12899089

Could you help to give a feedback for this?

Thank you! :)

Comment 24

[Ricky Chien [:rickychien] (inactive)]

Absolutely agree the point with Fernando since there already exists similar tools in front-end world to do the same thing.

Here are some of my suggestions:

* Fernando is right, we should extract that preprocess feature as a common build module.
* It's better to take such preprocess responsibility in app's build script, so app's owner can import preprocess modules on demand in their build script. It means that preprocess step will not live in pre-app or post-app, it should be invoked during app's building step (a.k.a apps' build script).
* Thus, we don't have to create preprocess.json for listing necessary files, but parsing it through build-time.

Comment 25

[Sean Lee [:seanlee][:weilonge]]

Attachment: build flag prototype

Hi Ricky, Fernando,

Could you give the patch a feedback?

There are two commits in this PR. Please see 'build flag prototype'.
https://github.com/weilonge/gaia/commit/d7a6d916734e74035b3c5c76450d7bf3e0b362be

I will rebase them after your feedback.

Thank you!

Comment 26

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8656477 [details] [review]
build flag prototype

My general impression was that it was quite good. Please set r?=me.

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8656477 [details] [review]
build flag prototype

Excellent job Sean!

I left a few comments on the PR. But in general, the approach looks good to me. Thank you again for working on this.

Comment 28

[GitHub Autolander Bot]

Attachment: [gaia] weilonge:seanlee/DataSync/master/Bug1191770 > mozilla-b2g:master

Comment 29

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8657052 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191770 > mozilla-b2g:master

Hi Ricky,

Could you help to review my PR? Thank you!

Comment 30

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8657052 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191770 > mozilla-b2g:master

Hey Fernando! The code is modified based on your suggestion. Please help to provide a feedback again. Thank you!

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8657052 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191770 > mozilla-b2g:master

Looks great! Thank you.

Once this lands, it would be great if you could update the MDN doc about the Gaia build system with this information and also send an email to dev-b2g/dev-gaia announcing that we have a new way to enable/disable features.
 
Some possible improvements that can be done as follow ups:

- Support for IFNDEF and ELSE.
- preprocessor.execute could take a list of flags instead of a single flag.

Comment 32

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8657052 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1191770 > mozilla-b2g:master

Thanks for your contribution in build system Sean \o/

Almost of that seems good to me. I've leaved some comments on GH so please fix it before landing. Free feel to ask me if you have any unsure part of these refactoring. thanks!

Comment 33

[Sean Lee [:seanlee][:weilonge]]

Hi Ricky,

Thanks a lot for your review and suggestions! :)
Based on Fernando's comment, I would like to add CSS support with the similar strategy.
The comments in CSS codes will be removed when the preprocessor executes, so IFDEF/ENDIF comments is removed as well.

Could you help to share what's the right place to execute Preprocessor? Thank you again.

SettingsAppBuilder.prototype.execute = function(options) {
  this.writeGitCommit(options);
  this.writeDeviceFeaturesJSON(options);
  this.writeSupportsJSON(options);
  this.writeFindMyDeviceConfigJSON(options);
  this.writeEuRoamingJSON(options);

  return this.executeRjs(options).then(function() {
    this.enableDataSync(options);  /// <<<<  Preprocessor 
    this.executeJsmin(options);
  }.bind(this));
};

Comment 34

[Ricky Chien [:rickychien] (inactive)]

Preprocessor is able to change HTML or JS content so it's better to be placed behind executeRjs. I prefer to wrap enableDataSync as a thenable method and chain it prior to executeRjs.

Comment 35

[Sean Lee [:seanlee][:weilonge]]

Thank you, Ricky! I will apply your suggestion at comment 34 to bug 1168185 because that's the real case to use preprocessor module.

Comment 36

[Sean Lee [:seanlee][:weilonge]]

landed at master: https://github.com/mozilla-b2g/gaia/commit/522b99088ffe49b6dcb17b1e9f4be090ec753247
gaia-test: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=df6624afb5d6960f00925f412d29c6c08db7b405