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

test: fix variables quoated when path has spaces on Windows #1122

Closed
wants to merge 1 commit into from

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Mar 11, 2015

This fixes test failures when a path of the repository has spaces on Windows as it was pointed out in
c7ad320#commitcomment-10043315
and also fixes missed set in vcbuild.bat

r= @piscisaureus

@@ -2,10 +2,10 @@ var common = require('../common');
var assert = require('assert');
var exec = require('child_process').exec;

var cmd = process.execPath
+ ' '
var cmd = '"' + process.execPath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really hard to read... Can you make it something like

  cmd = '"' + process.execPath + '" ' +
             '"' + common.fixturesDir + '/test-regress-GH-4015.js"';

@piscisaureus
Copy link
Contributor

Thanks @shigeki, +1 with the following comments:

  • s/test/quoated/quoted/ in the commit message
  • "fix variables quoated when path has spaces on Windows" is a bit weird. The issue really doesn't have anything to do with variables.
  • See 2 inline comments.

External commands are enclosed in double quotes so as not to be failed
when spaces are included in execPATH especially on Windows.
@shigeki
Copy link
Contributor Author

shigeki commented Mar 12, 2015

@piscisaureus Thanks for your reviewing. I fixed styles and the commit message. The commit of vcbuild.bat was removed because it's already fixed, though I should have taken care of it more. PTAL.

@piscisaureus
Copy link
Contributor

@shigeki Great work, lgtm!
Running the CI just to be sure: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/285/

shigeki pushed a commit that referenced this pull request Mar 13, 2015
Paths used on the Windows command line need to be enclosed in double
quotes, or they'll be parsed incorrectly when there are spaces in the
path.

PR-URL: #1122
Reviewed-by: Bert Belder <bertbelder@gmail.com>
@piscisaureus
Copy link
Contributor

Thanks for your patience and good work, @shigeki.
Landed in 82f067e.

@shigeki
Copy link
Contributor Author

shigeki commented Mar 13, 2015

@piscisaureus Thanks for revising my commit message. It gets more clear and better English.

@rvagg rvagg mentioned this pull request Mar 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants