Teach poetry run to find .cmd scripts on Windows

[andyleejordan]

The Poetry script generation creates .cmd scripts, not .exe files, but poetry run did not know to look for .cmd scripts resulting in a FileNotFoundError when running custom scripts on Windows.

This doesn’t reproduce with poetry shell because the executable search happens in the shell (like PowerShell).

Because this function now needs to search across a list of suffixes on Windows (from PATHEXT), it was refactored and the negative logic flipped to read more clearly:

1. Get the path to the bin in Poetry’s environment
2. On Windows, test if path.(exe, cmd, etc. from PATHEXT) exists, and return it (handling the existing edge case with base paths).
3. If the path (without an extension) exists, return it
4. Return the original ‘bin’ if none of the above succeeded

Pull Request Check List

• Added tests for changed code.
• Updated documentation for changed code. (Bug fix, so no documentation to change.)

[sinoroc]

Related: #2481

Also look at this change: master...huwper:master

From this comment: #2481 (comment)

[kevincon]

I think this is also a duplicate of #3265 which I have this PR up to fix: #3339

[sinoroc]

I see.. Maybe you all 3 could collaborate on figuring out what is the best solution, discuss the pros and cons of each approach.

[andyleejordan]

@sinoroc @kevincon My recommendation would be to take this PR. Setting shell=True as done in #3339 technically works, but kind of simply by happenstance. The logic in env.py for finding binaries on Windows was incorrect, and I believe this PR fixes it (I just updated it to use PATHEXT as it should, I think I'd even alluded to this, but couldn't remember what the variable was called, thanks @huwper, and I added a test, thanks for the help with that @kevincon, and I'm happy to reset-author and give you credit on that commit).

With the logic to find binaries in place as it should be on Windows (see start's docs) poetry run now works like a "proper" Windows program. I advise against setting shell=True because it works only by "faking it" where the logic in env.py is still wrong (finds the wrong file to launch), but by invoking an extra process (the shell) it happens to work. We should also avoid opening a shell in between poetry run and the user's executable, as this can cause problems (and indeed, I'd recommend removing shell=True from elsewhere in the code).

P.S. Here are some of Python's own security considerations for using shell=True. The behavior of the invoked program also changes because of that additional indirection:

If shell is True, the specified command will be executed through the shell. This can be useful if you are using Python primarily for the enhanced control flow it offers over most system shells and still want convenient access to other shell features such as shell pipes, filename wildcards, environment variable expansion, and expansion of ~ to a user’s home directory.

[kevincon]

Setting shell=True as done in #3339 technically works, but kind of simply by happenstance.

I advise against setting shell=True because it works only by "faking it" where the logic in env.py is still wrong (finds the wrong file to launch), but by invoking an extra process (the shell) it happens to work.

I think it would be good if a maintainer could confirm this, but my interpretation of the goal of poetry run is that you have some command you would like to run from the command line but need it to be run within the virtual environment managed by poetry. From this angle, I think it is appropriate to call the command the user provides with shell=True to match that intention of wanting to run some command from a shell so that you automatically apply the same features the shell would normally apply if the user ran the command from the shell without using poetry run, including this behavior around .cmd scripts. My understanding is the only reason calling a console script works from a shell outside of using poetry run is because the Windows shell (cmd.exe) provides that behavior of finding the script with the .cmd file extension via the PATHEXT environment variable, so I don't think we are faking anything but rather accurately matching the user's expectations/intention and doing so without unnecessarily reimplementing that same behavior in Python.

I added a test, thanks for the help with that @kevincon, and I'm happy to reset-author and give you credit on that commit

Yes I think that would be appropriate since you copied the code from my PR, and I saw you updated this PR already to do that, so thank you.

[andyleejordan]

Keep in mind that poetry run is probably 100% of the time already being executed in a shell, so shell=True "double-shells." You're going to have your outer shell perform variable expansion, quoting, the like, and then pass all that to poetry run, which would then invoke a second shell that would perform all of that again, and in my experience this leads to a world of issues.

[huwper]

Thanks @andschwa for adding me in. Just tested it and it solves the issue I was having, however I see 2 issues still present:

1. if the suffix is supplied, it will be ignored. I think pathlib.Path.with_suffix() removes any existing suffix.
2. if the .bat/.cmd/etc. file was not installed via poetry (so is on the PATH and not in Env._bin_dir) then poetry still won't find it.

Would be good to solve 1 here. Not sure what is best to do about 2

edit: maybe https://docs.python.org/dev/library/shutil.html#shutil.which is a reasonable answer in both cases

[sinoroc]

Good to see you all collaborating. I can't help with the PR itself, since I do not know the code and to be honest I do not understand the reason why there is a need for poetry to be intrusive here (my naive assumption was that just setting some environment variable would have been enough to run a command in the virtual environment, but obviously that is not as simple).

Let's ping @abn and/or @finswimmer, to hopefully help you move forward.

[andyleejordan]

I am back from holiday and would love to get this fix in. @huwper what are you suggesting, and have you tried anything in the interim?

[huwper]

Hey @andschwa, your comment inspired me to try something. see this:

andyleejordan#1

I believe resolves the issues 1 and 2 I made above.

[andyleejordan]

Hi, I'm sorry, but I'm sadly no longer working on Python projects at work and won't be able to work on this further. I am glad you've found a good way to do this @huwper. I'll merge that PR so it shows up here, but it looks like it may need to be rebased and such. If that's the case, please feel free to take any/all changes you need and replace this PR.

[andyleejordan]

@huwper did you get a chance to replace this PR? I hope so. I'll leave the branch around but I'm closing the PR itself (just trying to do some cleanup work).

[github-actions]

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.