classification
Title: Modules/pyexpat.c violates PEP 384
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Mark.Shannon, doko, georg.brandl, jkloth, nedbat, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2014-09-22 15:51 by Mark.Shannon, last changed 2014-11-29 14:56 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
expat.patch Mark.Shannon, 2014-09-22 15:51 Patch review
expat_traceback.patch pitrou, 2014-10-07 21:15
Messages (10)
msg227278 - (view) Author: Mark Shannon (Mark.Shannon) * Date: 2014-09-22 15:51
Modules/pyexpat.c includes some archaic code to create temporary frames
so that, in even of an exception being raised, expat appears in the traceback.

The way this is implemented is a problem for three reasons:

1. It violates PEP 384.
2. It  is incorrect, see http://bugs.python.org/issue6359.
3. It is inefficient, as a frame is generated for each call, regardless of whether an exception is raised or not.

The attached patch fixes these issues.
msg228680 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-10-06 13:08
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.
msg228738 - (view) Author: Mark Shannon (Mark.Shannon) * Date: 2014-10-06 21:43
W.r.t PEP 384:
Every module, except pyexpat, in the stdlib library treats frames as opaque objects, as PEP 384 requires.
(I'm exempting builtins and sys here)
I think it is unreasonable to expect authors of 3rd party modules to respect PEP 384 if the standard library does not.

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.
msg228739 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-06 21:51
FWIW, I think the patch's approach is ok. I just did a small comment on the review UI.
msg228740 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-10-06 22:01
> I think it is unreasonable to expect authors of 3rd party modules to respect PEP 384 if the standard library does not.

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 have simply moved a function from ctypes.c to traceback.c (and renamed it accordingly) so that pyexpat.c can access it.

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.
msg228774 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-07 21:15
Here is an updated patch with a test.
msg228802 - (view) Author: Roundup Robot (python-dev) Date: 2014-10-08 18:03
New changeset 5433ef907e4f by Antoine Pitrou in branch '3.4':
Issue #22462: Fix pyexpat's creation of a dummy frame to make it appear in exception tracebacks.
https://hg.python.org/cpython/rev/5433ef907e4f

New changeset f2f13aeb590a by Antoine Pitrou in branch 'default':
Issue #22462: Fix pyexpat's creation of a dummy frame to make it appear in exception tracebacks.
https://hg.python.org/cpython/rev/f2f13aeb590a
msg228805 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-08 18:08
Patch pushed. I've kept the changes together :) Hopefully there won't be any ctypes regression.
msg231125 - (view) Author: Matthias Klose (doko) * (Python committer) Date: 2014-11-13 17:39
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?
msg231863 - (view) Author: Roundup Robot (python-dev) Date: 2014-11-29 14:56
New changeset e4b986350feb by Antoine Pitrou in branch '3.4':
Close issue #22895: fix test failure introduced by the fix for issue #22462.
https://hg.python.org/cpython/rev/e4b986350feb

New changeset 4990157343c6 by Antoine Pitrou in branch 'default':
Close issue #22895: fix test failure introduced by the fix for issue #22462.
https://hg.python.org/cpython/rev/4990157343c6
History
Date User Action Args
2014-11-29 14:56:49python-devsetmessages: + msg231863
2014-11-18 13:14:17jklothsetnosy: + jkloth
2014-11-13 17:39:24dokosetnosy: + doko
messages: + msg231125
2014-10-08 18:08:38pitrousetstatus: open -> closed
resolution: fixed
messages: + msg228805

stage: patch review -> resolved
2014-10-08 18:03:10python-devsetnosy: + python-dev
messages: + msg228802
2014-10-07 21:15:11pitrousetfiles: + expat_traceback.patch

messages: + msg228774
2014-10-06 22:01:09georg.brandlsetmessages: + msg228740
2014-10-06 21:51:58pitrousetmessages: + msg228739
2014-10-06 21:47:12pitrousetstage: patch review
type: behavior
versions: + Python 3.4
2014-10-06 21:43:44Mark.Shannonsetmessages: + msg228738
2014-10-06 13:08:45georg.brandlsetnosy: + georg.brandl, pitrou
messages: + msg228680
2014-10-06 13:08:08georg.brandllinkissue6359 superseder
2014-09-22 15:52:19Mark.Shannonsetnosy: + nedbat
2014-09-22 15:51:23Mark.Shannoncreate