-
Notifications
You must be signed in to change notification settings - Fork 43
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
Use PyErr_FormatUnraisable() on Python 3.13 #34
Conversation
src/c/_cffi_backend.c
Outdated
@@ -6109,6 +6108,18 @@ static void _my_PyErr_WriteUnraisable(PyObject *t, PyObject *v, PyObject *tb, | |||
if (extra_error_line == NULL) | |||
extra_error_line = ""; | |||
|
|||
#if PY_VERSION_HEX >= 0x030D0000 | |||
if (obj != NULL) { | |||
PyErr_FormatUnraisable("Exception ignored %c%s%R%s", |
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.
Maybe it's an opportunity to enhance the error message?
Should it be "Exception ignored ...", "Exception ignored in ..." (add in
) or "Exception ignored: ..." (add :
)?
By the way, |
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.
You forget to add PyErr_Restore()
.
src/c/_cffi_backend.c
Outdated
first_char, objdescr + 1, extra_error_line); | ||
} | ||
#else | ||
PyObject *s; |
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.
This does not compile on older Visual Studio versions, but I'm unsure how old these need to be. You can't declare a variable anywhere else than at the start of a block in older versions of C.
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.
Ok, fixed. I made the assumption that cffi is built by a C99 compiler these days.
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'm trying to keep the code compiling for 2.7, and that might require much older VS on Windows.
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'm trying to keep the code compiling for 2.7
@arigo I'm hoping to convince you that that's not a realistic goal in 2023 with the resources available to this project 😉 . It's clearly still possible to make it work, but keeping test/CI infrastructure alive that's capable of actually verifying that (before breaking changes get merged) is already prohibitively expensive, and getting more so by the day. For the past few years, it's not even possible to download the 2.7 Python build tooling from Microsoft anymore- I keep a private stash of some of those tools around because I still have to deal with it occasionally, but that's not really workable for modern GHA runners. My general approach is "if it's not being tested, it should be assumed broken", and I can't justify spending any time trying to test or fix that.
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'm trying hard for provide a best effort support for Python 2.7 in my https://pythoncapi-compat.readthedocs.io/ project. I had C code to support VS2015 which doesn't support C99 static inline
. But I removed it, it made the code harder to maintain. Moreover, GitHub Action doesn't provide Python 2.7. I'm now considering to officially remove Python 2 support and remove Python 2 code in pythoncapi-compat.
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.
Yep, I've tried "best effort" support like this so many times, but even with the best of intentions, it's effectively always broken when not backed by automated testing. It also ends up actively discouraging contributions (like this one!)...
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.
@nitzmahone Sure, I see the point, but in this case the final result is that the pull request was changed to 4 obvious lines instead of a more complex #if dance that missed originally a PyErr_Restore()
, in addition to breaking a compiler (that may or may not be too old to consider). I think it's a win for a project like CFFI.
Oops, fixed. |
@serhiy-storchaka has a point. And I'd be happier with some change that avoids making two versions of the formatting code. As far as I'm concerned, killing a semi-private API is a minor annoyance that I'd be more happy to work around, instead of trying to rewrite more CFFI logic, because that ends up making duplication. So I wouldn't kill |
Use the new public PyErr_FormatUnraisable() on Python 3.13. The private _PyErr_WriteUnraisableMsg() function was removed in Python 3.13: python/cpython#111643
Sure, it makes sense. I rewrote my PR to also use the s string on Python 3.13. The patch is now just 4 lines. @arigo: Would you mind to review the updated PR? |
Sure. Now I have no hesitation to merge the pull request. @nitzmahone, can you please confirm if I should "Enable auto-merge (squash)" for this kind of case? |
Yeah, that's fine- basically "I'm happy with the code, merge on my behalf if all checks pass" vs babysitting it... |
Thanks for the review and merging my change! I first wanted to enhance the error message, but I understand the desire to have the same message on all Python versions and it makes sense. Ah yeah, I missed |
@vstinner @arigo Bleh- I threw together a manylinux_2_28 container with 3.13.0a1 in it and hacked up
I don't see any libpython exports for FormatUnraisable:
|
(or maybe the new libpython export didn't make it into 3.13.0a1 yet)- in any case, this doesn't appear to be fully working yet... |
OK, looks like this will be fixed in 3.13.0a2 (I just built against the top of
|
... and yep, full test suite passes against that build of "almost-alpha2", so I think we're good to go here... |
git clean -xdf tar zcvf ../python-cffi_1.16.0.orig.tar.gz --exclude=.git . debuild -uc -us cp python-cffi.spec ../python-cffi_1.16.0-1.spec cp ../python*-cffi*1.16.0*.{gz,xz,spec,dsc} /osc/home\:alvistack/python-cffi-cffi-1.16.0/ rm -rf ../python*-cffi*1.16.0*.* ../python*-cffi-backend*1.16.0*.* See python-cffi#34 Signed-off-by: Wong Hoi Sing Edison <hswong3i@pantarei-design.com>
Use the new public PyErr_FormatUnraisable() on Python 3.13.
The private _PyErr_WriteUnraisableMsg() function was removed in Python 3.13:
python/cpython#111643