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

lib: print Python executable path using UTF-8 #2987

Conversation

huseyinacacak-janea
Copy link
Contributor

@huseyinacacak-janea huseyinacacak-janea commented Mar 5, 2024

Checklist
  • npm install && npm run lint && npm test passes
  • commit message follows commit guidelines
Description of change

The Python executable path may have non-ASCII characters, which can make the print function fail if the environment encoding is different. This PR fixes this issue by using stdout.buffer, which can be used with UTF-8 encoding for the output, regardless of the environment encoding.

Fixes: #2829

@cclauss
Copy link
Contributor

cclauss commented Mar 6, 2024

Please add a test or tests that fails with the old code and pass with the new code so we will be alerted if there are future regressions.

The Python executable path may have non-ASCII characters, which can make
the print function fail if the environment encoding is different. This fixes
this issue by using stdout.buffer, which can be used with UTF-8 encoding
for the output, regardless of the environment encoding.

Fixes: nodejs#2829
@huseyinacacak-janea
Copy link
Contributor Author

@cclauss thanks for reviewing. I've added a test.

@lukekarrys
Copy link
Member

Linting is failing in CI which is unfortunately preventing the tests from being run.

I did check this out and run it locally on macOS and they were failing. It could be because the test assumes that the result of PythonFinder.findPython(null) will be a single file which can be copied, but this is not the case at least on my machine.

@lukekarrys lukekarrys self-requested a review March 8, 2024 00:29
@cclauss
Copy link
Contributor

cclauss commented Mar 8, 2024

The linter errors are caused by a new version of ruff and are fixed in

@huseyinacacak-janea
Copy link
Contributor Author

@lukekarrys thank you for your feedback. I've updated the test to fix the failure on MacOS.
There are still linter errors because of ruff. I can re-run them once ruff fix lands in node-gyp.

@lukekarrys
Copy link
Member

@huseyinacacak-janea I think linting should now be fixed in main. Can you try updating your branch now?

@huseyinacacak-janea huseyinacacak-janea deleted the huseyin-python-encoding-utf-8 branch March 13, 2024 06:12
@huseyinacacak-janea
Copy link
Contributor Author

I rebased the branch to get the ruff fix. However, as I deleted the branch accidentally while pushing, I've opened a new PR with the same changes: #2995

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.

Python path cannot handle Unicode characters like ü
3 participants