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

False, ncu lies: "All global packages are up-to-date :)" is not true. #293

Closed
ghost opened this issue Nov 13, 2016 · 35 comments
Closed

False, ncu lies: "All global packages are up-to-date :)" is not true. #293

ghost opened this issue Nov 13, 2016 · 35 comments

Comments

@ghost
Copy link

ghost commented Nov 13, 2016

If I use: ncu -g it sasys to me:

$ ncu -g
[..................] \ :
All global packages are up-to-date :)

If I use in my gulpfile.js this:

gulp.task('ncu', function () {
  exec('ncu -g', function (err, stdout, stderr) {
    console.log(stdout);
    console.log(stderr);
  });
});

it says to me:

[11:33:05] Starting 'ncu'...
[11:33:05] Finished 'ncu' after 13 ms

 bower      1.7.9  →   1.8.0
 eslint     3.6.0  →  3.10.0
 jshint     2.9.3  →   2.9.4

Why this behaviour?

Windows 10 x64
NodeJs 7.1.0

@raineorshine
Copy link
Owner

Hi! Thanks for reporting.

It is likely that exec is running ncu in the wrong working directory. Trying explicitly setting cwd.

@ghost
Copy link
Author

ghost commented Nov 14, 2016

But what cwd if I'm calling the -g option for global check?

@raineorshine
Copy link
Owner

Ah, good point. I'm sorry I overlooked that. Are you using the normal child_process.exec? I can investigate more. It seems as if it's not running the global flag.

@raineorshine
Copy link
Owner

It seems to be working as expected for me. Here are my results:

raine[ncu-293]$ type ncu
ncu is hashed (/Users/raine/.nvm/versions/node/v6.1.0/bin/ncu)

raine[ncu-293]$ npm -g prefix
/Users/raine/.nvm/versions/node/v6.1.0

raine[ncu-293]$ ncu -g
⸨░░░░░░░░░░░░░░░░░░⸩ ⠧ :
 npm                3.8.6  →  3.10.9 
 npm-check-updates  2.8.1  →   2.8.6 

And then with this test file:

const exec = require('child_process').exec

exec('type ncu', (err, stdout, stderr) => {
  console.log('type ncu: ', stdout)
})

exec('npm -g prefix', (err, stdout, stderr) => {
  console.log('npm -g prefix: ', stdout)
})

exec('ncu -g', (err, stdout, stderr) => {
  console.log('ncu -g:', stdout)
})

Results:

raine[ncu-293]$ node test.js 
type ncu:  ncu is /Users/raine/.nvm/versions/node/v6.1.0/bin/ncu

npm -g prefix:  /Users/raine/.nvm/versions/node/v6.1.0

ncu -g:
 npm                3.8.6  →  3.10.9 
 npm-check-updates  2.8.1  →   2.8.6 

There must be some difference between your running it in the terminal and running it with child_process.exec. Can you do what I did above to compare the location of ncu and the npm prefix between your terminal and the executing node process? Post the results here. If there is a difference, that could give us a clue into the differing behavior. If they are the same, we will have to get more creative :).

@timcosta
Copy link

This also occurs if you type ncu check instead of just ncu, which I do frequently because nsp requires check as an argument to run.

@raineorshine
Copy link
Owner

If you're able to run the commands above to check the prefix locations, I can try to troubleshoot more. Thanks!

@raineorshine
Copy link
Owner

Closing due to no activity, but let me know if the issue is still unresolved!

@borgogelli
Copy link

borgogelli commented Feb 12, 2019

@raineorshine , same problem for me under Ubuntu:

$ ncu -g
[..................] - :
All global packages are up-to-date :)
$ npm -v
6.5.0
$ ncu -v
2.15.0
$ lsb_release -a
Description:    Ubuntu 18.04.1 LTS
Release:        18.04

@raineorshine
Copy link
Owner

@borgogelli Thanks for reporting. Can you execute this code and share the results? That might help me troubleshoot.

const exec = require('child_process').exec

exec('type ncu', (err, stdout, stderr) => {
  console.log('type ncu: ', stdout)
})

exec('npm -g prefix', (err, stdout, stderr) => {
  console.log('npm -g prefix: ', stdout)
})

exec('ncu -g', (err, stdout, stderr) => {
  console.log('ncu -g:', stdout)
})

@raineorshine raineorshine reopened this Feb 12, 2019
@borgogelli
Copy link

borgogelli commented Feb 22, 2019

The output is:

type ncu:  ncu is /usr/local/bin/ncu

npm -g prefix:  /usr/local

ncu -g:
All global packages are up-to-date :)

ncu itself cannot upgrade global packages. Run the following to upgrade all global packages:

npm -g install

(same output for user and sudo)

@raineorshine
Copy link
Owner

