You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by vladimir-kotikov <gi...@git.apache.org> on 2015/09/10 16:54:59 UTC
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
GitHub user vladimir-kotikov opened a pull request:
https://github.com/apache/cordova-lib/pull/305
CB-9297 Parse xcode project syncronously to avoid issues with node v4
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-lib CB-9297
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/305.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #305
----
commit 8ffa3239dc2af2626bebaee265f8f9ff1a610130
Author: Vladimir Kotikov <v-...@microsoft.com>
Date: 2015-09-10T12:29:41Z
CB-9297 Parse xcode project syncronously to avoid issues with node v4
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by pmuellr <gi...@git.apache.org>.
Github user pmuellr commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-139631229
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-139294305
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-140734415
Sent a PR to node-xcode with suggested fix: https://github.com/alunny/node-xcode/pull/66
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-139293826
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-140033673
Ok, so this PR could be merged then?
Regarding fix for node-xcode, could you please take a look at https://github.com/vladimir-kotikov/node-xcode/commit/beb12df445646a8dd0d466131dd43552339c4206. This should be enough to resolve the issue and have one code working in both node v0.12 and v4.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by csantanapr <gi...@git.apache.org>.
Github user csantanapr commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-141170683
:+1: @vladimir-kotikov
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by csantanapr <gi...@git.apache.org>.
Github user csantanapr commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-140061087
@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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-139314186
I will test it out today. I agree about doing a minor tools release with this fix.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-139556202
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.
Although, `parseSync` method is even 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by csantanapr <gi...@git.apache.org>.
Github user csantanapr commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-139548204
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-139524490
@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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by csantanapr <gi...@git.apache.org>.
Github user csantanapr commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-140060516
@vladimir-kotikov yes, This PR is good to merge to cordova-lib, that's what I meant with "I agree with the change then..." :-)
:+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by csantanapr <gi...@git.apache.org>.
Github user csantanapr commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-139555231
If it's something wrong with npm package xcode, we should open an issue and maybe help with a PR
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-lib/pull/305
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by csantanapr <gi...@git.apache.org>.
Github user csantanapr commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-139655827
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: CB-9297 Parse xcode project syncronously...
Posted by csantanapr <gi...@git.apache.org>.
Github user csantanapr commented on the pull request:
https://github.com/apache/cordova-lib/pull/305#issuecomment-140060717
We need to merge and release fast cli and lib with this bug, nodejs@4 is out and this is a major bug
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org