RUN_ON_NODE=1 make is broken

RESOLVED FIXED

Status

Firefox OS
Gaia::Build
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rickychien, Assigned: rickychien)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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

2 years ago
Created attachment 8664147 [details] [review]
[gaia] rickychien:fix-build-on-node > mozilla-b2g:master
(Assignee)

Comment 2

2 years ago
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 =)
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 3

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

https://gist.github.com/rickychien/da640f608a05fc7d0de8
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Updated

2 years ago
Flags: needinfo?(poirot.alex)
Attachment #8664147 - Flags: review?(poirot.alex)
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.
(Assignee)

Updated

2 years ago
Attachment #8664147 - Flags: review?(poirot.alex)

Comment 6

2 years ago
Created attachment 8704978 [details] [review]
[gaia] rickychien:fix-run-on-node > mozilla-b2g:master
(Assignee)

Comment 7

2 years ago
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.
Attachment #8704978 - Flags: review?(scwwu)
(Assignee)

Comment 8

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

>https://github.com/mozilla-b2g/gaia/pull/31969
Attachment #8664147 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
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.
Attachment #8704978 - Flags: review?(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

2 years ago
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+
Attachment #8704978 - Flags: review?(scwwu) → review+
(Assignee)

Comment 12

2 years ago
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!
Flags: needinfo?(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.
Flags: needinfo?(stas)
(Assignee)

Comment 14

2 years ago
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.
Flags: needinfo?(stas)
Comment on attachment 8704978 [details] [review]
[gaia] rickychien:fix-run-on-node > mozilla-b2g:master

LGTM. Thanks, Ricky!
Flags: needinfo?(stas)
Attachment #8704978 - Flags: review?(stas) → review+
(Assignee)

Comment 16

2 years ago
Many thanks!

Landed in master:

https://github.com/mozilla-b2g/gaia/commit/605edea466140090c701ce39d841279123186553
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.