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

chore: nicer error message for windows users #739

Closed
wants to merge 1 commit into from

Conversation

Lewiscowles1986
Copy link
Contributor

@Lewiscowles1986 Lewiscowles1986 commented Oct 16, 2024

From https://phpc.social/deck/@[email protected]/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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Lewiscowles1986
Copy link
Contributor Author

see python/cpython#125568 and pypi/support#4929 as well...

@Lewiscowles1986
Copy link
Contributor Author

Lewiscowles1986 commented Oct 16, 2024

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
Copy link
Owner

amoffat commented Oct 16, 2024

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
Copy link
Contributor Author

Lewiscowles1986 commented Oct 17, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

Apparently no linting errors anyway.

Closing this in favor of #740

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants