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

Escape path-related npm options on Windows #181

Open
wants to merge 2 commits into
base: latest
from

Conversation

Projects
None yet
7 participants
@zkat
Copy link
Owner

commented May 4, 2018

This patch supersedes #138 and adds a bit more escaping, now that I actually managed to wrap my head around what was actually going on, and the different bizarre escaping things related to windows (oh god it's so bad).

I think this should iron out the main issue on Windows and solve some others that were lurking in the shadows, ready to strike.

/cc @brianpeiris @noahleigh

@n-leigh

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

@zkat
I ran npm test with this PR on my name with spaces Windows 10 VM and encountered some errors:

# Subtest: npx --always-spawn
    not ok 1 - Command failed: node "C:\Users\name with spaces\dev\npx\test\util\npx-bin.js" --always-spawn echo-cli hewwo
      ---
      stack: |
        ChildProcess.child.on.code (child.js:1:11367)
      at:
        line: 1
        column: 11367
        file: child.js
        function: ChildProcess.child.on.code
      isOperational: true
      stderr: "npx: installed 47 in 3.961s\ninternal/modules/cjs/loader.js:573\r\n    throw err;\r\n    ^\r\n\r\nError: Cannot find module 'C:\\Users\\name'\r\n    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:571:15)\r\n    at Function.Module._load (internal/modules/cjs/loader.js:497:25)\r\n    at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)\r\n    at startup (internal/bootstrap/node.js:228:19)\r\n    at bootstrapNodeJSCore (internal/bootstrap/node.js:575:3)\r\n"
      exitCode: 1
      test: npx --always-spawn
      source: |
        (function (exports, require, module, __filename, __dirname) { 'use strict'
      ...
    
    1..1
    # failed 1 test
not ok 1 - npx --always-spawn # time=4967.41ms
# Subtest: --node-arg works on Windows
    not ok 1 - Command failed: node "C:\Users\name with spaces\dev\npx\test\util\npx-bin.js" --quiet --node-arg --no-deprecation echo-cli hewwo
      ---
      stack: |
        ChildProcess.child.on.code (child.js:1:11367)
      at:
        line: 1
        column: 11367
        file: child.js
        function: ChildProcess.child.on.code
      isOperational: true
      stderr: "internal/modules/cjs/loader.js:573\r\n    throw err;\r\n    ^\r\n\r\nError: Cannot find module 'C:\\Users\\name'\r\n    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:571:15)\r\n    at Function.Module._load (internal/modules/cjs/loader.js:497:25)\r\n    at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)\r\n    at startup (internal/bootstrap/node.js:228:19)\r\n    at bootstrapNodeJSCore (internal/bootstrap/node.js:575:3)\r\n"
      exitCode: 1
      test: '--node-arg works on Windows'
      source: |
        (function (exports, require, module, __filename, __dirname) { 'use strict'
      ...
    
    1..1
    # failed 1 test
not ok 14 - --node-arg works on Windows # time=662.827ms

Investigating...

@n-leigh

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

I was able to make the errors go away by adding another escape:

existing = child.escapeArg(existing, false)

beneath

npx/index.js

Line 299 in c23c994

cmd = child.escapeArg(cmd, true)

This basically just wraps existing in quotes, but it seems to appease Windows' path requirements. Works on WSL too. It feels like a duct tape solution, but I'm not aware of all the potential edge-cases to be able to judge it for certain.

@zkat

This comment has been minimized.

Copy link
Owner Author

commented May 4, 2018

@noahleigh I think The Thing that's caused most of these problems is that child.spawn switches between shell and direct mode. We just see the issue most often in Windows, but using -c would be just as problematic in many of these cases, I think.

The refactor to make that not as painful is something I can't even think about right now, so I think this is a matter of figuring out how child.spawn() will actually be used and checking all of the arguments in those situations, when we generate args ourselves.

@zkat zkat force-pushed the zkat/windows-npm-escape branch from c23c994 to 92e5330 May 4, 2018

@zkat

This comment has been minimized.

Copy link
Owner Author

commented May 4, 2018

@noahleigh I pushed a patch because I realized there was a bug. Can you try it again with this branch and see if you still need the existing to be escaped, too?

@zkat zkat force-pushed the zkat/windows-npm-escape branch from 92e5330 to dee7f22 May 4, 2018

@n-leigh

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

@zkat
Getting similar errors:

# Subtest: npx --always-spawn
    not ok 1 - Command failed: node "C:\Users\name with spaces\dev\npx\test\util\npx-bin.js" --always-spawn echo-cli hewwo
      ---
      stack: |
        ChildProcess.child.on.code (child.js:1:11367)
      at:
        line: 1
        column: 11367
        file: child.js
        function: ChildProcess.child.on.code
      isOperational: true
      stderr: "npx: installed 47 in 5.345s\ninternal/modules/cjs/loader.js:573\r\n    throw err;\r\n    ^\r\n\r\nError: Cannot find module 'C:\\Users\"name'\r\n    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:571:15)\r\n    at Function.Module._load (internal/modules/cjs/loader.js:497:25)\r\n    at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)\r\n    at startup (internal/bootstrap/node.js:228:19)\r\n    at bootstrapNodeJSCore (internal/bootstrap/node.js:575:3)\r\n"
      exitCode: 1
      test: npx --always-spawn
      source: |
        (function (exports, require, module, __filename, __dirname) { 'use strict'
      ...
    
    1..1
    # failed 1 test
not ok 1 - npx --always-spawn # time=6537.188ms
# Subtest: --node-arg works on Windows
    not ok 1 - Command failed: node "C:\Users\name with spaces\dev\npx\test\util\npx-bin.js" --quiet --node-arg --no-deprecation echo-cli hewwo
      ---
      stack: |
        ChildProcess.child.on.code (child.js:1:11367)
      at:
        line: 1
        column: 11367
        file: child.js
        function: ChildProcess.child.on.code
      isOperational: true
      stderr: "internal/modules/cjs/loader.js:573\r\n    throw err;\r\n    ^\r\n\r\nError: Cannot find module 'C:\\Users\"name'\r\n    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:571:15)\r\n    at Function.Module._load (internal/modules/cjs/loader.js:497:25)\r\n    at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)\r\n    at startup (internal/bootstrap/node.js:228:19)\r\n    at bootstrapNodeJSCore (internal/bootstrap/node.js:575:3)\r\n"
      exitCode: 1
      test: '--node-arg works on Windows'
      source: |
        (function (exports, require, module, __filename, __dirname) { 'use strict'
      ...
    
    1..1
    # failed 1 test
not ok 14 - --node-arg works on Windows # time=956.295ms

I tried setting asPath to true when escaping existing using escapeArgs (since existing is a path) when I was investigating earlier. But apparently just enclosing the name with spaces part isn't enough in this context, which is why I set asPath to false in my example ¯\_(ツ)_/¯

@zkat zkat force-pushed the zkat/windows-npm-escape branch from dee7f22 to 1d1ee54 May 4, 2018

@zkat

This comment has been minimized.

Copy link
Owner Author

commented May 4, 2018

@noahleigh I think that was a bug in the test itself! I think Node itself gets kinda weird about that style of escaping and it's really annoying?

@n-leigh

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

@zkat Here's where I'm at with testing.

As this branch currently stands, I get the same results running npm test whether existing is escaped asPath, not asPath, or not escaped at all, with a Cannot find module 'C:\Users"name' error for seven tests, even ones that don't use the nodeArg logic branch. Something in the test logic is swallowing the second backslash as an escape character...

Running the commands that are being tested manually in the terminal accurately reflects the current state of existing's escape status:

Code

existing = child.escapeArg(existing, true)

Result

PS C:\Users\name with spaces\dev\npx> node .\test\util\npx-bin.js --always-spawn echo-cli success
npx: installed 47 in 3.862s
internal/modules/cjs/loader.js:573
    throw err;
    ^

Error: Cannot find module 'C:\Users"name'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:571:15)
    at Function.Module._load (internal/modules/cjs/loader.js:497:25)
    at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)
    at startup (internal/bootstrap/node.js:228:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:575:3)

Code

existing = child.escapeArg(existing, false)

Result

PS C:\Users\name with spaces\dev\npx> node .\test\util\npx-bin.js --always-spawn echo-cli success
npx: installed 47 in 3.965s
success

Code

// existing = child.escapeArg(existing, false)

Result

PS C:\Users\name with spaces\dev\npx> node .\test\util\npx-bin.js --always-spawn echo-cli success
npx: installed 47 in 4.19s
internal/modules/cjs/loader.js:573
    throw err;
    ^

Error: Cannot find module 'C:\Users\name'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:571:15)
    at Function.Module._load (internal/modules/cjs/loader.js:497:25)
    at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)
    at startup (internal/bootstrap/node.js:228:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:575:3)

Anything else you'd like me to test?

@zkat

This comment has been minimized.

Copy link
Owner Author

commented May 4, 2018

I think I'm good. Sorry to make you thrash like that. I'll have to spend some nice 1:1 time with Windows to make sure this stuff is actually covered. 💚 I'll update y'all when I'm more certain it actually works.

@n-leigh

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

Okay! Let me know if you need any more testing done 👍

@leofidus

This comment has been minimized.

Copy link

commented Oct 24, 2018

Is there any progress on this? Anything we can do to help?

@jtsom

This comment has been minimized.

Copy link

commented Dec 3, 2018

Will this fix ever make it in? npx is useless on Windows as it is...

@joejordan

This comment has been minimized.

Copy link

commented Mar 20, 2019

Just ran into this bug, I have a space in my Windows username and npx DID NOT like it. What's the holdup on a merge?

@illeatmyhat

This comment has been minimized.

Copy link

commented Apr 3, 2019

Combining this PR with #146 (comment), I was able to get npx create-react-app to work.
I opened C:\Program Files\nodejs\node_modules\npm\node_modules\libnpx\index.js and replaced its contents with the updates in this PR. Just copy pasting the whole file worked for me.
You can run cmd /K "cd C:\Users && dir /x" to see the shorthand version of your username, for example: C:\Users\JOHNDO~1\
And you should probably also be running at least the latest LTS of Node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.