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)
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.
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 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•9 years ago
|
||
Feel free to take my node-build-diff.sh verification script at
https://gist.github.com/rickychien/da640f608a05fc7d0de8
Assignee | ||
Comment 4•9 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•9 years ago
|
Flags: needinfo?(poirot.alex)
Attachment #8664147 -
Flags: review?(poirot.alex)
Comment 5•9 years ago
|
||
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•9 years ago
|
Attachment #8664147 -
Flags: review?(poirot.alex)
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 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•9 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•9 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)
Comment 10•9 years ago
|
||
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•9 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•9 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)
Comment 13•9 years ago
|
||
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•9 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 15•9 years ago
|
||
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•9 years ago
|
||
Many thanks!
Landed in master:
https://github.com/mozilla-b2g/gaia/commit/605edea466140090c701ce39d841279123186553
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.
Description
•