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

Input redirection error on Windows #2

Closed
SeanSnyders opened this issue May 29, 2017 · 7 comments
Closed

Input redirection error on Windows #2

SeanSnyders opened this issue May 29, 2017 · 7 comments
Assignees
Labels

Comments

@SeanSnyders
Copy link

SeanSnyders commented May 29, 2017

On Win 10 x64.

using versions:

node 7.4.0
npm 4.5.0
"shelljs": "0.7.7",
shelljs-plugin-sleep": "0.1.0",

Trying to do:

var shell = require('shelljs');
require('shelljs-plugin-sleep');
shell.sleep(5);

called via npm as a script.
It works fine on macOS 10.12.* but fails on Win 10 x64 with error:

ERROR: Input redirection is not supported, exiting the process immediately.
ShellJS: internal error
Error: Command failed: timeout 5
ERROR: Input redirection is not supported, exiting the process immediately.

    at checkExecSyncError (child_process.js:490:13)
    at Object.execSync (child_process.js:530:13)
    at Object.execSleep [as sleep] (<repo>\node_modules\shelljs-plugin-sleep\index.js:8:11)
    at Object.sleepImpl (<repo>\node_modules\shelljs-plugin-sleep\index.js:35:9)
    at Object.sleep <repo>\node_modules\shelljs\src\common.js:383:25)
@nfischer
Copy link
Owner

@SeanSnyders Thanks for trying this out and reporting the error!

Looks like our CI might not be covering this case.

Any reason why you don't have a C++ compiler installed? That'd be a good workaround until this is fixed.

This should be resolved by #1, I just need to weight the tradeoffs against unix sleep. If #1 is suitable, then that test case should be cross-platform, so we can prevent regressions in the future.

@nfischer nfischer self-assigned this May 30, 2017
@nfischer nfischer added the bug label May 30, 2017
nfischer added a commit that referenced this issue May 31, 2017
This adds a better fallback approach for Windows and systems lacking the
`sleep` CLI utility. This relies on spawning a Node.js process designed
to sleep for the desired time.

This has a tradeoff that it typically sleeps for longer than the desired
time, but it's available on all systems and is more predictable than
`timeout` for Windows. This makes busy-waiting obsolete, so that section
and test case have been removed.

This should still avoid blocking the CPU, which is a more critical
concern.

Fixes #1, #2
@nfischer
Copy link
Owner

@SeanSnyders can you try out that branch? Does it help out your issues?

$ npm install nfischer/shelljs-plugin-sleep#feat-better-fallback

@SeanSnyders
Copy link
Author

SeanSnyders commented May 31, 2017

I can have a look at that branch and see.

Any reason why you don't have a C++ compiler installed? That'd be a good workaround until this is fixed.

I indeed have a c++ compiler installed on my Win 10 x64 computer: MS VS2015.

In general I have several issues with sleep functionality in general between macOS and Windows.
Especially the async vs not async functionality and when it is called from other packages like Electron.
To get consistent behaviour in the several cases I have tested, I find that on Windows I need to use the fallback directly (i.e. childProcess.execSync) and on macOS it seems to use the Promise as expected. Still need to make sure about the sync vs. async on Windows....

@nfischer
Copy link
Owner

nfischer commented Jun 1, 2017

I indeed have a c++ compiler installed on my Win 10 x64 computer: MS VS2015.

Seems like npm isn't able to pick up on it. This module depends on the sleep package, which is a C++ native extension. If I recall correctly, compilation happens during npm install. So if we're using a fallback approach, that means that npm couldn't find a C++ compiler to build the package.

But I could be mistaken, I'm not well-versed in native extensions.

@SeanSnyders
Copy link
Author

My other native extensions compile fine, e.g. Edge.

The typical system sleep command/api is only available on *nix platforms (unistd.h), not Windows.
But I see you depend on https://github.com/erikdubbelboer/node-sleep which has some implementation of a 'sleep' function for windows: https://github.com/erikdubbelboer/node-sleep/blob/master/sleep.cc#L11

One needs to test that specifically to see if the correct function does indeed get called in that library.

@nfischer
Copy link
Owner

nfischer commented Jun 2, 2017

My other native extensions compile fine, e.g. Edge.

I've only seen us use the fallback functions if node-sleep fails to install. This usually means the native extension didn't compile.

Can you provide output for ls -R path/to/your/project/node_modules/sleep/ (or the equivalent Win command)?

The typical system sleep command/api is only available on *nix platforms (unistd.h), not Windows.

I misunderstood you. To me, "command" means shell command (i.e. man 1 sleep), I didn't realize you meant the function (i.e. man 3 sleep).

One needs to test that specifically to see if the correct function does indeed get called in that library.

Right now integration tests pass. Ideally, it shouldn't matter if they're using node-sleep or the fallback, as long as things work on appveyor.

I can have a look at that branch and see.

Any word on if the branch fixes things for you? If so, I'll land the change as the primary fallback.

nfischer added a commit that referenced this issue Oct 31, 2017
* feat: better fallback for Windows

This adds a better fallback approach for Windows and systems lacking the
`sleep` CLI utility. This relies on spawning a Node.js process designed
to sleep for the desired time.

This has a tradeoff that it typically sleeps for longer than the desired
time, but it's available on all systems and is more predictable than
`timeout` for Windows. This makes busy-waiting obsolete, so that section
and test case have been removed.

This should still avoid blocking the CPU, which is a more critical
concern.

Fixes #1, #2

* Bump engines value, since we didn't support v0.11 anyway
@nfischer
Copy link
Owner

Fix is out, this should resolve issues.

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

No branches or pull requests

2 participants