900288 - Implement node's child_process API

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P3)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Would be nice to implement node's child_process API:
http://nodejs.org/api/child_process.html#child_process_child_process

Implementation using jsctypes & no external dependencies has already being done, so this will only require forking and adjusting implementation to match node's API.

We'll probably need it for cfx-js, unless we find some other workarounds.

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Presumably we should be able to adapt this implementation:
http://hg.mozilla.org/ipccode/file/tip

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: Child Process

Hey Alex, the child process module is close enough to being finished. A few intermittent errors that I still need to look at, and some Windows issues that don't consistently work, but wanted to run this by you to get your thoughts. Also, I know you're using subprocess, and this has some of my changes, but if you need access to it, maybe we can move that elsewhere!

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8350380 [details] [review]
Child Process

I'd really like to see the diff from the original versions of subprocess*.js(m).

Very preliminary feedback is that you need the fix for the maverick crash.

Also note that I was having failures when using tip of ipc hg repo for the simulator... so there can be some bad changesets in the "recent" history.

You can find our fork over here:
https://github.com/mykmelez/jetpack-subprocess/

Note that in addition to the maverick crash we have some other older tweaks, like an encoding bugfix, automagick DISPLAY variable handling,... Take a look at repo history up to Sep 25, 2012 where we switched from the binary xpcom version to the jsctype one.

That would be handy if you can get it landed shortly. I already submitted a patch to land the simulator to mozilla central, see bug 944451. It comes with a full copy of our subprocess fork. But I'm already in process of getting it review (already did one review roundtrip), so we may just land it as-is. But if the review last longer than this bug, I may strip it down a lot!!

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Nice! Glad to see there are tests for it too. I'll do a diff of the changes I added -- should I send a PR to the jetpack-subprocess repo or in the simulator which is using it?

Forgot about the Maverick issue. Can you provide any more information on that? I don't have access to a Maverick machine at the moment.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Nevermind, I see your comments, but where should contributions go? Your fork, Myk's fork, or in the simulator? It may make more sense to add this to a common module we can both use without duplicating in `./toolkit/devtools/`, yeah?

Pinging Myk and Dave on how, if, and where we can have this common module!

Comment 6

[Myk Melez [:myk] [@mykmelez]]

ochameau may have a better idea, but I would send a PR to his jetpack-subprocess repo <https://github.com/ochameau/jetpack-subprocess>!  Then, once it lands there, I'll merge the changes into my fork <https://github.com/mykmelez/jetpack-subprocess>, which the Simulator currently depends upon as a submodule.  And then we'll update the submodule dependency (if the Simulator still lives in the GitHub repo) or generate a diff and drive that into mozilla-central (if the Simulator has landed in mozilla-central by then).

Comment 7

[Alexandre Poirot [:ochameau]]

I'm not sure that's any usefull to push your version to any git repo.
Tell me if I'm wrong, but it looks like you are aiming to land it to m-c so that it ships with firefox?
So that all these external repos/libraries should be deprecated and all applications using one of these libraries should be tweaked to use this new lib.

As said in previous comment, I'm not make this bug being a blocker for bug 944451 (landing the simulator in m-c). So I'll most likely land the simulator addon as-is, with its own copy of subprocess, living in the /b2g/simulator folder or something alike. It won't be shared by any other code.
You shouldn't really take care of that. Once your lib lands, I'll refactor the simulator code hosted on m-c to use your lib and get rid of the old one. FYI, once the simulator lands on m-c, code hosted on github will be almost frozen. (It will only and still be used to spawn new builds of 1.2 and 1.3 simulators with updated gecko and gaia.)

So to make it simple, land it somewhere we can all use it on m-c, but ensure cherry picking all fixes we have in our simulator forks of subprocess (the ones mentioned in comment 3).

Comment 8

[Dave Camp (:dcamp)]

