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

BUG: use errors=replace for stdout #536

Merged
merged 2 commits into from
Nov 25, 2023

Conversation

dnicolodi
Copy link
Member

No description provided.

@dnicolodi dnicolodi force-pushed the filename-encoding branch 8 times, most recently from 4c18fd6 to cd9b174 Compare November 16, 2023 20:47
@dnicolodi dnicolodi added bug Something isn't working enhancement New feature or request labels Nov 16, 2023
@dnicolodi
Copy link
Member Author

Unfortunately the added test catches the issue reported in #535 only when run without pytest stdout capture (namely with the -s command line flag).

@dnicolodi dnicolodi changed the title TST: add test using filename with non latin-1 characters BUG: use errors=replace for stdout Nov 16, 2023
@ankith26
Copy link

I can confirm that this PR fixes my issue. Instead of an error and build fail now, I get question mark symbols (?) in stdout in place of the korean characters, and the build finishes correctly.

@dnicolodi
Copy link
Member Author

I can confirm that this PR fixes my issue. Instead of an error and build fail now, I get question mark symbols (?) in stdout in place of the korean characters, and the build finishes correctly.

Thanks for testing. We can choose other alternative representations for characters that cannot be represented in the current encoding. I went for the ? because it is the simplest. Other options that make sense are 'backslashreplace' or 'namereplace'. https://docs.python.org/3/library/codecs.html#error-handlers I don't have an opinion about which one is the best choice.

@ankith26
Copy link

IMHO the 'replace' error handling is fine

@rgommers
Copy link
Contributor

I went for the ? because it is the simplest

+1 from me, seems like a good choice.

@rgommers rgommers added this to the v0.16.0 milestone Nov 22, 2023
Copy link
Contributor

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @dnicolodi. This looks fine to merge as is, and quite nice that we can drop a dependency. Just one question about the one difference with the corresponding code in Meson for color handling.

mesonpy/_util.py Outdated
if not kernel.GetConsoleMode(stdout, byref(mode)):
return False

return bool(kernel.SetConsoleMode(stdout, mode.value | ENABLE_VIRTUAL_TERMINAL_PROCESSING))
Copy link
Contributor

Choose a reason for hiding this comment

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

The one difference with mesonbuild/mlog.py is that that adds or os.environ.get('ANSICON') (see https://github.com/adoxa/ansicon/blob/37f92009f6ca4938a34a1d7cecff41d331c7423c/readme.txt#L119 for the docs of that env var). It's not clear to me that that case is covered by the checks before calling setup_windows_console here - should it be added?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should be handled indeed. I had the impression that ansicon was some legacy thing, but I checked and the ANSICON environment variable is used by other terminal emulators on Windows. I'll add checking for the environment variable.

We print log messages and error messages that may contain file names
containing characters that cannot be represented in the stdout
encoding. Use replacement markers for those instead than raising
UnicodeEncodeError.

Fixes mesonbuild#535.
@dnicolodi
Copy link
Member Author

I'm moving dropping the colorama dependency to another PR and merging this one.

@dnicolodi dnicolodi merged commit d2a4789 into mesonbuild:main Nov 25, 2023
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants