CB-9297 Parse xcode project syncronously to avoid issues with node v4

[vladimir-kotikov]

This fixes CB-9297

[nikhilkh]

This is a good one to fix. We might need a minor TOOLS release to get this out as node v4.0.0 might become popular. @stevengill Thoughts?

[nikhilkh]

@vladimir-kotikov Are there tests that break with this scenario failure or should we take the opportunity to add some here - not sure how easy that might be. Also, do all of our tests pass currently with node v4.0.

[stevengill]

I will test it out today. I agree about doing a minor tools release with this fix.

[vladimir-kotikov]

@nikhilkh, i've reworked tests for ios_parser so they work with real xcode project instead of mocking it.

Regarding second question - yes, all the tests are passing (ran cordova-lib, cordova-js, platforms tests and mobilespec).

[csantanapr]

Hi @vladimir-kotikov can you give explanation why the fix is to run Sync?

In general I'm not fan on Sync in nodejs, but would like to understand what's the root cause of the problem, and the solution

[csantanapr]

If it's something wrong with npm package xcode, we should open an issue and maybe help with a PR

[vladimir-kotikov]

The xcode's parse method uses child_process.fork API to run parser in a separate worker process, which then reports parsing result to parent process via child_process.send. However send became async in node v4 so this communication doesn't work properly now.

Moreover, parseSync method is more efficient (at least for small project) since it doesn't creates a a new instance of node process. From official child_process docs:

These child Node.js processes are still whole new instances of V8. Assume at least 30ms startup and 10mb memory for each new Node.js. That is, you cannot create many thousands of them.

Also compare execution time for tests from https://github.com/MSOpenTech/cordova-lib/commit/81eef5f7d0ee46a88c2a6836fd87c6c1c1ab216d: ~0.9 seconds for this branch (sync parsing) and ~2.7 second if applied to current master (async). The difference IMO is caused by cost of spawning node processes.

[pmuellr]

@csantanapr re: not a fan of sync in node.js

Sync becomes a problem if you're wasting precious cycles in a single operation, instead of using async to potentially handle multiple operations. For CLI tools, it's often the case that you need to do stuff serially anyway, so async actually becomes a problem, in terms of arranging those serial operations.

In this case, I think parsing sync is most likely fine. Especially given the timings provided - thx @vladimir-kotikov

Should also note that node v4 has added a callback to send() to fix this. Doesn't help for node < v3, but you could hack a 1 sec timeout to "emulate" it (under most conditions anyway). Smells bad, but prolly would work.

[csantanapr]

Thanks for explanation @vladimir-kotikov and thanks for your input @pmuellr
I agree with the change then after explanation.

I still think it should be nice to let the owner of Xcode about a problem of their code with Node4 and open a issue on their repo and suggestion on how to handle.

[vladimir-kotikov]

Ok, so this PR could be merged then?

Regarding fix for node-xcode, could you please take a look at vladimir-kotikov/node-xcode@beb12df. This should be enough to resolve the issue and have one code working in both node v0.12 and v4.

[csantanapr]

@vladimir-kotikov yes, This PR is good to merge to cordova-lib, that's what I meant with "I agree with the change then..." :-)

👍

[csantanapr]

We need to merge and release fast cli and lib with this bug, nodejs@4 is out and this is a major bug

[csantanapr]

@vladimir-kotikov yes change to node-code looks good send the PR to see what they want to do, we don't have to wait for a new node-xcode since we switched cordova-lib to use sync method which is preferred in this situation

[vladimir-kotikov]

Sent a PR to node-xcode with suggested fix: alunny/node-xcode#66

[csantanapr]

👍 @vladimir-kotikov