I'll defer to ochameau and myk on where to land this.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Alex, sent a PR with my changes -- looks like most of my changes were abstracted around in our child_proc wrapper, or made redundant with other changes you've implemented: https://github.com/ochameau/jetpack-subprocess/pull/9

So once child_proc lands, would you just consume that via the simulator itself? (require('child-process')) or the underlying subprocess module? (require('child-process/subprocess'), something like that)

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8350380 [details] [review]
Child Process

All tests and implementation fixed up for both unix/windows! Currently using the version of subprocess forked from Alex's repo with my changes (sent in the PR), ready for review!

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8350380 [details] [review]
Child Process

Looks good regarding simulator usage. Subprocess usage also looks good.
But I'd like to delegate child_process.js review to someone from the sdk team,
as I'm no expert in the node module.

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8350380 [details] [review]
Child Process

Thanks Alex! Lets talk about what to do once this lands regarding sharing code between Simulator and SDK.

Adding gozala for r?

Comment 13

[Alexandre Poirot [:ochameau]]

Review ping, I'd really like to drop this code from the simulator and use sdk one instead!

BTW, have you tried pushing this patch to try, filesystem tests sounds like something that can easily fail on test slaves...

Comment 14

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Alexandre Poirot (:ochameau) from comment #13)
> Review ping, I'd really like to drop this code from the simulator and use
> sdk one instead!
>

Do you wanna it enough to take over the review ?

> 
> BTW, have you tried pushing this patch to try, filesystem tests sounds like
> something that can easily fail on test slaves...

Comment 15

[Alexandre Poirot [:ochameau]]

I'm not the best candidate to review child_process.js.
I'm not up to date with sdk code guidelines, I never used node child process api so it would take me quite significant time to review it, or review it poorly.
But you can consider that I reviewed subprocess and the usage of it.

Isn't this work any usefull for the sdk??

Comment 16

[Alexandre Poirot [:ochameau]]

So I'll land subprocess builtin into the simulator on monday (bug 944451).
Note that I had to fix some stuff in it during the review.
Triming comments and removing the hacky sizeof() thing (see attachment 8370756 [details] [diff] [review]).

Comment 17

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/62eab53f55396adf00a81e3231bb30065b9c70ff
Bug 900288 - Implement Child Process

https://github.com/mozilla/addon-sdk/commit/3fe178d82804726e23845cea046659c068cd8fe8
Merge pull request #1330 from jsantell/implement-child_process

Fix Bug 900288 Implement child_process, r=@gozala

Comment 18

[David Bruant]

<3
I was wondering. If you aim at Node interoperability, why not importing their test suite?
In https://github.com/joyent/node/tree/master/test/simple I see lots of tests starting with "test-child-process-" (all MIT licence apparently). Importing these tests would both save efforts from writing tests and would ensure some level of interoperability with Node.

Comment 19

[Jordan Santell [:jsantell] [@jsantell]]

In some cases, that's doable -- for buffer, that was mostly done with some changes (as we do not support all the same encodings as node), and there are different environments available, diff test suites, etc., but mostly work. Child process was a bit different, and running into some testing failures on CI (due to availability of calling dynamically generated bash/batch scripts in a profile dir, which has been a fun time..), so maybe we will borrow more testing from there.

For more simple modules (path, url, utils), this is most certainly the way to go, but child process felt like it's own, wild beast :)

Comment 20

[Jordan Santell [:jsantell] [@jsantell]]

Thanks for the tip, David -- was seeing some errors in WinXP and Linux on this push on CI, and after using some tricks from Node's tests, was able to fix them. I wrote my own bash/batch scripts to pipe stdin->stdout, which was more than difficult across all the different build systems. Node just uses 'cat' and 'more' directly:

spawn(isWindows ? 'more' : 'cat')

Worked great, although required a full path to the utilities (node inherits the env of the current process, whereas we can't really do that, so we can't resolve 'cat' for example based off of the process PATH, as of now).

Thanks!