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 running console scripts of editable dependencies on Windows #3339

Merged
merged 5 commits into from
Jun 4, 2022

Conversation

kevincon
Copy link
Contributor

@kevincon kevincon commented Nov 7, 2020

Pull Request Check List

Resolves: #3265
Resolves: #2481

  • Added tests for changed code.
  • Updated documentation for changed code. (I'm not aware of any documentation that needs updating for this change)

This PR changes poetry run to use shell=True when executing the user-provided command on Windows.

This ensures that the CMD wrapper script file that Poetry generates for console scripts of editable dependencies can be called via poetry run by only specifying the name of the console script (i.e. without the .cmd extension of the wrapper script file) because shell=True causes the command to be executed via cmd.exe on Windows which has support for calling .cmd files without specifying the .cmd extension.

I don't think that adding shell=True here only for Windows should cause any issues. I saw that the run() method on the Env class (which calls the _run() private method) already uses shell=True when executing any command on Windows:

poetry/poetry/utils/env.py

Lines 1067 to 1068 in 825cf66

if self._is_windows:
kwargs["shell"] = True

I also think I understand from #1236 that the motivation for having a separate execute() method from the run() method on the Env class was so that poetry run could use execute() to ensure that signals sent to poetry are forwarded correctly to what is executed by poetry run, but I don't think that adding shell=True should interfere with that goal on Windows.

@kevincon
Copy link
Contributor Author

@finswimmer can you help me find a reviewer for this PR?

@arzamas500
Copy link

Any progress on that? Seems like an easy-fix?

@kevincon
Copy link
Contributor Author

@arzamas500 I have resolved the merge conflicts, but we will need a maintainer to approve the check workflows since this is my first time contributing.

@kevincon
Copy link
Contributor Author

kevincon commented Apr 3, 2022

@finswimmer it's been a few years since I opened this PR and I believe the targeted issues are still present in the latest Poetry release(s). Is there anything I need to do before a maintainer would approve running the remaining PR checks (since I am a first-time contributor) and/or review this PR?

@abn
Copy link
Member

abn commented Apr 3, 2022

@kevincon I have approved the run. Will review it in the coming week unless someone else gets to it before me.

@kevincon
Copy link
Contributor Author

kevincon commented Apr 3, 2022

Thanks @abn! I saw a test that was recently introduced in #5243 failed on Windows due to the changes in this PR, so I just pushed a commit to fix it. Can you approve running the checks once more?

@abn
Copy link
Member

abn commented Apr 3, 2022

@kevincon how does this change impact non command prompt shells on windows?

@kevincon
Copy link
Contributor Author

kevincon commented Apr 3, 2022

how does this change impact non command prompt shells on windows?

@abn I'm not familiar with any other shells used on Windows besides Command Prompt (but understand they are technically supported by Python via the COMSPEC environment variable). A quick skim of https://en.wikipedia.org/wiki/Comparison_of_command_shells suggests there might only be https://jpsoft.com/products/tcc-le.html and https://www.4dos.info/, both created by JP Software. The latter appears to have been abandoned (last release in 2009), and the former claims to be a superset of Command Prompt and aims to be compatible with it.

@Secrus
Copy link
Member

Secrus commented May 16, 2022

@kevincon how about Powershell? is it fully compatible with CMD?

@kevincon
Copy link
Contributor Author

@Secrus I don't think Powershell is compatible with COMSPEC, see https://social.technet.microsoft.com/Forums/lync/en-US/584fff37-af8b-4560-bcb0-324c84275a09/using-powershell-as-comspec?forum=ITCG and https://superuser.com/a/289760.

I don't think that Powershell not being compatible with COMSPEC should prevent us from making this change, just that Powershell in general can't be used with COMSPEC.

@neersighted neersighted merged commit 53e50d9 into python-poetry:master Jun 4, 2022
@dimbleby
Copy link
Contributor

dimbleby commented Jun 5, 2022

Per #5773 (comment) I suspect this has broken commands with more than one word.

@abn abn mentioned this pull request Jun 6, 2022
Copy link

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 Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants