Skip to content

issue #17: fix space in username on windows#263

Closed
arve0 wants to merge 1 commit intoshelljs:masterfrom
arve0:windows_user_directory_with_space
Closed

issue #17: fix space in username on windows#263
arve0 wants to merge 1 commit intoshelljs:masterfrom
arve0:windows_user_directory_with_space

Conversation

@arve0
Copy link
Copy Markdown

@arve0 arve0 commented Dec 21, 2015

Avoiding this error:

$ npm test

> shelljs@0.5.3 test c:\Users\Arve Seljebu\Documents\GitHub\shelljs
> node scripts/run-tests

module.js:340
    throw err;
    ^

Error: Cannot find module 'c:\Users\Arve'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:289:25)
    at Function.Module.runMain (module.js:457:10)
    at startup (node.js:138:18)
    at node.js:974:3
 shell.js: internal error
Error: Command failed: "c:\Program Files\nodejs\node.exe" c:\Users\Arve Seljebu\AppData\Local\Temp\shelljs_94059e280f6ce03ce67b
    at checkExecSyncError (child_process.js:464:13)
    at Object.execSync (child_process.js:504:13)
    at execSync (c:\Users\Arve Seljebu\Documents\GitHub\shelljs\src\exec.js:81:11)
    at _exec (c:\Users\Arve Seljebu\Documents\GitHub\shelljs\src\exec.js:214:12)
    at c:\Users\Arve Seljebu\Documents\GitHub\shelljs\src\common.js:182:23
    at Object.<anonymous> (c:\Users\Arve Seljebu\Documents\GitHub\shelljs\scripts\run-tests.js:19:5)
    at Module._compile (module.js:425:26)
    at Object.Module._extensions..js (module.js:432:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:313:12)
npm ERR! Test failed.  See above for more details.

@arve0
issue shelljs#17: fix space in username on windows
8692b9f 
Avoiding this error:
```sh
$ npm test

> shelljs@0.5.3 test c:\Users\Arve Seljebu\Documents\GitHub\shelljs
> node scripts/run-tests

module.js:340
    throw err;
    ^

Error: Cannot find module 'c:\Users\Arve'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:289:25)
    at Function.Module.runMain (module.js:457:10)
    at startup (node.js:138:18)
    at node.js:974:3
 shell.js: internal error
Error: Command failed: "c:\Program Files\nodejs\node.exe" c:\Users\Arve Seljebu\AppData\Local\Temp\shelljs_94059e280f6ce03ce67b
    at checkExecSyncError (child_process.js:464:13)
    at Object.execSync (child_process.js:504:13)
    at execSync (c:\Users\Arve Seljebu\Documents\GitHub\shelljs\src\exec.js:81:11)
    at _exec (c:\Users\Arve Seljebu\Documents\GitHub\shelljs\src\exec.js:214:12)
    at c:\Users\Arve Seljebu\Documents\GitHub\shelljs\src\common.js:182:23
    at Object.<anonymous> (c:\Users\Arve Seljebu\Documents\GitHub\shelljs\scripts\run-tests.js:19:5)
    at Module._compile (module.js:425:26)
    at Object.Module._extensions..js (module.js:432:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:313:12)
npm ERR! Test failed.  See above for more details.
```
@nfischer
Copy link
Copy Markdown
Member

nfischer commented Jan 13, 2016

@arve0 Do you have steps I could take to reproduce this (without changing my username)?

@arve0
Copy link
Copy Markdown
Author

arve0 commented Jan 13, 2016

Mock tempdir to give you c:\temp dir with spaces\.

Edit: added link to tempdir.

@nfischer nfischer self-assigned this Jan 13, 2016
@nfischer
Copy link
Copy Markdown
Member

nfischer commented Jan 14, 2016

Linking to #17 (so that the link is clickable and I don't forget later on)

@nfischer nfischer added the fix Bug/defect, or a fix for such a problem label Jan 14, 2016
@nfischer nfischer mentioned this pull request Jan 15, 2016
Closed
@ariporad ariporad added this to the v0.6.0 milestone Jan 24, 2016
@ariporad ariporad modified the milestones: v0.7.0, v0.6.0 Feb 1, 2016
@TimothyGu
Copy link
Copy Markdown
Contributor

TimothyGu commented Feb 14, 2016

Is there a reason why this was not merged?

@TimothyGu
Copy link
Copy Markdown
Contributor

TimothyGu commented Feb 14, 2016

Some more information. This PR itself is not enough to fix all the test failures, but it is a step in the right direction. The test file needs to be changed as well, and even then the test at coffeescript will fail.

After the switch to rechoir (#258), the last issue will fix itself.

@nfischer
Copy link
Copy Markdown
Member

nfischer commented Feb 15, 2016

I just forgot to review. I'll try to test it out tonight. Sorry about that!

@nfischer nfischer mentioned this pull request Feb 19, 2016
Merged
@nfischer
Copy link
Copy Markdown
Member

nfischer commented Feb 19, 2016

@arve0 @TimothyGu Finally got around to reviewing (sorry for the delay!). It looks mostly good (thanks for tracking down this bug!), but I have two issues:

  1. This should be rebased, to resolve conflicts
  2. This should also escape quote characters, otherwise we're vulnerable to some security attacks

I've done #365 as an example of what I mean. If you want to fix this to have the security provisions #365 has, then I have no issue with taking this PR. Or we can just merge mine, unless there are issues. Either way is fine.

@nfischer nfischer added the exec Issues specific to the shell.exec() API label Feb 19, 2016
@arve0
Copy link
Copy Markdown
Author

arve0 commented Feb 19, 2016

Just get the unit- and battle-tested ship off shore.

@arve0 arve0 closed this Feb 19, 2016
@nfischer
Copy link
Copy Markdown
Member

nfischer commented Feb 19, 2016

👍 will do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nfischer nfischer

Labels

exec Issues specific to the shell.exec() API fix Bug/defect, or a fix for such a problem

Projects

None yet

Milestone

v0.7.0

Development

Successfully merging this pull request may close these issues.

4 participants

@arve0 @nfischer @TimothyGu @ariporad