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

Open the build file with universal-newlines mode #1053

Closed
wants to merge 1 commit into from

Conversation

guymguym
Copy link
Contributor

@guymguym guymguym commented Nov 13, 2016

To make sure platform specific newlines ('\r\n' or '\r') are converted to '\n' which otherwise will fail eval().

Reference to python docs (Note: this mode is deprecated in python3 but needed in python2)
https://docs.python.org/2.7/glossary.html#term-universal-newlines
https://docs.python.org/2.7/library/functions.html#open

This should handle multiple issues reported on syntax error reading binding.gyp (partial list):
#979
#199
nospaceships/node-net-ping#24
nospaceships/node-net-ping#21
mathiask88/node-snap7#11
node-hid/node-hid#28
xdenser/node-firebird-libfbclient#24

To make sure platform specific newlines ('\r\n' or '\r') are converted to '\n' which otherwise will fail eval().

This should handle multiple issues reported on syntax error reading binding.gyp (partial list):
nodejs#979
nodejs#199
nospaceships/node-net-ping#24
nospaceships/node-net-ping#21
mathiask88/node-snap7#11
node-hid/node-hid#28
xdenser/node-firebird-libfbclient#24
@guymguym
Copy link
Contributor Author

Hi @bnoordhuis
See this regarding the discussion in #979.
The solution is straightforward with python's universal-newline file open mode.
Thanks!

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

@bnoordhuis
Copy link
Member

CI looks happy. Your author name is "Guy", is that how you want to show up in the release notes?

@guymguym
Copy link
Contributor Author

Oh didn't notice, fixed my profile to my full name. Thanks

bnoordhuis pushed a commit that referenced this pull request Nov 16, 2016
To make sure platform specific newlines ('\r\n' or '\r') are converted
to '\n' which otherwise will fail eval().

This should handle multiple issues reported on syntax error reading
binding.gyp (partial list):

#979
#199
nospaceships/node-net-ping#24
nospaceships/node-net-ping#21
mathiask88/node-snap7#11
node-hid/node-hid#28
xdenser/node-firebird-libfbclient#24

PR-URL: #1053
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis
Copy link
Member

Thanks, landed in 05c4494.

@bnoordhuis bnoordhuis closed this Nov 16, 2016
refack pushed a commit to refack/node-gyp that referenced this pull request Apr 23, 2017
To make sure platform specific newlines ('\r\n' or '\r') are converted
to '\n' which otherwise will fail eval().

This should handle multiple issues reported on syntax error reading
binding.gyp (partial list):

nodejs#979
nodejs#199
nospaceships/node-net-ping#24
nospaceships/node-net-ping#21
mathiask88/node-snap7#11
node-hid/node-hid#28
xdenser/node-firebird-libfbclient#24

PR-URL: nodejs#1053
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
refack pushed a commit to refack/node-gyp that referenced this pull request Aug 19, 2017
To make sure platform specific newlines ('\r\n' or '\r') are converted
to '\n' which otherwise will fail eval().

This should handle multiple issues reported on syntax error reading
binding.gyp (partial list):

nodejs#979
nodejs#199
nospaceships/node-net-ping#24
nospaceships/node-net-ping#21
mathiask88/node-snap7#11
node-hid/node-hid#28
xdenser/node-firebird-libfbclient#24

PR-URL: nodejs#1053
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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