Closed Bug 1207073 Opened 9 years ago Closed 9 years ago

RUN_ON_NODE=1 make is broken

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rickychien, Assigned: rickychien)

Details

Attachments

(1 file, 1 obsolete file)

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.
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)
Feel free to take my node-build-diff.sh verification script at https://gist.github.com/rickychien/da640f608a05fc7d0de8
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.
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.
Attachment #8664147 - Flags: review?(poirot.alex)
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)
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
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 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+
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)
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+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: