amoffat / sh

Python process launching
https://sh.readthedocs.io/en/latest/
MIT License
6.98k stars 506 forks source link

chore: nicer error message for windows users #739

Closed Lewiscowles1986 closed 2 weeks ago

Lewiscowles1986 commented 2 weeks ago

From https://phpc.social/deck/@mistersql@mastodon.social/113314907209898815 image

Hi, curious library. Users are complaining on socials about using [python because of] it, and I'm not sure it's fair on either side.

I Can see that sysops folks might not want to use python in the same way I want to. (I would open a damn sub-process)

This new error lets folks know that only unix style python is supported, and points them at Docker, which I see you make use of in this repo.

The core use case is:

As a user, who has been able to successfully pip install sh I would like to receive meaningful errors, that signpost me So that I do not rage on socials, about python itself, or try to install non-packages such as fnctl

I do also intend upon raising this with python. Maybe this would be a good stub behaviour for fnctl so that users at least know "hey we deliberately chose not to include this" rather than "Module not found"

Lewiscowles1986 commented 2 weeks ago
fnctl = { version = "^1.0", markers = "sys_platform != 'linux' and sys_platform != 'darwin'" }

Might also work now I've published a package to pypi called fnctl it might be prettier than this.

Lewiscowles1986 commented 2 weeks ago

bugger, got the name wrong in that python package. No wonder it didn't complain 😂

Lewiscowles1986 commented 2 weeks ago

see https://github.com/python/cpython/issues/125568 and https://github.com/pypi/support/issues/4929 as well...

Lewiscowles1986 commented 2 weeks ago

btw tested the following monstrosity and confirmed still working with sh

poetry run pip install https://github.com/Lewiscowles1986/python-fcntl/releases/download/1.0.0/fcntl-1.0.0-py3-none-any.whl

then

import sh
from sh import bash

bash(['-c'], 'echo hi')

under windows it obviously shows the error discussed / proposed here and exits the code. under osx, it runs the code and returns

'hi\n'
amoffat commented 2 weeks ago

I think we just need to code this error up earlier in the file:

https://github.com/amoffat/sh/blob/develop/sh.py#L73-L76

If you agree, would you mind making this change?

Lewiscowles1986 commented 2 weeks ago

Hi Andrew, I don't necesarrily disagree, but if you look at the imports needed and import ordering, it might conflict with a few linting rules.

Thank you for considering this. I've also approached python and pypi, as it's sad that the library needs to solve for what feels like a runtime defficiency in standard libraries.

Also are you principally against asking folks to run in a docker container? The reason I mention that is that for a Windows user, it's not that windows is not supported per-se, as much as they have a hobbled core utils layer (WSL, Git Bash or Docker can fill that hole).

The thought was also that the language is that this is a windows problem, but it's not. It's a non Linux or OSx problem.

ecederstrand commented 2 weeks ago

I think it's fair to keep the wording in our error message that Windows isn't supported. If you run Docker on Windows, or any other virtualization platform for that matter, then you're effectively not running Windows anymore.

I like the solution of printing the error message before attempting to import packages not available on Windows. We can just add noqa specifiers to silence linters.

Lewiscowles1986 commented 2 weeks ago

Apparently no linting errors anyway.

Closing this in favor of #740