It must be something other than the prefix. I am afraid I don't know how to reproduce the problem. It will require someone who is able to reproduce the problem to go into the npm-check-updates module code to troubleshoot.

@digeomel
Copy link

I seem to be having a similar issue in Windows.

λ ncu -g
[====================] 1/1 100%

 @angular/cli  6.0.8  →  8.3.12

ncu itself cannot upgrade global packages. Run the following to upgrade all global packages:

npm -g install @angular/cli@8.3.12

And then:

λ npm outdated -g --depth=0
Package                                     Current  Wanted  Latest  Location
@compodoc/compodoc                           1.1.10  1.1.11  1.1.11
@graphql-codegen/cli                          1.7.0   1.8.1   1.8.1
@graphql-codegen/fragment-matcher             1.7.0   1.8.1   1.8.1
@graphql-codegen/typescript-apollo-angular    1.7.0   1.8.1   1.8.1
@graphql-codegen/typescript-operations        1.7.0   1.8.1   1.8.1
bit-bin                                      14.3.0  14.4.1  14.4.1
ionic                                         5.4.1   5.4.4   5.4.4
node-gyp                                      5.0.3   5.0.5   6.0.0
nodemon                                      1.19.2  1.19.4  1.19.4
npm-check-updates                            3.1.23  3.1.24  3.1.24
phonegap                                      8.2.2   8.2.2   9.0.0
sass                                        1.22.12  1.23.0  1.23.0
typescript                                    3.6.3   3.6.4   3.6.4
webpack-bundle-analyzer                       3.5.2   3.6.0   3.6.0

It seems to me that ncu is reading the wrong npm list.
In fact, in my .npmrc I have setup a different path for the npm cache than the default, like this:

prefix=C:\Users\digeomel\AppData\Local\npm
cache=C:\Users\digeomel\AppData\Local\npm-cache

and I think that ncu is checking the roaming profile (\AppData\Roaming\) which is the default path for npm.
Is this the case?

@digeomel
Copy link

digeomel commented Jan 8, 2020

After reading the two threads carefully, I got it to work like this:
ncu -g --prefix /c/Users/geomedi/AppData/Local/npm
Still, I don't understand why it doesn't pick up the prefix by default.

@raineorshine
Copy link
Owner

@digeomel Glad you found a solution! I'm sure it would be a simple fix, just need someone to be generous enough to find the issue in the codebase since I am unable to reproduce it on my end.

@digeomel
Copy link

digeomel commented Jan 9, 2020

