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

Fix PEP 597 UTF-8 Warnings #8117

Merged
merged 1 commit into from Nov 25, 2023
Merged

Fix PEP 597 UTF-8 Warnings #8117

merged 1 commit into from Nov 25, 2023

Conversation

danyeaw
Copy link
Contributor

@danyeaw danyeaw commented Nov 21, 2023

This PR fixes PEP 597 EncodingWarnings when PYTHONWARNDEFAULTENCODING is set to true.

It also helps simplify the exec_command_stdout compat function by making the default encoding UTF-8 which will be the default in the future with PEP 686.

@rokm
Copy link
Member

rokm commented Nov 21, 2023

Should we also try setting PYTHONWARNDEFAULTENCODING=1 on our CI, and ensure errors are raised on those warnings? This way we won't actually re-introduces the problems with non-specified encoding.

Also, does utf8 actually make sense in all the scenarios?

  • the osxutils are probably OK with it
  • for anything called on Windows, I would expect that local encoding is actually more suitable? But then again, we had to add errors=ignore in 1f8953a to the gi hook due to encoding issues. So perhaps a mismatch there does not really matter.
  • the upx check probably does not output non-ASCII characters, so utf8 should be fine
  • I can see only two uses of exec_command_stdout in the PyInstaller itself (which should probably be replaced with subprocess.run anyway, and exec_command_stdout should be phased out), and none in the contributed hooks; so I think this should not be a problem, either.

@danyeaw
Copy link
Contributor Author

danyeaw commented Nov 22, 2023

Hi @rokm, thanks so much for the feedback. I made the following changes:

  • Enable the EncodingWarnings in CI
  • Remove the exec_command_stdout compat function

As you said, UTF-8 should be good on macOS and Linux because it is already the default Python locale on those platforms. On Windows, if we specify the encoding and use it consistently, we should hopefully help avoid errors like the one you mentioned above. This should help get ahead of the game when UTF-8 becomes the default on all platforms.

@rokm
Copy link
Member

rokm commented Nov 22, 2023

  • Enable the EncodingWarnings in CI

Ufff, that did open a whole can of warnings :)

I would prefer to turn them into errors, but it looks like that EncodingWarning type is unavailable in python < 3.10, so adding error::EncodingWarning: to pytest's filterlist won't work. (And it also looks like some of the warnings are beyond our control).

Still, we can track them in the tests' summary output, and that's good enough. I see majority are from the test suite, but there are some remaining problems in the "main" code. At least this one

https://github.com/rokm/pyinstaller/blob/dd40fb2cd75b86b5bbe23843bafde09740f451af/PyInstaller/compat.py#L94

should be taken care of immediately, because it poisons the expected stderr output of the test and causes it to fail.

Do you want to take care of the rest, or would you prefer I take over and mop them up later?

@rokm
Copy link
Member

rokm commented Nov 22, 2023

On Windows, if we specify the encoding and use it consistently, we should hopefully help avoid errors like the one you mentioned above. This should help get ahead of the game when UTF-8 becomes the default on all platforms.

The problem is that UTF-8 will become default on all platforms in python, but the external programs that we need to run via subprocess.run will not necessarily make that switch. But I guess we'll be taking care of that on per-case basis, though, same as we have up until now.

@danyeaw
Copy link
Contributor Author

danyeaw commented Nov 22, 2023

Hi @rokm, I'll try to cleanup the remaining warnings today. Thanks!

PyInstaller/compat.py Outdated Show resolved Hide resolved
PEP 685 makes UTF-8 the default mode for Python. Explicitly add
encoding parameters to open calls.

- Replace subprocess check_output with the more modern run
command and set the default encoding of UTF-8
- compat: remove exec_command_stdout function since no longer
needed to maintain compatibility across platforms
@rokm
Copy link
Member

rokm commented Nov 25, 2023

@danyeaw Is the PR ready for final review and merge?

@danyeaw
Copy link
Contributor Author

danyeaw commented Nov 25, 2023

Hi @rokm, yup, all set!

@rokm
Copy link
Member

rokm commented Nov 25, 2023

Hi @rokm, yup, all set!

Cool, thanks!

@rokm rokm merged commit 75ca04f into pyinstaller:develop Nov 25, 2023
18 checks passed
@danyeaw danyeaw deleted the utf-8 branch November 25, 2023 16:22
@danyeaw
Copy link
Contributor Author

danyeaw commented Nov 25, 2023

Thanks!!

@Avasam
Copy link
Contributor

Avasam commented Dec 17, 2023

There's still two references to exec_command_stdout in docstrings: in exec_command and in exec_command_all

@danyeaw
Copy link
Contributor Author

danyeaw commented Dec 17, 2023

@Avasam, thanks for noticing this, I can submit a PR to clean it up 👍

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants