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
Add support for specifying response file encoding #15406
Conversation
… the response file name, and autodetect the response file encoding using the suffix of the response file name if one is specified there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.. with a couple of commend.
I still think it would be good to see who llvm does this without resorting to such measures (I assume clang accepts all these different encodings without issue?)
This new test seems to be failing on the emscripten-releases roller: https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8831614067975027953/+/steps/Emscripten_testsuite__upstream__other_/0/stdout specifically:
|
I would guess because of "This test assumes that the default system encoding is CP-1252."? I think this PR was submitted via auto-submit by mistake.. I suggest we revert it for now and re-land with comments addressed? (Unless @juj is around right now and wants to fix forward?) |
This reverts commit d65442e. This commit broke the windows emscripten-releases builder, and I think it was auto-commited by mistake anyway.
Yes, it should, although I now understand the term "default system encoding" I used there is wrong. It is the "default Python encoding on Windows" and not the system's default encoding. Python documentation for this can be found here: https://docs.python.org/3/glossary.html#term-locale-encoding where it states
However this is not the default Windows encoding - that is at least for me cp437, as detected by
The above wording suggests that Python does not make a system-specific choice, however:
In Python docs for
so I presume that this Python UTF-8 Mode must be in effect on the autoroller. There's this comment also in the docs about the Python UTF-8 Mode:
|
Interestingly we set this in github CI also: emscripten/.circleci/config.yml Lines 409 to 411 in 0d24418
Which I guess means we are not running this tests on windows CI. The list of tests we currently run on windows during github CI is: emscripten/.circleci/config.yml Line 431 in 0d24418
emscripten-releases also looks like it sets I think it should be possible to "unset PYTHONUTF8" (using |
I rewrote the test to work in either case when |
Add support for specifying response file encoding using the suffix of the response file name, and autodetect the response file encoding using the suffix of the response file name if one is specified there.
Fixes #15292.