I've forked the repo and debugged it. I traced it down to this line:
https://github.com/tjunnone/npm-check-updates/blob/7b0d321a39b5325148d033e20b335ec6490ab98b/lib/package-managers/npm.js#L155
Which basically says that if the platform is win32 and I'm checking global packages and there's no prefix set in the enviroment (not sure which environment process.env refers to here, but it doesn't seem to be npm, seems to be the system/windows environment) then it sets the prefix to ${process.env.AppData}\\npm, which points to the RoamingProfile in Windows.

Now, the question is why?

PS. And on another peculiar note, what is that prefix.match('Cellar') code doing on line 150 above?

@raineorshine
Copy link
Owner

Thank you for the sleuthing @digeomel!

That line was added by @anantoghosh in 9be7b2f as described here.

It seems to be related to ncu's legacy use of npm.commands. Since ncu no longer relies on this, and always spawns npm as a child process, we should be able to remove this special case on Windows without negative repercussion.

@raineorshine
Copy link
Owner

I removed it and published on the next tag. Would love to have a couple Windows folks confirm.

npx npm-check-updates@next

@digeomel
Copy link

@raineorshine thank you for the quick response! 😄
I can confirm that ncu@next works as expected on both global and local packages on my Windows 10 system. 👍

On another note which I forgot to mention earlier, during my debugging I discovered that this spawn command:
https://github.com/tjunnone/npm-check-updates/blob/7b0d321a39b5325148d033e20b335ec6490ab98b/lib/package-managers/npm.js#L147
seems to be taking way too long (on my system at least). We're talking minutes, not seconds.
If the sole purpose of this is to get the npm config prefix, perhaps a library like https://www.npmjs.com/package/libnpmconfig could do the trick faster?

@raineorshine
Copy link
Owner

raineorshine commented Jan 13, 2020

@digeomel

$ npm config get prefix
/Users/raine/.nvm/versions/node/v12.13.0

libnpmconfig does not seem to return the prefix:

console.log(require('libnpmconfig').read())

...

FiggyPudding {
  cache: '/Users/raine/.npm',
  loglevel: 'warn',
  '@bit:registry': 'https://node.bit.dev',
  configNames: [ 'npmrc', '.npmrc' ],
  envPrefix: /^npm_config_/i,
  cwd: '/Users/raine/projects/npm-check-updates',
  globalconfig: '/Users/raine/.nvm/versions/node/v12.13.0/etc/npmrc',
  userconfig: '/Users/raine/.npmrc'
}

Repository owner deleted a comment from stoically Jan 13, 2020
@digeomel
Copy link

digeomel commented Jan 14, 2020

@raineorshine this does on my machine:

console.log(require('libnpmconfig').read().get('prefix'))

But I guess it does because it is explicitly set in my .npmrc. In your case you probably have none specified, so it gets the default.
So you're right, npm config get prefix is guaranteed to return the prefix in all cases.

How about importing npm as a dependency and doing something like this:

const npm = require('npm');

npm.load(function(err, npm) {
    if (err) {
        console.error("npm.load failed: ", err);
        process.exit(1)
    }

    console.log(npm.config.get('prefix'));
});

@raineorshine
Copy link
Owner

But I guess it does because it is explicitly set in my .npmrc. In your case you probably have none specified, so it gets the default.

We could always start with lbnpmconfig and then if that doesn't work do the spawn. Will speed it up for some people at least.

How about importing npm as a dependency

Last I knew requiring npm has been deprecated for many years. npm-check-updates v3 included changing from using npm.commands to spawning the npm cli. I believe the CLI is the only recommended use. Please point me towards some documentation if I am wrong about this.

@digeomel
Copy link

I think your solution of using libnpmconfig and reverting to spawning if there's no prefix is the best.
I found some discussions related to the use of either libnpmconfig or npm directly, but the consensus seems to be not to use npm because the api is not documented and subject to change:

zkat/pacote#156
angular/angular-cli#12350

In any case, thank you for your support! 👍

@raineorshine
Copy link
Owner

@digeomel Thanks for your help! Any interest in making a PR for this? Should be a very small change.

@digeomel
Copy link

Sure, I'll give it a try over the weekend!

@stoically
Copy link
Collaborator

stoically commented Jan 16, 2020

FWIW, it seems that npm uses the package find-npm-prefix under the hood, so maybe that works? Scratch that, that's only about looking up the node_modules folder.

@stoically
Copy link
Collaborator

stoically commented Jan 16, 2020

So the actual logic to get the global prefix appears to be this. It depends on process.execPath and goes one/two levels up, and that's also officially documented. I'd say that would be reasonable to manually implement.

@digeomel
Copy link

digeomel commented Jan 17, 2020

@stoically The problem I see with the code snippet from your link above:

  if (process.env.PREFIX) {
    globalPrefix = process.env.PREFIX
  } else if (process.platform === 'win32') {
    // c:\node\node.exe --> prefix=c:\node\
    globalPrefix = path.dirname(process.execPath)
  } else {
    // /usr/local/bin/node --> prefix=/usr/local
    globalPrefix = path.dirname(path.dirname(process.execPath))


    // destdir only is respected on Unix
    if (process.env.DESTDIR) {
      globalPrefix = path.join(process.env.DESTDIR, globalPrefix)
    }
  }

is that PREFIX is not a "global flag", it's not a system environment variable, it's only defined in .npmrc (in my case at least, I haven't read anything about it being defined at the system level).
So depending on what process.env points to, it may not be set. In the case of ncu, it seems to point to the system environment variables, and of course it doesn't find it there. In the case of npm above, maybe it points to npm itself?

@stoically
Copy link
Collaborator

process.env.PREFIX is just a way to override the prefix in npm, it isn't set by default. The default are the two else-branches that follow.

@digeomel
Copy link

digeomel commented Jan 17, 2020

Then it doesn't make sense, because on win32 it will just pick the directory where node is installed.
In my case node is installed on C:\Program Files\Nodejs\node, but the prefix defined in .npmrc is pointing to the local profile.
How can it even work then?

@stoically
Copy link
Collaborator

It's the npm default, so I guess it should work by default. But sure, a prefix configured in a .npmrc should take precedence over the default detection.

@digeomel
Copy link

But there is no reading of .npmrc on the code above. So we're missing something.

@stoically
Copy link
Collaborator

stoically commented Jan 17, 2020

The code there only sets the defaults, that's why the variable it's assigned to is called defaults. Later on npm merges in user-defined configurations, like e.g. provided by .npmrc's. So, to get that behavior in ncu, we could check the config read from libnpmconfig and if that doesn't have a prefix, fall back to the default global detection like npm does it instead of spawning the expensive npm cli.

@stoically
Copy link
Collaborator

However, just checking libnpmconfig for prefix and falling back to spawning npm like it's already done would be equally fine for me personally. Just wanted to point out that it could be avoided.

@neminovno
Copy link

Similar to #1127

@raineorshine
Copy link
Owner

Closing due to inactivity, but please let me know if this still an issue and we have a way to move forward on it.

@raineorshine raineorshine closed this as not planned Won't fix, can't repro, duplicate, stale Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants