-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-34537: Fix test_gdb:test_strings with LC_ALL=C #9483
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
Conversation
We cannot simply call locale.getpreferredencoding() here, as GDB might have been linked against a different version of Python with a different encoding and coercion policy with respect to PEP 538 and PEP 540. Thanks to Victor Stinner for a hint on how to fix this.
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.
LGTM, but I have a few minor requests.
@@ -0,0 +1,2 @@ | |||
Fix test_gdb:test_strings when LC_ALL=C and GDB was compiled with Python 3.6 |
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 suggest "test_gdb.test_strings()" or "test_strings() of test_gdb". You might use `` around function and file names.
Lib/test/test_gdb.py
Outdated
if err: | ||
raise RuntimeError( | ||
'unable to determine the preferred encoding ' | ||
'of embedded Python in GDB.') |
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.
Would you mind to include err in the error message? You can use an f-string.
You might also raise the error if encoding is an empty string (if err or not encofing: ...)
Lib/test/test_gdb.py
Outdated
'unable to determine the preferred encoding ' | ||
'of embedded Python in GDB.') | ||
|
||
encoding = out.strip() |
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.
.rstrip() should be enough ;-)
Lib/test/test_gdb.py
Outdated
if not encoding: | ||
raise RuntimeError( | ||
f'unable to determine the preferred encoding ' | ||
f'of embedded Python in GDB: the command returned no output') |
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.
Oh, I don't think that we need two code path, just reuse the first if.
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 mean, the first error message is fine if encoding is an empty string.
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.
LGTM. Thanks for update!
I tested manually the PR on Linux using LC_ALL=C: it works as expected.
We cannot simply call locale.getpreferredencoding() here, as GDB might have been linked against a different version of Python with a different encoding and coercion policy with respect to PEP 538 and PEP 540. Thanks to Victor Stinner for a hint on how to fix this. (cherry picked from commit 7279b51) Co-authored-by: Elvis Pranskevichus <elvis@magic.io>
GH-9485 is a backport of this pull request to the 3.7 branch. |
We cannot simply call locale.getpreferredencoding() here, as GDB might have been linked against a different version of Python with a different encoding and coercion policy with respect to PEP 538 and PEP 540. Thanks to Victor Stinner for a hint on how to fix this. (cherry picked from commit 7279b51) Co-authored-by: Elvis Pranskevichus <elvis@magic.io>
We cannot simply call locale.getpreferredencoding() here,
as GDB might have been linked against a different version
of Python with a different encoding and coercion policy
with respect to PEP 538 and PEP 540.
Thanks to Victor Stinner for a hint on how to fix this.
https://bugs.python.org/issue34537