gyp: update xml string encoding conversion #1203
Conversation
|
Maybe cc/ @joaocgreis @refack ? Proposed change seems reasonable, but I don't know enough to review. |
|
@lc-soft I'll have a look |
|
P.S. I keep forgetting to say... |
|
@refack I found these: #366 #945 #297. |
Yeah, for a long time upstream I'm looking at your patch, we might land it here, and then try to push it upstream. Anyway since AFAIK this is your first contribution, I'll give you some info on our review and landing process. |
gyp/pylib/gyp/easy_xml.py
Outdated
| xml_string = unicode(xml_string, 'latin-1').encode(encoding) | ||
|
|
||
| default_encoding = locale.getdefaultlocale()[1] | ||
| if default_encoding != encoding.upper(): |
There was a problem hiding this comment.
why not .upper or .lower both sides?
Also are you sure this won't throw?
There was a problem hiding this comment.
Ok, this is my negligence, I mistakenly thought it is capitalized, by this result:
There was a problem hiding this comment.
I think it makes sense to "normalize" both sides, since AFAIK there are no two Language Tags that differ only by case.
|
I can not run gyptest.py on Windows 10. |
|
Which version of Visual Studio you got Installed? Cause if it's VS2017 Build Tools, Also you could run BTW: could you add a test case here (maybe add a copy of |
|
@refack OK, It has been running successfully. But there are some errors: |
lc-soft
changed the title
|
Try |
|
@refack There will be more problems after using this method. |
|
Yeah, the test harness is very rusty. |
|
@refack Good job! 👍 |
|
Did I click submit on a comment asking you to add a test? Anyway, please do. I can suggest copying Bonus points if you add a test also to https://github.com/refack/GYP/tree/node-gyp-1203 😉 |
|
OK, I will take a few days to add test. |
test/test-addon.js
Outdated
| } | ||
| runCmd(t, [nodeGyp, 'configure', '-C', addonPath, '--loglevel=verbose', | ||
| '-nodedir=' + testNodeDir]) | ||
| runCmd(t, [nodeGyp, 'build', '-C', addonPath, '--loglevel=verbose']) |
There was a problem hiding this comment.
Please add a clean up (try { fs.unlinkSync...)
There was a problem hiding this comment.
@refack I have tried to use the rebuild command before, but there will be errors:
...
gyp verb clean removing "build" directory
gyp ERR! clean error
gyp ERR! stack Error: EPERM: operation not permitted, unlink 'F:\repos\GitHub\node-gyp\test\node_modules\hello_world\build\Release\hello.node'
...
how to solve this problem? a temporary solution is to use configure and build commands.
There was a problem hiding this comment.
I guess the reason hello.node can not be deleted is that it is occupied at the time of execution to require('hello_world'). so, solution is to create a new project (E.g:
hello_world_2), and rebuild it.
There was a problem hiding this comment.
IMHO execFileSync is also completely fine.
OR you could create a hello_非英文字符 fixture.
There was a problem hiding this comment.
I mean something like execFileSync(node, ['-e', "var x = require('hello_world'); if (x.hello() !== 'hello') process.exit(2)"])
Which is more sanitary anyway.
There was a problem hiding this comment.
I have tried to use execFileSync before, but there have been some error messages, so I switched to the current solution.
OK, I will try to use execFileSync again.
|
Thank you! |
test/test-addon.js
Outdated
| t.plan(5) | ||
|
|
||
| configure(t).on('exit', function () { | ||
| var data, config, nodeDir, testNodeDir |
There was a problem hiding this comment.
@refack I'am directly merged this test into the test build simple addon.
There was a problem hiding this comment.
If it works I'm +1
Another solution is to use execFileSync
test/test-addon.js
Outdated
| } | ||
| } | ||
|
|
||
| rebuild(t, testNodeDir).on('exit', function () { |
test/test-addon.js
Outdated
| }) | ||
|
|
||
| test('build simple addon in path with non-ascii characters', function (t) { | ||
| t.plan(3) |
There was a problem hiding this comment.
@refack OK, this test has been able to run successfully.
F:\repos\GitHub\node-gyp>node ./test/test-addon.js
TAP version 13
# build simple addon
ok 1 should be equal
ok 2 should end in ok
ok 3 should be equal
# build simple addon in path with non-ascii characters
ok 4 should be equal
ok 5 should end in ok
ok 6 should be equal
1..6
# tests 6
# pass 6
# ok
| var path = require('path') | ||
| var fs = require('graceful-fs') | ||
| var child_process = require('child_process') |
There was a problem hiding this comment.
question to @nodejs/node-gyp
Can we use destructuring?
const { execFileSync, execFile } = require('child_process')Or do we test on pre ES6 versions?
There was a problem hiding this comment.
I believe the CI still tests 0.10 and 0.12.
|
CI test did not pass. |
|
Look like bad argument handling. |
| if (!checkCharmapValid()) { | ||
| return t.skip('python console app can\'t encode non-ascii character.') | ||
| } | ||
|
|
There was a problem hiding this comment.
@refack I added the character map check, if python app can not convert non-ASCII characters, then skip this test. console output like this:
TAP version 13
# build simple addon
ok 1 should be equal
ok 2 should end in ok
ok 3 should be equal
# build simple addon in path with non-ascii characters
node : Traceback (most recent call last):
At line:5 char:1
+ node ./test/test-addon
+ ~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : NotSpecified: (Traceback (most recent call last)::String) [], RemoteException
+ FullyQualifiedErrorId : NativeCommandError
File "test-charmap.py", line 20, in <module>
print main()
File "test-charmap.py", line 15, in main
print textmap[encoding]
File "C:\Python27\lib\encodings\cp1252.py", line 12, in encode
return codecs.charmap_encode(input,errors,encoding_table)
UnicodeEncodeError: 'charmap' codec can't encode character u'\u012b' in position 3: character maps to <undefined>
ok 4 python console app can't encode non-ascii character. # SKIP
1..4
# tests 4
# pass 4
# ok
|
@refack |
package.json
Outdated
| @@ -46,6 +46,6 @@ | |||
| "require-inject": "~1.3.0" | |||
| }, | |||
| "scripts": { | |||
| "test": "tape test/test-*" | |||
| "test": "tape test/test-*.js" | |||
There was a problem hiding this comment.
I'd rather you move test-charmap.py to test/fixtures
| var path = require('path') | ||
| var fs = require('graceful-fs') | ||
| var child_process = require('child_process') |
There was a problem hiding this comment.
@refack
Do I need rename child_process to childProcess ?
There was a problem hiding this comment.
|
If you could rebase on |
|
https://ci.nodejs.org/job/nodegyp-test-commit/187/nodes=win2012r2/console |
|
Ignore it's |
|
New CI: https://ci.nodejs.org/job/nodegyp-test-pull-request/30/ Green ✅ |
|
I need to modify this test to make it support |
No. We stopped supporting node < 4 a week ago. |
* test: build simple addon in path with non-ascii characters * test: add test-charmap.py PR-URL: nodejs#1203 Reviewed-By: Refael Ackermann <refack@gmail.com>
|
Landed in d6139b5 |
|
Kind of late to the party but I think there is a bug in the python script. It assumes that $ env LC_ALL=C python -c 'import locale; print locale.getdefaultlocale()'
(None, None)It makes the test fail (or rather: pass when it shouldn't) like this: $ env LC_ALL=C node test/test-addon.js
TAP version 13
# build simple addon
ok 1 should be equal
ok 2 should end in ok
ok 3 should be equal
# build simple addon in path with non-ascii characters
Traceback (most recent call last):
File "fixtures/test-charmap.py", line 19, in <module>
print main()
File "fixtures/test-charmap.py", line 8, in main
sys.setdefaultencoding(encoding)
TypeError: setdefaultencoding() argument 1 must be string, not None
ok 4 python console app can't encode non-ascii character. # SKIP
1..4
# tests 4
# pass 4
# ok |
|
@bnoordhuis |
|
@bnoordhuis is |
| xml_string = unicode(xml_string, 'latin-1').encode(encoding) | ||
|
|
||
| default_encoding = locale.getdefaultlocale()[1] | ||
| if default_encoding.upper() != encoding.upper(): |
There was a problem hiding this comment.
should test default_encoding and default_encoding.upper() != encoding.upper():
Bug: nodejs/node-gyp#1203 Change-Id: I30d71a2bb3d4b09e7bd9409c3c45c32bd182d736
Bug: nodejs/node-gyp#1203 Change-Id: I30d71a2bb3d4b09e7bd9409c3c45c32bd182d736
My project depends on the
node-sass, but it can not successfully install, so I tried using thenpm rebuild node-sass --forcecommand to build thenode-sasson Windows 10. But it could not be built successfully, error message is:"node_version.h": No such file or directoryI guess this is the problem of compiling configuration, and I found the
node-sass\build\binding.vcxprojfile from the output information, so I opened it with the editor and found that the configuration item has garbled text:My username can not be displayed correctly, there seems to be a encoding conversion bug when writing to the
binding.vcxprojfile. I took some time to debug it, Eventually I found the problem out here: easy_xml.py#L120Since
xml_stringcontains non-ascii characters,xml_string.encode()will fail and throw an exception, and then this string will be treated aslatin-1encoded string to convert, but my default locale character encoding is notlatin-1, therefore, thebinding.vcprojfile is written with the wrong content.After modification, the contents of the output file is correct.
node-sasscan be built successfully.