Skip to content

Comments

Fix running console scripts of editable dependencies on Windows#3339

Merged
neersighted merged 5 commits intopython-poetry:masterfrom
kevincon:issue/3265
Jun 4, 2022
Merged

Fix running console scripts of editable dependencies on Windows#3339
neersighted merged 5 commits intopython-poetry:masterfrom
kevincon:issue/3265

Conversation

@kevincon
Copy link
Contributor

@kevincon kevincon commented Nov 7, 2020 •
edited
Loading

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.

@finswimmer finswimmer requested a review from a team November 8, 2020 06:23
This was referenced Nov 22, 2020
Closed
Closed
@kevincon
Copy link
Contributor Author

kevincon commented Dec 18, 2020

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

@arzamas500 arzamas500 mentioned this pull request Apr 26, 2021
Closed
@arzamas500
Copy link

arzamas500 commented Apr 26, 2021

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

@kevincon
Copy link
Contributor Author

kevincon commented Apr 27, 2021

@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

kevincon commented May 17, 2022

@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 dimbleby mentioned this pull request Jun 5, 2022
Closed
3 tasks
@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
Merged
@dimbleby dimbleby mentioned this pull request Jun 25, 2022
Closed
3 tasks
@github-actions
Copy link

github-actions bot commented Feb 29, 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 Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@neersighted neersighted neersighted approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

6 participants

@kevincon @arzamas500 @abn @Secrus @dimbleby @neersighted