Repository navigation
gh-125873: Improve error message when PYTHONHOME is invalid - #126027
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
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 |
63de824 to
c14d804
Compare
|
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 |
|
This is user-facing, so it needs a NEWS entry. Something like "Improved error message when |
|
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 |
6c9bdc4 to
6e83963
Compare
refeed
changed the title
6e83963 to
c4156f6
Compare
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
Thank you! It's good to see new contributors :)
|
I doubt that we need a special treatment just for one environment variable. Same error happens if you wrongly override other |
|
@refeed, please avoid rebase/force-pushes/etc next time. Just add new commits while your pr being reviewed. |
| 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?"); |
There was a problem hiding this comment.
Can we tell the user what the (wrong) value of PYTHONHOME is?
There was a problem hiding this comment.
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.
|
Eh, I disagree. I've encountered this error myself, it's common when embedding the interpreter because if |
|
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: I think we still have to mention the What do you think? |
|
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: |
There's some contention about what should happen here. I'm dismissing my review until that gets solved.
|
@skirpichev This is a more extensive message now, do you like this better? |
|
Yes, looks better. Perhaps we could add some reference to |
|
Alright, how about this? |
|
I believe mentioning PYTHONHOME explicitly is important, see the examples in #125873. As a user, the error message links me to |
|
Alright, how about this? |
|
This PR is stale because it has been open for 30 days with no activity. |
|
Reopening. This was incorrectly closed as duplicate. |
Co-authored-by: Muse Code (Meta Muse Spark) # Conflicts: # Python/codecs.c
5 participants
Summary
Improves the error message shown when the
encodingsmodule fails to import due to a misconfiguredPYTHONHOMEenvironment variable.Before:
Failed to import encodings moduleAfter:
Failed to import encodings module. Your environment probably has misconfigured variables! Please check PYTHONHOME otherwise refer to --help-envThe new message gives users an actionable next step instead of a bare failure notice, which was particularly unhelpful since this error almost always means
PYTHONHOMEis set to an invalid path.A NEWS entry is included under
Core_and_Builtins.