1207073 - RUN_ON_NODE=1 make is broken

Status: RESOLVED FIXED

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

Description

[Ricky Chien [:rickychien] (inactive)]

As title, RUN_ON_NODE=1 make is broken for some libraries issues.

It seems that there are some problems from jsdom and it will be fixed by upgrading to newer jsdom and nodejs.

Some API changes bring from nodejs 4.0 which are beneficial to jsdom, it makes sense to upgrade build system to use node 4.0, but we have to make sure it doesn't break js-marionette.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] rickychien:fix-build-on-node > mozilla-b2g:master

Comment 2

[Ricky Chien [:rickychien] (inactive)]

In this patch:

package.json

* Install jsdom instead of jsdom-nogyp for better dom operation support
* Moved packages witch are used by build system from dependencies to devDependencies

utils-node.js

* Remove jsdom-nogyp hack

How to verify it? 

RUN_ON_NODE=1 make    

it should work properly as usual

RUN_ON_NODE will enable nodejs run-time if such build script is able to run on node.js, or fallback to xpcshell run-time if build script doesn't support yet. So it will take a longer time than xpcshell run-time in this experimental stage.

Alex, would you please take a look for my patch if you have any question, please feel free to leave comment or ping me on IRC. thanks =)

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Feel free to take my node-build-diff.sh verification script at

https://gist.github.com/rickychien/da640f608a05fc7d0de8

Comment 4

[Ricky Chien [:rickychien] (inactive)]

I've already verified that patch by my node-build-diff.sh to compare two diff results. There are some installTimes differences but I think it's negligible.

Comment 5

[Alexandre Poirot [:ochameau]]

I am all but node expert, so I don't fully understand this modification.
Do you have any nodejs expert around that could look at that?
Tim? Greg?

Also, it looks like try is still orange.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] rickychien:fix-run-on-node > mozilla-b2g:master

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8704978 [details] [review]
[gaia] rickychien:fix-run-on-node > mozilla-b2g:master

I'll be Scott's mentor for reviewing and verifying this patch.

Scott, feel free to reach out to me if you any question during the reviewing.

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8664147 [details] [review]
[gaia] rickychien:fix-build-on-node > mozilla-b2g:master

>https://github.com/mozilla-b2g/gaia/pull/31969

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8704978 [details] [review]
[gaia] rickychien:fix-run-on-node > mozilla-b2g:master

Set review request to Stas as the reviewer for l20n.js.

Stas, I'd like to inform you we're currently migrating build system from xpc to node and node 4.x doesn't default support some of ES6 features such as restructuring assignment (it depends on V8 engine). Thus, I compiled build/l10n/l20n.js from babel's repl https://babeljs.io/repl/ to ES5 manually, and also we have to keep l20n.js running on ES5 environment temporarily until build system is able to fully support node and babel.

Comment 10

[Staś Małolepszy :stas]

Hi Ricky -- I'm on PTO tomorrow but I'll get to this on Tuesday morning. L20n.js is developed in a separate repo [1] and our build system supports transpilation targeting node.  Perhaps we can solve this in a systematic manner on our side which will make it much easier to update build/l10n/l20n.js in the future.

1: https://github.com/l20n/l20n.js

Comment 11

[Scott Wu [:scottwu]]

Comment on attachment 8704978 [details] [review]
[gaia] rickychien:fix-run-on-node > mozilla-b2g:master

Looks good from the diff results! As you have mentioned, the only differences are in the time-stamps, tag attribute orders, and file hashes. r+

Comment 12

[Ricky Chien [:rickychien] (inactive)]

All right, I found we've already enable ES6 transform options at https://github.com/l20n/l20n.js/blob/master/build/tasks/compat.js#L23-L34 and the l20n.js supports ES5 version as well if we build with `grunt release`.

I guess that l20n/dist/compat/node/l20n.js should be the correct version we want, and I'll copy that directly to gaia instead of this one. Stas, if I'm wrong please correct me, thanks!

Comment 13

[Staś Małolepszy :stas]

l20n/dist/compat/node/l20n.js is an experimental version of the l20n.js library for localizing node apps.  It doesn't contain the buildtime optimizations specific to Gaia and uses a simple fs.readFile for IO.

We need a version of dist/bundle/gaia/build/l20n.js which is transpiled to es5.  That would be dist/compat/gaia/build/l20n.js.   It still uses Gaia buildtime's IO, i.e. HTMLOptimizer.getFileByRelativePath, so it should be agnostic to the engine that runs it.

In https://github.com/l20n/l20n.js/commit/4ee0b6c29790a936f17bd7d00d12386ab5524198 I made dist/compat/gaia/build/l20n.js the default variant that we copy into Gaia's build/l10n/l20n.js.  I've tested with RUN_ON_NODE=1 make and it appears to be working, although I'm going to test a bit more to verify that some of the more intricate optimizations still work.

Comment 14

[Ricky Chien [:rickychien] (inactive)]

Thank you stas! I've updated my patch to dist/compat/gaia/build/l20n.js from latest l20n repo and it works as well with RUN_ON_NODE=1. I verified it with node-build-diff.sh and it looks good to me as well.

Feel free to take my diff script mentioned on comment 3 for verifying if you need.

Comment 15

[Staś Małolepszy :stas]

Comment on attachment 8704978 [details] [review]
[gaia] rickychien:fix-run-on-node > mozilla-b2g:master

LGTM. Thanks, Ricky!

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Many thanks!

Landed in master:

https://github.com/mozilla-b2g/gaia/commit/605edea466140090c701ce39d841279123186553