Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

run python scripts as "python -c text" #8565

Merged

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Oct 22, 2023

and not as "python -", and then passing the script as input

this sidesteps encoding issues, fixes #8550

comments:

  • this has knock-on effects in the unit tests: scripts that were previously executed via subprocess.run() are now executed via subprocess.check_output()
  • since run_python_script was the only caller making use of input_ in this code, I've removed the subprocess.run() branch altogether
  • we're not close to worrying about too-long arguments
    • len(GET_ENVIRONMENT_INFO) is 1717, the Windows limit should be 8191, it's much larger elsewhere
    • (if we were I suppose we could write the script to a temporary directory and run it from there)
  • this is the pattern already followed throughout env_manager.py

@dimbleby dimbleby force-pushed the python-scripts-not-as-input branch 2 times, most recently from 98ccbdc to f11f81d Compare October 22, 2023 12:23
@privet-kitty
Copy link

Thank you for addressing it. I guess that your change would resolve the reported error. I checked that a non-ascii working directory test with cp1250 locale passed after merging your change.

Unfortunately, even after your change, I realized that test_pip_install_successful and some other tests failed on another default encoding like cp932. subprocess.check_output called from run_pip raised an exception during decoding output of pip, which is like

Processing c:\users\hugov\あ\poetry\tests\fixtures\distributions\demo-0.1.0-py2.py3-none-any.whl
Installing collected packages: demo
Successfully installed demo-0.1.0

There the parent assumed the default cp932 while pip returned the output in utf-8. So it may be about the behavior of pip. I guess I or any other users will raise an issue if this would be an apparent problem. (At least I know that pip doesn't always use utf-8 for output. The reason it passed in CI was because bytes("ä", encoding="utf-8") is valid also in the cp1250 context but bytes("あ", encoding="utf-8") isn't in the cp932 one.)

@dimbleby
Copy link
Contributor Author

pip_install() mostly isn't very interesting to poetry any more: it is almost - though sadly not quite entirely - deprecated in favour of the "modern" installer.

Further fixes welcome I expect, meanwhile if we're happy that this one is good and does fix some things then hopefully we can take that win. Progress is progress.

@dimbleby
Copy link
Contributor Author

also none of the callers of pip_install() actually care about the output, so far as I can see: so probably a fix that converts that to use the call variant of Env.run() would be straightforward

and not as "python -", and then passing the script as input
@radoering radoering force-pushed the python-scripts-not-as-input branch from f11f81d to ace4865 Compare October 28, 2023 14:47
@radoering radoering merged commit 7401aa3 into python-poetry:master Oct 28, 2023
19 checks passed
@dimbleby dimbleby deleted the python-scripts-not-as-input branch October 28, 2023 15:05
MrGreenTea pushed a commit to MrGreenTea/poetry that referenced this pull request Dec 18, 2023
Copy link

github-actions bot commented Mar 3, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

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

Successfully merging this pull request may close these issues.

UTF-8 path-chars Encoding Issue during poetry install
3 participants