lifecycle: don’t warn about PATH for symlinked node #14374
Conversation
addaleax
force-pushed
the
fix-symlinked-node-scripts-prepend-node-path
branch
from
de7b489
to
a66616b
Compare
|
Updated the PR a fixed patch, this should do better in CI |
Apply a `fs.realpath()` check to the found `node` executable to mirror the fact that `process.execPath` is always a real path on modern Node.js versions, too. Fixes: npm#14372
addaleax
force-pushed
the
fix-symlinked-node-scripts-prepend-node-path
branch
from
a66616b
to
63a5fa6
Compare
| checkPath({ | ||
| withDirOfCurrentNode: 'extra-node', | ||
| prependNodePathSetting: 'warn-only', | ||
| symlinkNodeInsteadOfCopying: true |
There was a problem hiding this comment.
so it turns out this isn't going to work consistently across Windows platforms, depending on system configurations and stuff. Could we skip this entirely on Windows? (we have an isWindows utility for this)
There was a problem hiding this comment.
(the skip is probably better off inside checkPath itself)
There was a problem hiding this comment.
@zkat The only way to skip a test with node-tap’s API that I know of is setting skip: [true/string] in the test options, and that’s what I’ve seen in the wild so far. I totally agree that it would be better off inside checkPath, and if you know how to do that off the top of your head, feel free to show me or edit my changes yourself :)
|
|
||
| test('make sure there is no warning with a symlinked node and warn-only detection', function (t) { | ||
| checkPath({ | ||
| withDirOfCurrentNode: 'extra-node', |
There was a problem hiding this comment.
I originally made this a boolean+ for the sake of not adding more arguments, but since you're switching to a config object, do you mind making withDirOfCurrentNode a plain bool, and adding an extra config for the extra-node bit?
Apply a
fs.realpath()check to the foundnodeexecutable tomirror the fact that
process.execPathis always a real path, too.Fixes: #14372
/cc @zkat