-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
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
Modules/pyexpat.c violates PEP 384 #66652
Comments
Modules/pyexpat.c includes some archaic code to create temporary frames The way this is implemented is a problem for three reasons:
The attached patch fixes these issues. |
Not sure how this can violate PEP-384, as it doesn't make it mandatory for builtin extensions to use the stable ABI. The other concerns seem valid, although I don't like how the patch includes an unrelated change to ctypes. |
W.r.t PEP-384: W.r.t. the change to ctypes: I have simply moved a function from ctypes.c to traceback.c (and renamed it accordingly) so that pyexpat.c can access it. |
FWIW, I think the patch's approach is ok. I just did a small comment on the review UI. |
I don't understand why you think that. PEP-384 is intended to provide the means to maintain binary compatibility of extension modules so that they don't have to be recompiled for every minor version. Standard library extensions are recompiled for every Python release anyway. Also, we don't expect authors to respect PEP-384. Either they choose to do so, and get the benefits, or they don't.
I can see that. It would be cleaner to do the change in two steps, first move the utility function out from ctypes, then use it from pyexpat. On the other hand we haven't been adamant about one-patch-one-change in the past. |
Here is an updated patch with a test. |
New changeset 5433ef907e4f by Antoine Pitrou in branch '3.4': New changeset f2f13aeb590a by Antoine Pitrou in branch 'default': |
Patch pushed. I've kept the changes together :) Hopefully there won't be any ctypes regression. |
according to https://jenkins.qa.ubuntu.com/job/vivid-adt-python3.4/7/ test.test_pyexpat.HandlerExceptionTest now fails, but only when running in the installed location, not when running the tests from the builddir. any idea why? |
New changeset e4b986350feb by Antoine Pitrou in branch '3.4': New changeset 4990157343c6 by Antoine Pitrou in branch 'default': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: