node-gyp icon indicating copy to clipboard operation
node-gyp copied to clipboard

Published 20 hours ago • verified publisher icon nodejs

configure.js: escape '<'

Open yeerkkiller1 opened this issue 6 years ago • 10 comments

Fixes: https://github.com/nodejs/node-gyp/issues/1501

On windows invoking spawn on a batch file results in additional argument processing. Special characters need to be escaped (twice).

Checklist
  • [x] npm install && npm test passes
  • [ ] tests are included
  • [ ] documentation is changed or added
  • [x] commit message follows commit guidelines
Description of change

Fixes issue where npm install fails if python is a batch file. This occurs because the batch file tries to interpret the '<' specially.

I tested this change on my windows machine (with and without python.bat in my path). I don't think this change will impact other platforms, as it only applies changes when the platform is win32, and when python is a batch file (which would have broken anyway).

yeerkkiller1 avatar Jul 19 '18 01:07 yeerkkiller1

CI https://ci.nodejs.org/job/nodegyp-test-pull-request/66/

Fishrock123 avatar Jul 20 '18 15:07 Fishrock123

I'm not confident enough to pull this in atm, does someone else with deep enough expertise want to confirm this is the right thing to do?

rvagg avatar Aug 09 '18 00:08 rvagg

I'm experiencing this bug as well, any update on reviews? Applying the patch manually seems to fix the issue for me.

samuelmaddock avatar Oct 06 '18 22:10 samuelmaddock

@yeerkkiller1 if you're still interested, would you mind rebasing and squashing your two commits?

rvagg avatar Jun 20 '19 02:06 rvagg

@rvagg, no problem, it's squashed and pushed now.

yeerkkiller1 avatar Jun 20 '19 03:06 yeerkkiller1

SGTM but I would love a +1 from @bzoz, @joaocgreis, @refack or one of our other core Windows experts, I've never had experience escaping in .bat with ^ (I had to Google it just now to see it was a thing!).

rvagg avatar Jun 20 '19 03:06 rvagg

@rvagg This is indeed the correct fix.

I just spent an hour debugging this issue myself. It would be great if this fix would be merged.

jlennox avatar Feb 24 '20 20:02 jlennox

@bzoz, @joaocgreis, please confirm

rvagg avatar Feb 25 '20 00:02 rvagg

I think this is not needed anymore. https://github.com/nodejs/node-gyp/pull/1582 made a change that extracts the Python .exe filename even from batch files and then uses it. We are no longer using the batch file to spawn Python executables.

I've also was not able to trigger the original issue. @yeerkkiller1, @jlennox could you verify that the issue is still present in master?

bzoz avatar Feb 25 '20 16:02 bzoz

#1582 went out with v5.0.0 so should be in npm already, maybe update npm: npm install npm -g

rvagg avatar Feb 26 '20 03:02 rvagg