Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Modules/pyexpat.c violates PEP 384 #66652

Closed
markshannon opened this issue Sep 22, 2014 · 10 comments
Closed

Modules/pyexpat.c violates PEP 384 #66652

markshannon opened this issue Sep 22, 2014 · 10 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@markshannon
Copy link
Member

BPO 22462
Nosy @birkenfeld, @doko42, @pitrou, @nedbat, @jkloth, @markshannon
Files
  • expat.patch: Patch
  • expat_traceback.patch
    		•

    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:

    assignee = None
closed_at = <Date 2014-10-08.18:08:38.005>
created_at = <Date 2014-09-22.15:51:23.367>
labels = ['type-bug', 'library']
title = 'Modules/pyexpat.c violates PEP 384'
updated_at = <Date 2014-11-29.14:56:49.789>
user = 'https://github.com/markshannon'

    bugs.python.org fields:

    activity = <Date 2014-11-29.14:56:49.789>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2014-10-08.18:08:38.005>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2014-09-22.15:51:23.367>
creator = 'Mark.Shannon'
dependencies = []
files = ['36686', '36833']
hgrepos = []
issue_num = 22462
keywords = ['patch']
message_count = 10.0
messages = ['227278', '228680', '228738', '228739', '228740', '228774', '228802', '228805', '231125', '231863']
nosy_count = 7.0
nosy_names = ['georg.brandl', 'doko', 'pitrou', 'nedbat', 'jkloth', 'Mark.Shannon', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue22462'
versions = ['Python 3.4', 'Python 3.5']
    @markshannon
    Copy link
    Member Author

    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.

    @markshannon markshannon added the stdlib Python modules in the Lib dir label Sep 22, 2014
    @birkenfeld
    Copy link
    Member

    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.

    @markshannon
    Copy link
    Member Author

    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.

    @pitrou pitrou added the type-bug An unexpected behavior, bug, or error label Oct 6, 2014
    @pitrou
    Copy link
    Member

    pitrou commented Oct 6, 2014

    FWIW, I think the patch's approach is ok. I just did a small comment on the review UI.

    @birkenfeld
    Copy link
    Member

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 7, 2014

    Here is an updated patch with a test.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 8, 2014

    New changeset 5433ef907e4f by Antoine Pitrou in branch '3.4':
    Issue bpo-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 bpo-22462: Fix pyexpat's creation of a dummy frame to make it appear in exception tracebacks.
    https://hg.python.org/cpython/rev/f2f13aeb590a

    @pitrou
    Copy link
    Member

    pitrou commented Oct 8, 2014

    Patch pushed. I've kept the changes together :) Hopefully there won't be any ctypes regression.

    @pitrou pitrou closed this as completed Oct 8, 2014
    @doko42
    Copy link
    Member

    doko42 commented Nov 13, 2014

    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?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 29, 2014

    New changeset e4b986350feb by Antoine Pitrou in branch '3.4':
    Close issue bpo-22895: fix test failure introduced by the fix for issue bpo-22462.
    https://hg.python.org/cpython/rev/e4b986350feb

    New changeset 4990157343c6 by Antoine Pitrou in branch 'default':
    Close issue bpo-22895: fix test failure introduced by the fix for issue bpo-22462.
    https://hg.python.org/cpython/rev/4990157343c6

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @vstinner vstinner mentioned this issue Sep 10, 2023
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @birkenfeld @pitrou @markshannon @doko42