Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change the way to use spawn #142

Closed
wants to merge 1 commit into from
Closed

change the way to use spawn #142

wants to merge 1 commit into from

Conversation

jdneo
Copy link

@jdneo jdneo commented Sep 5, 2016

If the platform is win32 and path contains space, spawn will not execute correctly.
For example, if the path of slimerjs is C:\Program Files\slimerjs\sliemrjs.bat. The spawn will treat C:\Program as the command and Files\slimerjs\sliemrjs.bat... as args. The way to solve it is to use \S and \C args of the cmd, which is methioned by @progmars of the following post.

Relate Post: nodejs/node-v0.x-archive#25895

Relate Issue: #140

If the platform is win32 and path contains space, spawn will not execute correctly.

Relate Post: nodejs/node-v0.x-archive#25895

Relate Issue: #140
@jdneo
Copy link
Author

jdneo commented Sep 8, 2016

Hi, is there any one to check about this? Or should I give some additional information?

@baudehlo
Copy link
Owner

baudehlo commented Sep 8, 2016

Not many people use Windows.

On Sep 7, 2016, at 9:23 PM, 陈晟 [email protected] wrote:

Hi, is there any one to check about this? Or should I give some additional information?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

var phantom = spawn(options.path, args);
var phantom;
// if platform is win32 and the path contains space, using some hacks for the spawn method
if (process.platform == 'win32' && options.path.indexOf(' ') >= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, execution method on win32 should not depend on path content.

@puzrin
Copy link
Contributor

puzrin commented Sep 8, 2016

I don't care much about win, but don't have objections to support it better if someone provides good & complete solution.

  1. See code comments.
  2. CI should be configured to run tests on win platform (appveyor, for example)
  3. It's not clear how are you going to detect phantomjs port after spawn, because current commands are linux-specific

@jdneo
Copy link
Author

jdneo commented Sep 8, 2016

@puzrin yes, I agree about what you said. This spawn way seems weird and you want to make the code easy enough to be understood. But the issue really exist on wnidows platform, which is mentioned in this post.

Currently I have no idea about configuring CI to run tests on win platform. So I understand the objections to merge. Maybe I'll create another PR after I find a more elegant solution.

@baudehlo what you said is true to some extend. But I dont think it's a right way to refuse the PR. We are all trying to make this tool better and better right?

@jdneo jdneo closed this Sep 8, 2016
@puzrin
Copy link
Contributor

puzrin commented Sep 8, 2016

@jdneo see https://github.com/eslint/eslint/blob/master/appveyor.yml as example. There is nothing difficult.

@baudehlo what you said is true to some extend. But I dont think it's a right way to refuse the PR. We are all trying to make this tool better and better right?

Any code means additional maintenance cost (developpers time) in future. Bad code -> high cost, good code -> low cost. If you RP a clear code with verified config for appveyour, i see no problems to accept it.

@jdneo
Copy link
Author

jdneo commented Sep 8, 2016

@puzrin thanks, I'll take a look.

@baudehlo
Copy link
Owner

baudehlo commented Sep 8, 2016

@jdneo you misunderstood. I meant not many people can check it because they don't program this stuff on Windows. I certainly can't test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants