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

Incorrectly waits for stdin #136

Closed
Igloczek opened this Issue Sep 20, 2015 · 44 comments

Comments

Projects
None yet
@Igloczek

Igloczek commented Sep 20, 2015

Looks like ncu is currently not compatible with Node v4.1.0. I can run ncu, but nothing happens - cursor just goes to new line.

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Sep 21, 2015

I installed node v4.1.0 and ran ncu without a problem. All unit tests passing, too.

What happens when you run this command?

echo '{"dependencies":{"express": "1"}}' | ncu

Please provide any additional information you can about the problem.

@Igloczek

This comment has been minimized.

Igloczek commented Sep 21, 2015

echo '{"dependencies":{"express": "1"}}' | ncu
express  1  →  4 

ncu without any extra parameters works too, but it takes over 3 minutes to get results...

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Sep 22, 2015

It looks like it's working. It may take a long time if your package.json is long since npm view has to be called for each dependency to get the latest version number. If you know of a way to get latest versions of multiple dependencies in one call, we could speed this up considerably, but npm view does not appear to support it.

A user experience improvement would be to buffer the results to stdout so that you could see dependencies as they were found, instead of waiting till the end. This would require a significant refactor.

Sorry I don't have a good answer. Let me know if you think of something that I haven't!

@mrhyde

This comment has been minimized.

mrhyde commented Sep 22, 2015

I had pretty much the same issue last night, but it was apparently related to slow connection to npm registry. But as you said - it's not really intuitive at the moment and people can just stare there at blinking cursor and wondering that's going on. Is there any options for verbose output?

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Sep 23, 2015

It would be simple enough to print “Finding latest versions from npm…” at
the start.

On Tue, Sep 22, 2015 at 5:46 PM Mr. Hyde notifications@github.com wrote:

I had pretty much the same issue last night, but it was apparently related
to slow connection to npm registry. But as you said - it's not really
intuitive at the moment and people can just stare there at blinking cursor
and wondering that's going on. Is there any options for verbose output?


Reply to this email directly or view it on GitHub
#136 (comment)
.

@mrhyde

This comment has been minimized.

mrhyde commented Sep 23, 2015

Sounds good for me :)

@kalmanb

This comment has been minimized.

kalmanb commented Sep 24, 2015

Note I had the same issue, was burning lot so CPU and not returning. Cleaned/removed node_modules directory fixed for me.

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Sep 26, 2015

@kalmanb Thanks... I'm not sure what is happening there, but definitely is problematic. Let's keep an eye on it, and if we can reproduce it with a given install state then we can debug.

@etiktin

This comment has been minimized.

Contributor

etiktin commented Sep 26, 2015

Refactoring so we can check each dependency and print it's state as we get it sounds good to me. It will also make it easier to support interactive mode (#95).
For this to work we will need to rethink how we display satisfied dependencies that are not on the latest (currently we display them in a different group after all the unsatisfied).

Mean while, we could use the progress module or something similar, that allows us to show a progress bar in the CLI. The 'total' will be the number of dependencies and as we retrieve the latest version for each one, we will increase it by one. For example:

Finding latest versions [============--------] 60% 13.7s
@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Sep 26, 2015

Good points. We spent a bit of timing discussing satisfied text, but it's probably worth changing in order to get incremental installs and interactive mode. Let's discuss in #95.

Progress bar would be good!

@Daniel15

This comment has been minimized.

Daniel15 commented Oct 11, 2015

ncu without any extra parameters works too, but it takes over 3 minutes to get results...

Are you using npm v3? It had pretty bad performance problems until recently (see http://blog.npmjs.org/post/130359991775/npm-weekly-31-npm-3-speed-fixes-modules-demoed and https://github.com/npm/npm/releases/tag/v3.3.6) which could explain the slowness

@Igloczek

This comment has been minimized.

Igloczek commented Oct 12, 2015

@Daniel15 Nope, npm v2.14.4 + node v4.1.1.

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Oct 13, 2015

I'm tagging this unable-to-reproduce as aside from the performance issues mentioned above, which could give the appearance of nothing happening , I am unable to reproduce a concrete bug.

@mriehema

This comment has been minimized.

mriehema commented Oct 30, 2015

I'm using npm v2.14.4 and node v4.1.1 with Windows 7, 64bit and ncu is not working. It starts, but never quits. Without any logs I cant check why. Tried to use it with "time ncu", because it may take longer, but I aborted it after 2 hours waiting. My package.json just has 16 entries. Any idea?

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Nov 1, 2015

Hi Michael, thanks for reporting. Clearly it is getting hung up somewhere. We should add verbose logs so that we could at least see where it is hanging. Until then, the best we could do is actually debug from the source code.

Does it hang on an empty package.json? echo "{}" > package.json && ncu
Does it hang for some of the dependencies but not others? You could test this by removing some dependencies from your package.json and running ncu, or removing all of them and then adding them back in one-by-one.

Adding console.log's or more robust logging is going to be the best approach to really find where in the code it hangs, but since I cannot reproduce you may be on your own for the time being. Perhaps someone else who can reproduce would be willing to debug.

@mriehema

This comment has been minimized.

mriehema commented Nov 5, 2015

Does it hang on an empty package.json? echo "{}" > package.json && ncu

Yes.

I tried to debug and added some logging to function programRunLocal() before return:

    console.log(pkgFileName);
    console.log(pkgFile);
    console.log(pkgData);

Tested with an empty package.json returns:

    package.json
    undefined
    lib$rsvp$promise$$Promise {
    _id: 0,
    _label: undefined,
    _state: undefined,
    _result: undefined,
    _subscribers: [] }

And it never gets to analyzeProjectDependencies() and doesn't stop.

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Nov 5, 2015

Great information, thank you Michael. Maybe readFileAsync is not resolving? Can you tell if pkgData (promise) is resolving? Maybe add .catch to pkgData at the end of programRunLocal and see if an error is getting swallowed. Or try pkgData.then(console.log.bind(console, 'hi')) without the partial functional application, just to test a simpler case.

I wish I was able to reproduce and troubleshoot directly...

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Nov 10, 2015

@mriehema I added verbose logging to v2.40. Can you upgrade then report the output of the following command?

ncu --loglevel verbose
@mriehema

This comment has been minimized.

mriehema commented Nov 10, 2015

Awesome! I would upgrade and test it, if I could. :(

$ npm install npm-check-updates -g

> snyk@1.1.1 postinstall C:\Users\mriehema\AppData\Roaming\npm\node_modules\npm-check-updates\node_modules\snyk
> node cli/index.js help api-license

The use of Snyk's API, whether through the use of the 'snyk' npm package or otherwise, is subject to the terms & conditions specified here: https://snyk.io/policies/customer-agreement.pdf

C:\Users\mriehema\AppData\Roaming\npm\npm-check-updates -> C:\Users\mriehema\AppData\Roaming\npm\node_modules\npm-check-updates\bin\old-alias
C:\Users\mriehema\AppData\Roaming\npm\ncu -> C:\Users\mriehema\AppData\Roaming\npm\node_modules\npm-check-updates\bin\npm-check-updates

> npm-check-updates@2.4.0 postinstall C:\Users\mriehema\AppData\Roaming\npm\node_modules\npm-check-updates
> npm run snyk-protect


> npm-check-updates@2.4.0 snyk-protect C:\Users\mriehema\AppData\Roaming\npm\node_modules\npm-check-updates
> snyk protect

ENOENT: no such file or directory, open 'C:\Users\mriehema\AppData\Roaming\npm\node_modules\npm-check-updates\node_modules\bower\node_modules\handlebars\node_modules\uglify-js\.snyk-npm:uglify-js:20150824.flag'

npm ERR! Windows_NT 6.1.7601
npm ERR! argv "C:\\Program Files\\nodejs\\node.exe" "C:\\Users\\mriehema\\AppData\\Roaming\\npm\\node_modules\\npm-check-updates\\node_modules\\npm\\bin\\npm-cli.js" "run" "snyk-protect"
npm ERR! node v4.2.2
npm ERR! npm  v3.3.12
npm ERR! code ELIFECYCLE
npm ERR! npm-check-updates@2.4.0 snyk-protect: `snyk protect`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the npm-check-updates@2.4.0 snyk-protect script 'snyk protect'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the npm-check-updates package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     snyk protect
npm ERR! You can get their info via:
npm ERR!     npm owner ls npm-check-updates
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     C:\Users\mriehema\AppData\Roaming\npm\node_modules\npm-check-updates\npm-debug.log
npm ERR! Windows_NT 6.1.7601
npm ERR! argv "C:\\Program Files\\nodejs\\node.exe" "C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "install" "npm-check-updates" "-g"
npm ERR! node v4.2.2
npm ERR! npm  v2.14.7
npm ERR! code ELIFECYCLE

npm ERR! npm-check-updates@2.4.0 postinstall: `npm run snyk-protect`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the npm-check-updates@2.4.0 postinstall script 'npm run snyk-protect'.
npm ERR! This is most likely a problem with the npm-check-updates package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     npm run snyk-protect
npm ERR! You can get their info via:
npm ERR!     npm owner ls npm-check-updates
npm ERR! There is likely additional logging output above.
@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Nov 10, 2015

Snyk would be great if it worked out of the box, but it's not worth adding if it is going to break in some environments. I'll remove it.

raineorshine added a commit that referenced this issue Nov 10, 2015

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Nov 10, 2015

Try it now :). v2.4.1

@robertbaker

This comment has been minimized.

robertbaker commented Nov 11, 2015

@metaraine shouldn't synk be a dev-dep and not linked to install? npm install shouldn't do anything with synk

@mriehema

This comment has been minimized.

mriehema commented Nov 11, 2015

@metaraine Thanks for your support. So I checked it:

$ ncu --loglevel verbose
Initializing...
Running in local mode...
Finding local package file...
Waiting for package data on stdin...

BTW: I updated to node v4.2.2 and npm v2.14.7

So it looks like it still hangs up like I described. I added following code:

    pkgData.then(function(value) {
      console.log(value); // "Success!"
      throw 'oh, no!';
    }).catch(function(e) {
      console.log(e); // "oh, no!"
    }).then(function(e){
      console.log('after a catch the chain is restored');
    }, function () {
      console.log('Not fired due to the catch');
    });

Well... Same output as before. Just no response of the Promise.

@mriehema

This comment has been minimized.

mriehema commented Nov 11, 2015

Success! After some more debugging I tried to run it with explicit packageFile, and it works!

$ ncu --loglevel verbose --packageFile package.json
Initializing...
Running in local mode...
Finding local package file...
Getting installed packages...
Fetching latest versions...

All dependencies match the latest package versions :)

Looks like pkgData = require('get-stdin-promise'); is not working as expected.

@etiktin

This comment has been minimized.

Contributor

etiktin commented Nov 11, 2015

The question is why didn't it find the local package.json when you run it without parameters. "Finding local package file..." was supposed to succeed so it shouldn't have tried to read it from stdin.

By the way, we can replace get-stdin-promise with get-stdin since they added builtin support for promises in v5.0.0.

@mriehema

This comment has been minimized.

mriehema commented Nov 26, 2015

FYI: Updated to ncu v2.5.1 but it still doesn't work without --packageFile

$ ncu -l silly
Initializing...
Running in local mode...
Finding package file data...
Waiting for package data on stdin...

@raineorshine raineorshine changed the title from npm-check-updates never completes to Incorrectly waits for stdin Dec 6, 2015

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Dec 6, 2015

I'll keep this issue open since ncu still incorrectly waits for stdin in some environments. Adding an explicit --packageFile is a viable workaround.

As for the general performance issues, I fixed a bottleneck that should give a ~3x speed improvement (published in v2.5.3).

@mikemagnusw

This comment has been minimized.

mikemagnusw commented Dec 7, 2015

Confirming hang on Windows 8 and Cygwin64.

Windows, being backwards, has no native concept of a TTY. I'm not going to dig into it myself as specifying the package file explicitly of course works, but unless any underlying package/implementation of isTTY is Cygwin-aware i wouldn't expect things to work out well.

However, it does work as expected under the native 'command processor' cmd.exe

@mriehema

This comment has been minimized.

mriehema commented Dec 8, 2015

@mikemagnusw Thx for this infos.
Updated to v2.5.4 and used ncu in Git Bash. Still same error. Used in cmd.exe. Works.
Git Bash based on MinTTY. Found an issue according to STDIN, see mintty/mintty#56

@etiktin

This comment has been minimized.

Contributor

etiktin commented Dec 8, 2015

I can confirm it's happening with MinTTY (Cygwin/Babun). I tried it with node v4 and v0.12 and in both of them it's waiting for stdin.

This issue also happens when you try to run node with no parameters. In cmd it will enter node's REPL, but on MinTTY it will wait for stdin (you can force it to go to REPL by passing -i).

As a possible solution we could try to recognize we're running on Cygwin (e.g. check the env for the CYGWIN key, but there's probably a better way), and skip waiting for stdin. We can add a flag to force wait for stdin, for those rare cases when you want to pipe the data over.

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Dec 8, 2015

@etiktin Just fyi, adding a flag to force wait for stdin is merely the inverse of requiring --packageFile to not wait.

npm-check-updates must not be the only module that looks for input on stdin by default. There are so many unix modules that do that.

@etiktin

This comment has been minimized.

Contributor

etiktin commented Dec 8, 2015

There's an open issue about process.stdout.isTTY not working with MinTTY:
nodejs/node#3006

adding a flag to force wait for stdin is merely the inverse of requiring --packageFile to not wait.

I'm aware of that, but I think the inverse is more aligned with the way people actually use it (I assume most use ncu without params and expect it to read their package.json as opposed to piping the data).

@Daniel15

This comment has been minimized.

Daniel15 commented Dec 8, 2015

