search

ivogabe / gulp-typescript

A TypeScript compiler for gulp with incremental compilation support.
MIT License
831 stars 129 forks source link

Replace execFile with exec to fix tests on Windows #617

Closed ivogabe closed 4 years ago

ivogabe commented 5 years ago

I got a mail that the tests failed on Windows because of the use of execFile. execFile executes a command without opening a shell, whereas exec will first open a shell and then execute a command. I think that a shell is needed for Windows, as it did work on Mac and Linux.

@lddubeau Do you agree with this change?

(The commit name should be the other way around..)

lddubeau commented 5 years ago

The change implies that on Windows, running a shell is desirable. Why is a shell desirable on Windows?

ivogabe commented 5 years ago

Without the shell it cannot find gulp:

Error: spawn gulp ENOENT
    at _errnoException (util.js:992:11)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:190:19)
    at onErrorNT (internal/child_process.js:372:16)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickDomainCallback (internal/process/next_tick.js:218:9)
lddubeau commented 5 years ago

On a Unix system it will start an unnecessary shell. That's a waste, but not a huge one. And this is test code rather than the main app. So I don't see major problems with this change.

If it were code in the main app, I'd try to preserve execFile. I've seen solutions of the form execFile(isRunningInWindows ? "gulp.cmd" : "gulp", ...)