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

Add tests for spaces in folder names #28819

Closed
wants to merge 1 commit into from

Conversation

@PaulBags
Copy link
Contributor

commented Jul 23, 2019

This file of npx uses __dirname. On windows paths are being split on spaces (as of current node.js LTS release), leading to issues like this (which still isn't resolved, despite dev protest).

See below for a log of this issue while attempting to run the react.js tutorial command npx create-react-app my-app (this error occurs both inside and outside the provided node.js cmd environment, with or without adminministrative permissions).

I therefore propose adding a test for spaces in directory names.

0 info it worked if it ends with ok
1 verbose cli [ 'C:\\Program Files\\nodejs\\node.exe',
1 verbose cli   'C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js',
1 verbose cli   'install',
1 verbose cli   'create-react-app@latest',
1 verbose cli   '--global',
1 verbose cli   '--prefix',
1 verbose cli   'C:\\Users\\User',
1 verbose cli   'Name\\AppData\\Roaming\\npm-cache\\_npx\\3672',
1 verbose cli   '--loglevel',
1 verbose cli   'error',
1 verbose cli   '--json' ]
2 info using npm@6.4.1
3 info using node@v10.15.3
4 verbose npm-session dfbc7ea6f6a9f350
5 silly install loadCurrentTree
6 silly install readGlobalPackageData
7 silly fetchPackageMetaData error for file:Name\AppData\Roaming\npm-cache\_npx\3672 Could not install from "Name\AppData\Roaming\npm-cache\_npx\3672" as it does not contain a package.json file.
8 http fetch GET 304 https://registry.npmjs.org/create-react-app 391ms (from cache)
9 silly pacote tag manifest for create-react-app@latest fetched in 403ms
10 timing stage:rollbackFailedOptional Completed in 1ms
11 timing stage:runTopLevelLifecycles Completed in 529ms
12 verbose stack Error: ENOENT: no such file or directory, open 'c:\react_test\Name\AppData\Roaming\npm-cache\_npx\3672\package.json'
13 verbose cwd c:\react_test
14 verbose Windows_NT 10.0.15063
15 verbose argv "C:\\Program Files\\nodejs\\node.exe" "C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "install" "create-react-app@latest" "--global" "--prefix" "C:\\Users\\User" "Name\\AppData\\Roaming\\npm-cache\\_npx\\3672" "--loglevel" "error" "--json"
16 verbose node v10.15.3
17 verbose npm  v6.4.1
18 error code ENOLOCAL
19 error Could not install from "Name\AppData\Roaming\npm-cache\_npx\3672" as it does not contain a package.json file.
20 verbose exit [ 1, true ]
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Add tests for spaces in folder names
[This file of npx](https://github.com/npm/npx/blob/latest/bin/index.js) uses __dirname. On windows paths are being split on spaces (as of current node.js LTS release), leading to issues like [this](zkat/npx#144) (which still isn't resolved, despite dev protest).

See below for a log of this issue while attempting to run the react.js [tutorial](https://reactjs.org/docs/create-a-new-react-app.html) command `npx create-react-app my-app` (this error occurs both inside and outside the provided node.js cmd environment, with or without adminministrative permissions).

I therefore propose adding a test for spaces in directory names.

```
0 info it worked if it ends with ok
1 verbose cli [ 'C:\\Program Files\\nodejs\\node.exe',
1 verbose cli   'C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js',
1 verbose cli   'install',
1 verbose cli   'create-react-app@latest',
1 verbose cli   '--global',
1 verbose cli   '--prefix',
1 verbose cli   'C:\\Users\\User',
1 verbose cli   'Name\\AppData\\Roaming\\npm-cache\\_npx\\3672',
1 verbose cli   '--loglevel',
1 verbose cli   'error',
1 verbose cli   '--json' ]
2 info using npm@6.4.1
3 info using node@v10.15.3
4 verbose npm-session dfbc7ea6f6a9f350
5 silly install loadCurrentTree
6 silly install readGlobalPackageData
7 silly fetchPackageMetaData error for file:Name\AppData\Roaming\npm-cache\_npx\3672 Could not install from "Name\AppData\Roaming\npm-cache\_npx\3672" as it does not contain a package.json file.
8 http fetch GET 304 https://registry.npmjs.org/create-react-app 391ms (from cache)
9 silly pacote tag manifest for create-react-app@latest fetched in 403ms
10 timing stage:rollbackFailedOptional Completed in 1ms
11 timing stage:runTopLevelLifecycles Completed in 529ms
12 verbose stack Error: ENOENT: no such file or directory, open 'c:\react_test\Name\AppData\Roaming\npm-cache\_npx\3672\package.json'
13 verbose cwd c:\react_test
14 verbose Windows_NT 10.0.15063
15 verbose argv "C:\\Program Files\\nodejs\\node.exe" "C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "install" "create-react-app@latest" "--global" "--prefix" "C:\\Users\\User" "Name\\AppData\\Roaming\\npm-cache\\_npx\\3672" "--loglevel" "error" "--json"
16 verbose node v10.15.3
17 verbose npm  v6.4.1
18 error code ENOLOCAL
19 error Could not install from "Name\AppData\Roaming\npm-cache\_npx\3672" as it does not contain a package.json file.
20 verbose exit [ 1, true ]
```
@Trott

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

On windows paths are being split on spaces (as of current node.js LTS release)

I ran this on all supported Node.js releases and got the same expected result.

path.win32.dirname('c:\\foo bar\\baz'); // returns 'c:\\foo bar'
@Trott

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

/ping @PaulBags I don't understand what you're trying to say here. Can you please clarify? Are you suggesting there's a bug in some currently-supported versions of Node.js with regard to path.dirname() on Windows? If so, can you be explicit about what it is? If not, can you explain how adding these tests to Node.js will help fix bugs in other applications?

@PaulBags

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

The version of npx that ships with node LTS on windows is splitting paths on spaces, paths that it decides itself so I can't fix them by wrapping in quotes, and appears to be using __dirname from node to do so.

The author of npx claimed this was fixed, with zero proof - the project has since been marked read only and appears to have been taken over by node.

If the problem isn't here then I don't where to look, but yes it is currently supported node.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

appears to have been taken over by node.

Nope. It is handled by npm, not node.

If the problem isn't here then I don't where to look, but yes it is currently supported node.

The repo to use is https://github.com/npm/npx. (They have the issue tracker turned off probably because they use https://npm.community/ as their issue tracker.)

All that said...

Your output above says you're running node 10.15.3 and npm 6.4.1. Can you upgrade to node 10.16.0 and then update npm to 6.10.2. (npm install -g npm should do the trick.) With those updates, does the problem persist?

@Trott
Trott approved these changes Aug 2, 2019
Copy link
Member

left a comment

Although the explanation for why these tests are being added is (I believe) not quite correct, there's no reason not to test paths with spaces, so this LGTM. Whoever lands this (probably me) will want to update the commit message.

@Trott Trott added the author ready label Aug 2, 2019

@nodejs-github-bot

This comment has been minimized.

Trott added a commit to Trott/io.js that referenced this pull request Aug 2, 2019
test: add tests for spaces in folder names
Add tests for spaces in folder names for path.win32.dirname().

PR-URL: nodejs#28819
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

Landed in 1ddd46f.

Thanks for the contribution! 🎉

@Trott Trott closed this Aug 2, 2019

targos added a commit that referenced this pull request Aug 2, 2019
test: add tests for spaces in folder names
Add tests for spaces in folder names for path.win32.dirname().

PR-URL: #28819
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR referenced this pull request Aug 6, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
test: add tests for spaces in folder names
Add tests for spaces in folder names for path.win32.dirname().

PR-URL: nodejs#28819
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
test: add tests for spaces in folder names
Add tests for spaces in folder names for path.win32.dirname().

PR-URL: nodejs#28819
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.