Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-126742: allow to use non-UTF8 exception messages#126746
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-126742: allow to use non-UTF8 exception messages #126746
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ZeroIntensity commented Nov 12, 2024
Technically, since this is a bugfix, let's wait until #126555 has landed before we merge this, and then address all the uses of |
picnixz commented Nov 12, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I actually thought of doing it two PRs. Technically, this specific PR can be merged without waiting for the other and then you can use this interface in the other (rather the other way around). |
ZeroIntensity commented Nov 12, 2024
Eh, I would rather not. Let's not overwhelm someone with thread states and the private API on their first PR :) |
ZeroIntensity commented Nov 13, 2024
I've removed backport labels, per what Petr said in the issue. I think he's right--let's just focus on main with this. |
encukou commented Nov 15, 2024
Hmm, just had an idea: if we add a “ |
ZeroIntensity commented Nov 15, 2024
I like that idea! But we're exposing this as a public API--how often might users need this? (I've never needed to mess with locale in any of my C API usage, but to be fair I'm typically not calling APIs that return strings.) |
picnixz commented Nov 15, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I'm not very fond of adding a case to additionally check when there's not a lot of use cases (unless proven otherwise). It shouldn't affect performances much (but this needs verification) but this would complicate an already complicate function I think (and people need to remember this new format as well). |
ZeroIntensity commented Nov 15, 2024
No no, I don't think he meant a variation of |
picnixz commented Nov 15, 2024
And I understood. I don't think a new code should be added unless there is a real need. |
ZeroIntensity commented Nov 15, 2024
Ah! I couldn't tell, sorry. A code search for |
encukou commented Nov 19, 2024
It could also show that we're mainly handling errors from the OS at one point -- the OSError constructor. That would be a good thing :) Anyway, for this PR: @picnixz, do you want to add calls to the new helper(s)? |
ZeroIntensity commented Nov 19, 2024
FWIW, Bénédikt is on vacation right now. I don't think he'll be able to implement anything until next week. |
vstinner left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the purpose of these internal functions since they are now used. This PR only adds dead code. I would prefer to see how these functions are used.
If only dlerror() uses it, why not using code using dlerror()?
Uh oh!
There was an error while loading. Please reload this page.
Sorry, @picnixz and @encukou, I could not cleanly backport this to |
…r messages (pythonGH-126746) - Add a helper to set an error from locale-encoded `char*` - Use the helper for gdbm & dlerror messages (cherry picked from commit 7303f06) Co-authored-by: Bénédikt Tran <[email protected]> Co-authored-by: Victor Stinner <[email protected]>
GH-128023 is a backport of this pull request to the 3.13 branch. |
serhiy-storchaka commented Dec 17, 2024
This is a bug fix. It should be backported. |
picnixz commented Dec 17, 2024
Ah! I think we should also have a NEWS entry. I'll add one. Sorry for forgetting about it :( |
picnixz commented Dec 17, 2024
Concerning the failure on 3.12 backport it's because we did not backported the PR on |
…or messages (GH-126746) (GH-128023) - Add a helper to set an error from locale-encoded `char*` - Use the helper for gdbm & dlerror messages (cherry picked from commit 7303f06) Co-authored-by: Bénédikt Tran <[email protected]> Co-authored-by: Victor Stinner <[email protected]>
Sorry, @picnixz and @encukou, I could not cleanly backport this to |
picnixz commented Dec 17, 2024
Grrr. I thought that the conflicts would be easily solved but apparently not. I'll do the backport manually. |
…r messages (pythonGH-126746) - Add a helper to set an error from locale-encoded `char*` - Use the helper for gdbm & dlerror messages Co-authored-by: Victor Stinner <[email protected]>
GH-128027 is a backport of this pull request to the 3.12 branch. |
…or messages (GH-126746) (GH-128027) - Add a helper to set an error from locale-encoded `char*` - Use the helper for gdbm & dlerror messages Co-authored-by: Victor Stinner <[email protected]>
bedevere-bot commented Dec 17, 2024
|
picnixz commented Dec 17, 2024
Ok, so I didn't expect that on some archs, the error message would be different... |
bedevere-bot commented Dec 17, 2024
|
vstinner commented Dec 17, 2024
Tests are failing on PPC64LE RHEL8 LTO 3.x:
|
picnixz commented Dec 17, 2024
I've opened #128034 for this but Serhiy suggested removing the filename check as well. |
bedevere-bot commented Dec 17, 2024
|
bedevere-bot commented Dec 17, 2024
|
bedevere-bot commented Dec 17, 2024
|
bedevere-bot commented Dec 17, 2024
|
…r messages (pythonGH-126746) - Add a helper to set an error from locale-encoded `char*` - Use the helper for gdbm & dlerror messages Co-authored-by: Victor Stinner <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.