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

Update build.js #1290

Closed
wants to merge 1 commit into from
Closed

Update build.js #1290

wants to merge 1 commit into from

Conversation

gobwas
Copy link

@gobwas gobwas commented Mar 31, 2014

Is it necessary the cp.spawn calling in /lib/build.js? Seems it can be simplified to cp.exec to be compatible in windows environment.

Is it necessary the ```cp.spawn``` calling in ``/lib/build.js`? Seems it can be simplified to ```cp.exec``` to be compatible in windows environment.
@stucox
Copy link
Member

stucox commented Mar 31, 2014

cc @doctyper – as the original author, is there any reason why we need to use .spawn()?

@doctyper
Copy link
Member

Wait, child_process.spawn is not compatible on Windows?

@gobwas
Copy link
Author

gobwas commented Mar 31, 2014

@doctyper yes, it triggers ENOENT error. Some discussion is here.

@gobwas
Copy link
Author

gobwas commented Apr 2, 2014

What about this issue?

@doctyper
Copy link
Member

doctyper commented Apr 2, 2014

According to the docs, child_process.exec does not take an stdio option. The exec call returns a buffer rather than a stream. This means that executing this command via exec will not pipe the output to the current process. You'll only be able to see the stdout output after the command is finished. This would break the feedback flow in tools like grunt-modernizr.

Also, it appears exec has a maximum buffer setting. It may be overridden, but the buffer size in this case varies, so it's probably not the method to use in this case.

@gobwas
Copy link
Author

gobwas commented Apr 3, 2014

Yes, you are right with it. I just thought that one really important thing is a callback calling after child is exit - looks like build variable, that is typed as Stream is not returned to anybody.

Anyway - any ideas how can I run this in Windows env?

Thanks.

@gobwas
Copy link
Author

gobwas commented Apr 23, 2014

Guys? =)

It is still breaks on Windows...

@stucox
Copy link
Member

stucox commented Apr 23, 2014

Ping @doctyper

@doctyper
Copy link
Member

Unfortunately, I don't know the best way to get this working in Windows. I'm not quite sure it's even on the road map.

I can tell you that at some point, we will switch build systems entirely and will move away from Grunt lock-in (see #1184). Once that lands, this issue will no longer exist.

patrickkettner added a commit to patrickkettner/Modernizr that referenced this pull request Jun 14, 2014
fixes Modernizr#1290

this gives us the least terrible of both worlds, using spawn unless
windows, where we use exec. That way everyone can build
@backflip
Copy link

Using Patrick's commit from above (patrickkettner@c99e4c1), I was able to make gulp-modernizr work on Windows.

@doctyper Is it true that «This would break the feedback flow in tools like grunt-modernizr» does not apply to gulp-modernizr or was I just lucky?

@patrickkettner
Copy link
Member

"This would break the feedback flow in tools like grunt-modernizr"

@doctyper can correct me if I am wrong, but I took that to mean that it just makes it seem like grunt-modernizr isn't doing anything (since exec only outputs when completed). So on a slower machine, with a larger build, it would seem that it is just hanging for a long time. Therefore, spawn is preffered

patrickkettner added a commit to patrickkettner/Modernizr that referenced this pull request Jun 17, 2014
fixes Modernizr#1290

this gives us the least terrible of both worlds, using spawn unless
windows, where we use exec. That way everyone can build
patrickkettner added a commit to patrickkettner/Modernizr that referenced this pull request Jun 17, 2014
fixes Modernizr#1290

this gives us the least terrible of both worlds, using spawn unless
windows, where we use exec. That way everyone can build
@patrickkettner
Copy link
Member

big ups to @sindresorhus for pointing out win-spawn

patrickkettner added a commit to patrickkettner/Modernizr that referenced this pull request Feb 22, 2015
fixes Modernizr#1290

this gives us the least terrible of both worlds, using spawn unless
windows, where we use exec. That way everyone can build
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.

5 participants