I think the inverse is more aligned with the way people actually use it (I assume most use ncu without params and expect it to read their package.json as opposed to piping the data).

+1

I think a lot of apps take a hyphen as the file name in order to read from stdin. See http://unix.stackexchange.com/questions/41828/what-does-dash-at-the-end-of-a-command-mean for example.

@tw0517tw

This comment has been minimized.

Contributor

tw0517tw commented Feb 15, 2016

for Git Bash users with MinTTY:
try winpty ncu.cmd
or add alias ncu='winpty ncu.cmd' to your .bashrc

default

@sant123

This comment has been minimized.

sant123 commented Apr 6, 2016

It works as the @tw0517tw way.

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Feb 12, 2017

Would someone who is on Windows and has to specify an explicit packageFile try out the following two commands and let me know the output?

  1. node -p process.stdin.constructor.name
  2. echo "something" | node -p process.stdin.constructor.name

Possible new insights from nodejs/node#2339 (comment)

@raineorshine raineorshine reopened this Feb 12, 2017

@mriehema

This comment has been minimized.

mriehema commented Feb 12, 2017

In GitBash:

$ node -p process.stdin.constructor.name
ReadStream

$ echo "something" | node -p process.stdin.constructor.name
stdin is not a tty

$ node -v
v6.9.5

$ git --version
git version 2.11.1.windows.1

In Windows Console (cmd), which I don't use:

C:\Users\Michael>node -p process.stdin.constructor.name
ReadStream

C:\Users\Michael>echo "something" | node -p process.stdin.constructor.name
Socket
@davidgilbertson

This comment has been minimized.

davidgilbertson commented Feb 17, 2017

I'm getting this problem too.

Windows 10, Git Bash, Node 7.1, ncu 2.10.2.

It works fine in cmd, and today is the first time I've ever had a problem after years of use.

process.stdin.isTTY returns true.

image

I get the same results as @mriehema in the comment above

P.S. awesome package
P.P.S. modern.ie @raineorshine if you want a Windows environment to test on

@raineorshine

This comment has been minimized.

Collaborator

raineorshine commented Feb 17, 2017

@davidgilbertson Thanks! What happens when you try specifying a package file explicitly with --packageFile package.json?

@dscho

This comment has been minimized.

dscho commented Feb 22, 2017

echo "something" | node -p process.stdin.constructor.name

Please note that in Git Bash it makes a difference whether you call node or node.exe. The form without .exe assumes that you want to use node.js interactively, and fakes a Win32 Console (which Git Bash usually does not have, so you won't see any output if you call node.exe without any parameters and expect an interactive console).

@mackr2015

This comment has been minimized.

mackr2015 commented Jul 4, 2017

Hey guys, this command worked out for me.
Using gitbash MinTTY.
Try running this command
ncu -u --packageFile package.json
npm-check-updates

@jersonjcnt

This comment has been minimized.

jersonjcnt commented Aug 29, 2017

step 1 : ncu --loglevel verbose --packageFile package.json
step 2 : ncu -u --packageFile package.json

@kylios

This comment has been minimized.

kylios commented Nov 28, 2017

I've had a similar issue when using ncu within a docker container in gitlab CI. It relates to this issue with process.stdin.isTTY, but I suspect there may be an unrelated bug here, so let me know if I should file a separate issue for this.

I was confused when running ncu -e 2 was correctly exiting with status code 1 from my shell, but in our CI pipeline it would consistently return status code 0, despite the package version table looking identical. I added many debugging statements and determined that the options object was identical in both cases: both environments had options.cli set to true and options.errorLevel set to 2, so the programError function should have been exiting with an erroneous status code. However, I noticed that I wasn't getting the message Dependencies not up-to-date in my docker environment, while I was seeing that message in my shell. Tracing this in both environments, I found that args.pkgFile was undefined in docker, but it was set to package.json in my shell. Weird!

To make a long story short, I traced the issue down to the findPackage function, and the process.stdin.isTTY check, which was evaluating differently from my docker CI environment and my shell environment. In docker, process.stdin.isTTY is false, so this if-statement was being entered, but of course I wasn't supplying any data into stdin, so it was falling back and calling pkgFile = findUp.sync(pkgFileName); after the isEmptyStdin check. This is where the bug is: pkgFile is being returned as undefined from findPackage, despite pkgFileName being correct and getPackageDataFromFile reading the correct file and returning the right contents. My suspicion is that the value of pkgFile is not exiting the closure correctly, or else the promise returned from findPackage is resolving too soon. Regardless, the solution is what has already been stated here: simply invoke the script with the --packageFile argument and avoid the whole stdin thing.

I hope this helped somebody's debugging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment