-
Notifications
You must be signed in to change notification settings - Fork 579
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
Catch AttributeError in addition to OSError in pluginhunspell.py #742
Catch AttributeError in addition to OSError in pluginhunspell.py #742
Conversation
to work around Python 3.12.0a4 ctypes.cdll behavior change from python/cpython@101cfe6
Has this python3 change appeared in any production release of Python yet (non-alpha, non-beta)? Why would they make a backwards incompatible change in a point release without any deprecation warnings? What python bug was that commit meant to actually fix? Unfortunately your pull request is incomplete. Your change is needed in other places in that file, and in the ml (multiple languages) version of the hunspell plugin in multiple places as well sigilgumbo library and possibly others. So please revise your pull request to at least make all the changes where needed in just the file you targeted. I will merge it and I will track down all of the other places where cdll is used in our python code. Thank you for your bug report and PR! The python3 crowd should stop making backwards incompatible changes in minor point releases. It is a very bad practice. |
Yup, it appeared in 3.12.0a4 and has been part of it ever since, including 3.12 stable, 3.12.1 and the latest 3.12.2. I'm using Ubuntu 24.04 alpha (which is basically still Debian unstable at this point, since it's not yet frozen), which ships Sigil 2.0.1. 3.12 became the default system-wide Python a few days ago, and I noticed it the first time I ran a plugin on an EPUB :) Yeah, I'm pretty annoyed at this as well, it took the better part of two hours to find out what was going on (especially since I hadn't touched ctypes interop before today.) This was basically an undocumented API breakage :/ Didn't realize it needed changes in some other parts, this fixed it in my scenario :) I'll track down the others and push the rest. Thanks! |
Oh, as it turns out, the patch for this file at least is complete. The other
|
Great detective work. Thank you. |
As an Arch user, I'm amazed (and a little thankful) that they've shown as
much restraint as they have with Python 3.12. They're taking their time
(which is usually against their nature) to make sure that packages that
will be affected (the removal of the imp module in 3.12 being another major
point) are updated first.
But it looks like they're going to move on it fairly soon.
***@***.***/thread/J56DVVEVTSPPV3LNXVEQ6AEZUQFZLHMI/
…On Wed, Feb 28, 2024, 11:34 AM Mihai Limbășan ***@***.***> wrote:
Yup, it appeared in 3.12.0a4 and has been part of it ever since, including
3.12 stable, 3.12.1 and the latest 3.12.2. I'm using Ubuntu 24.04 alpha
(which is basically still Debian unstable at this point, since it's not yet
frozen), which ships Sigil 2.0.1. 3.12 became the default system-wide
Python a few days ago, and I noticed it the first time I ran a plugin on an
EPUB :)
Yeah, I'm pretty annoyed at this as well, it took the better part of two
hours to find out what was going on (especially since I hadn't touched
ctypes interop before today.) This was basically an undocumented API
breakage :/
Didn't realize it needed changes in some other parts, this fixed it in my
scenario :) I'll track down the others and push the rest. Thanks!
—
Reply to this email directly, view it on GitHub
<#742 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACG3CXQPK4HTV7YLD6S4KYDYV5MCDAVCNFSM6AAAAABD5RULX2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRZGM3TSMJUGU>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
BTW, for our peace of mind :) This is how it behaves: LoadLibrary on 3.11, throws OSError:
LoadLibrary() on 3.12.2 -- fortunately unchanged, throws OSError:
Attribute on 3.11.8, throws OSError:
Attribute on 3.12.2, surprise AttributeError:
|
BTW #2, still for our peace of mind and taking advantage of the opportunity: I've messed around with my usual workflows and have not come across any other Sigil problem with Python 3.12, so I think we should be free from major, uh, unexpected improvements :) Of course, individual plugins may or may not be affected, but the Sigil core plugin infra looks good for 3.12. Small mercies. |
Oh, and I'll take care of alerting the Debian Sigil maintainer of the issue and maybe trying to upstream 2.1 once it's out. |
Thank you. Feel free to point to your commits for cherry picking into Debian 2.0.2 as 2.1.0 will not be officially out until just after the end of March. |
Catch AttributeError in addition to OSError in pluginhunspell.py to work around Python 3.12.0a4 ctypes.cdll behavior change introduced in this commit as a response to this issue: In 3.12.0a4 or newer, a key lookup when treating
cdll
as a dict no longer raises anOSError
, but instead raises anAttributeError
.Fixes #741 .
This change is backwards compatible with any Python version.
Alternatively, instead of
cdll[hunspell_dllpath]
we could usecdll.LoadLibrary(hunspell_dllpath)
there, but:so let's be cautious and just catch the new exception, since this way we won't need to rely on any dynloader implementation subtleties.