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

gh-125873: Improve error message when PYTHONHOME is invalid #126027

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

refeed
Copy link

@refeed refeed commented Oct 27, 2024

Copy link

cpython-cla-bot bot commented Oct 27, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Oct 27, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link

bedevere-app bot commented Oct 27, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@ZeroIntensity
Copy link
Member

This is user-facing, so it needs a NEWS entry. Something like "Improved error message when PYTHONHOME is invalid" should work.

@refeed
Copy link
Author

refeed commented Oct 27, 2024

Hi @ZeroIntensity , thanks for that! Just added it through the heroku blurb, please let me know if there's something I need to do, thanks

@refeed refeed force-pushed the gh-125873-pythonhome-wrong-better-errmsg branch from 6c9bdc4 to 6e83963 Compare October 27, 2024 22:37
@refeed refeed changed the title gh-125873: Add hint when failing to import encodings gh-125873: Improve error message when PYTHONHOME is invalid Oct 27, 2024
@refeed refeed force-pushed the gh-125873-pythonhome-wrong-better-errmsg branch from 6e83963 to c4156f6 Compare October 27, 2024 22:38
ZeroIntensity
ZeroIntensity previously approved these changes Oct 27, 2024
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! It's good to see new contributors :)

@skirpichev
Copy link
Member

I doubt that we need a special treatment just for one environment variable. Same error happens if you wrongly override other PYTHON* variables.

@skirpichev
Copy link
Member

@refeed, please avoid rebase/force-pushes/etc next time. Just add new commits while your pr being reviewed.

Python/codecs.c Outdated
@@ -1526,7 +1526,7 @@ _PyCodec_InitRegistry(PyInterpreterState *interp)
// search functions, so this is done after everything else is initialized.
PyObject *mod = PyImport_ImportModule("encodings");
if (mod == NULL) {
return PyStatus_Error("Failed to import encodings module");
return PyStatus_Error("Failed to import encodings module. Are you sure PYTHONHOME is correct?");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we tell the user what the (wrong) value of PYTHONHOME is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but that significantly complicates things, because the error message has to get put on the heap (PyStatus can't do that, AFAIK). There's also a chance it's unset in the case of embedding.

@ZeroIntensity
Copy link
Member

Eh, I disagree. I've encountered this error myself, it's common when embedding the interpreter because if PYTHONHOME is unset this error comes up. I'm happy to make this more general though (maybe instead of specifying PYTHONHOME we could just point to the env var docs).

@refeed
Copy link
Author

refeed commented Oct 28, 2024

Alright, for the env link can I use this: https://docs.python.org/3/using/cmdline.html#environment-variables ?

I'm thinking that the final error message will be:

Failed to import encodings module. Are you sure PYTHONHOME or other variable is correct? Docs on environment variables can be found here: https://docs.python.org/3/using/cmdline.html#environment-variables

I think we still have to mention the PYTHONHOME there because for most cases, I think the culprit is going to be the PYTHONHOME

What do you think?

@ZeroIntensity
Copy link
Member

If we're going to go the route of casing this for all invalid variables (I'm not fully convinced, but let's go with it for now), then I don't think actually putting a link to the docs is the right way to do it. We should sort of hint where to look in the docs, something like this should work:

Failed to import encodings module. Your environment probably has misconfigured variables!

@ZeroIntensity ZeroIntensity dismissed their stale review October 28, 2024 23:39

There's some contention about what should happen here. I'm dismissing my review until that gets solved.

@ZeroIntensity
Copy link
Member

@skirpichev This is a more extensive message now, do you like this better?

@skirpichev
Copy link
Member

Yes, looks better. Perhaps we could add some reference to --help-env, that will list such variables?

@refeed
Copy link
Author

refeed commented Oct 29, 2024

Alright, how about this?

Failed to import encodings module. Your environment probably has misconfigured variables! Please refer to `--help-env`

@konstin
Copy link

konstin commented Oct 29, 2024

I believe mentioning PYTHONHOME explicitly is important, see the examples in #125873. As a user, the error message links me to --help-env, but then --help-env doesn't provide any guidance on how to fix the problem nor does it mention the import error i just encountered.

@refeed
Copy link
Author

refeed commented Oct 29, 2024

Alright, how about this?

Failed to import encodings module. Your environment probably has misconfigured variables! Please check PYTHONHOME otherwise refer to `--help-env`

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

Successfully merging this pull request may close these issues.

4 participants