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

ncu -g does not seems to work with custom node/npm installations. #1315

Open
3 tasks done
diegocr opened this issue Aug 19, 2023 · 12 comments
Open
3 tasks done

ncu -g does not seems to work with custom node/npm installations. #1315

diegocr opened this issue Aug 19, 2023 · 12 comments
Labels

Comments

@diegocr
Copy link

diegocr commented Aug 19, 2023

  • I have searched for similar issues
  • I am using the latest version of npm-check-updates
  • I am using node >= 14.14

Hi there,

at first it was not clear what was going on, but upon running ncu --verbose -g found there a options.prefix pointing to %APPDATA%\npm but that path here doesn't exists (because nodejs is not installed on the system drive)

I made a symlink and got ncu -g working again, but... perhaps ncu should be using the NODE_PATH environment variable for this "prefix" value (?)

fwiw, on ~/.npmrc i do have:

prefix=D:\\nodejs
cache=R:\Temp\npm-cache

and NODE_PATH does point to D:\nodejs\node_modules (the before mentioned symlink does point to this path)

Please let me know if i do have to provide further troubleshooting details, and/or if i can help in some other way.

Thanks!

@raineorshine
Copy link
Owner

raineorshine commented Aug 19, 2023

npm-check-updates does its best to guess the correct npm prefix based on the OS. Unfortunately, it looks like this is a regression from #297 (coming back to haunt us 7 years later 😅). While %APPDATA%\npm may still be appropriate as a default, it is supposed to get overridden by your specific npm prefix.

The original problem is documented in #146 and #163, with especially useful information in #163 (comment).

A few questions:

  • What do you get when you run npm config get prefix?
  • What do you get when you run which.exe npm?
  • What do you get when you run node -p process.execPath?

I made a symlink and got ncu -g working again, but... perhaps ncu should be using the NODE_PATH environment variable for this "prefix" value (?)

NODE_PATH is used for locating modules, and can contain multiple paths. What we need is the prefix. For what it's worth, the PREFIX environment variable works, although I would like to be able to autodetect the prefix without it if possible.

@diegocr
Copy link
Author

diegocr commented Aug 20, 2023

$ npm config get prefix
D:\nodejs

$ which.exe npm
/d/nodejs/npm

$ node -p process.execPath
D:\nodejs\node.exe

I'll try to debug what is going on with defaultPrefix(), at first glance the only reason to fallback to %APPDATA%\npm is if spawn() failed, but i didn't saw that message printed.

@diegocr
Copy link
Author

diegocr commented Aug 20, 2023

Okay, this turned interesting...

The spawn() call there does yield %APPDATA%\npm

On the generated log-file in npm-cache\_logs i can see this:

0 verbose cli D:\nodejs\node.exe D:\nodejs\node_modules\npm\bin\npm-cli.js
1 info using npm@9.6.7
2 info using node@v18.17.1
3 timing npm:load:whichnode Completed in 1ms
4 timing config:load:defaults Completed in 1ms
5 timing config:load:file:D:\nodejs\node_modules\npm\npmrc Completed in 2ms
6 timing config:load:builtin Completed in 2ms
7 timing config:load:cli Completed in 1ms
8 timing config:load:env Completed in 0ms
9 timing config:load:project Completed in 1ms
10 timing config:load:file:C:\Users\dc\.npmrc Completed in 1ms
11 timing config:load:user Completed in 1ms
12 timing config:load:file:C:\Users\dc\AppData\Roaming\npm\etc\npmrc Completed in 0ms
13 timing config:load:global Completed in 0ms
.....

at line 5 it did loaded D:\nodejs\node_modules\npm\npmrc where i do have:

prefix=${APPDATA}\npm

I suppose this is the default path that comes with all installations, therefore not from your defaultPrefix() function where the spawn() call is properly executed and it did succeed.

Now the interesting part... at line 10 it did tried to load C:\Users\dc\.npmrc which does NOT exists, so you may wonder how npm config get prefix was returning the correct path... my guess is because this spawn() call does not inherit the environment variables of the current shell/terminal, where i do have HOME defined and where i do have the .npmrc config file.

$ echo %HOME%
D:\msys2\home\dc

Here's the log-file from running npm config get prefix

0 verbose cli D:\nodejs\node.exe D:\nodejs\node_modules\npm\bin\npm-cli.js
1 info using npm@9.6.7
2 info using node@v18.17.1
3 timing npm:load:whichnode Completed in 2ms
4 timing config:load:defaults Completed in 1ms
5 timing config:load:file:D:\nodejs\node_modules\npm\npmrc Completed in 2ms
6 timing config:load:builtin Completed in 2ms
7 timing config:load:cli Completed in 1ms
8 timing config:load:env Completed in 0ms
9 timing config:load:file:C:\Users\dc\.npmrc Completed in 0ms
10 timing config:load:project Completed in 0ms
11 timing config:load:file:D:\msys2\home\dc\.npmrc Completed in 1ms
12 timing config:load:user Completed in 1ms
13 timing config:load:file:D:\nodejs\etc\npmrc Completed in 0ms
14 timing config:load:global Completed in 0ms
....

as you can see in line 11, the HOME environment variable is used.

fwiw, i do mainly use the git-for-windows's bash, but running the commands you asked me under a normal windows cmd terminal did yield the same results.

All that being said, i am not sure if you could come with some workaround for this given the above information, but in any case i will remove the symlink i made for %APPDATA%\npm and instead i will create one from D:\msys2\home\dc\.npmrc to C:\Users\dc\.npmrc which seems the more correct thing to do here.

@diegocr
Copy link
Author

diegocr commented Aug 20, 2023

Oh... i thought that was NodeJS's native spawn() implementation (aka, child_process), but just realized you're using the "spawn-please" package, which you're the creator of 😅

Perhaps i should file a ticket in that other repo (?) 😉

@raineorshine
Copy link
Owner

Very interesting. Thanks for investigating.

So if I'm following you correctly, the spawn-please call to npm config get prefix gives a different result than npm config get prefix in your shell. You suspect that it is due to $HOME not being defined or inherited from your system shell.

What do you get when you run node -p "console.log(process.env.HOME)"? That should tell us if the problem is in spawn-please or higher up.

Oh... i thought that was NodeJS's native spawn() implementation (aka, child_process), but just realized you're using the "spawn-please" package, which you're the creator of 😅

It's raineorshine all the way down... 🐢

@diegocr
Copy link
Author

diegocr commented Aug 20, 2023

Yes, you're following me correctly :)

$ node -p "console.log(process.env.HOME)"
D:\msys2\home\dc
undefined

Btw, AFAICT, Node's child-process.spawn does uses by default process.env to inherit from, hence i guess this is being lost / overridden somewhere around those additional spawn-packages (yours, or the node-cross-spawn one), i even tried to supply {env: process.env} as an option on the spawn() call at defaultPrefix() and it didn't work.

@raineorshine
Copy link
Owner

Thanks. That confirms the problem is with the spawn call.

After some further investigation, spawn option shell may be our ticket:

shell <boolean> - If true, runs command inside of a shell. Uses '/bin/sh' on Unix, and process.env.ComSpec on Windows. A different shell can be specified as a string. See Shell requirements and Default Windows shell. Default: false (no shell).

https://nodejs.org/api/child_process.html#child_processspawncommand-args-options

In fact, { shell: true } was cited as a solution to various Windows path issues on a few different occasions. Yet interestingly I never added it, as switching to cross-spawn solved the issue for most people. I'm now thinking that #297 was a overreach that could have been solved with { shell: true }.

I'll publish a prerelease with { shell true }. If that works for you, it'll make it into the next major version release (took risky for a patch).

@diegocr
Copy link
Author

diegocr commented Aug 20, 2023

Okay, hold, let me give it a try - as long it doesn't break running this through Bash on Windows we'll be good :)

@diegocr
Copy link
Author

diegocr commented Aug 20, 2023

er... nope, it doesn't work.

$ ncu -g
defaultPrefix().prefix =  C:\Users\dc\AppData\Roaming\npm

No dependencies.
--- npm.js	2023-08-20 15:20:39.816120000 +0200
+++ npm.js.sh	2023-08-20 15:19:16.644871000 +0200
@@ -460,3 +460,3 @@
     try {
-        prefix = await (0, spawn_please_1.default)(cmd, ['config', 'get', 'prefix']);
+        prefix = await (0, spawn_please_1.default)(cmd, ['config', 'get', 'prefix'], {shell: true});
     }
@@ -467,2 +467,4 @@
     }
+	console.log('defaultPrefix().prefix = ', prefix);
+	// process.exit(1)
     // FIX: for ncu -g doesn't work on homebrew or windows #146

@diegocr
Copy link
Author

diegocr commented Aug 20, 2023

Well, the same way the env option is overridden or whatever, this shell one might be as well (?)

Let me know if i should try to hackpatch any of the spawn-packages (didn't even looked at them, but i guess the issue is there)

@raineorshine
Copy link
Owner

raineorshine commented Aug 20, 2023

Dang.

So we could use process.execPath, however I don't think it's correct to overwrite npm config get prefix. As long as npm config get prefix is returning the wrong value, we have no way of differentiating a default install from a custom prefix. It would undoubtedly clobber someone's correct prefix.

Well, the same way the env option is overridden or whatever, this shell one might be as well (?)

Let me know if i should try to hackpatch any of the spawn-packages (didn't even looked at them, but i guess the issue is there)

Yeah, it's hard to tell what's going on. spawn sets env to process.env by default, and I don't see any reason why { shell: true } wouldn't pass through.

I'm open to ideas. Figuring out how to get npm config get prefix to return the correct value is the priority. You might have to experiment a bit on your machine since I'm not on Windows.

@diegocr
Copy link
Author

diegocr commented Aug 20, 2023

Yep, well for the time being i am happy with the .npmrc symlink, whenever i find more spare time i may get into the spawn packages to figure what is happening exactly.

Thanks for your time and help 🙇🏻 👍🏻

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