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.

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

Possibly out of date C extension documentation #75624

Closed
romuald mannequin opened this issue Sep 13, 2017 · 17 comments
Closed

Possibly out of date C extension documentation #75624

romuald mannequin opened this issue Sep 13, 2017 · 17 comments
Labels
3.7 (EOL) end of life docs Documentation in the Doc dir type-feature A feature request or enhancement

Comments

@romuald
Copy link
Mannequin

romuald mannequin commented Sep 13, 2017

BPO 31443
Nosy @amauryfa, @tiran, @bitdancer, @skrah, @embray, @romuald, @lazka
PRs
  • bpo-31443: Formulate the type slot initialization rules in terms of C99. #3688
  • bpo-31443: Update included code. #3697
  • 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 2017-09-22.17:39:48.956>
    created_at = <Date 2017-09-13.09:03:10.526>
    labels = ['type-feature', '3.7', 'docs']
    title = 'Possibly out of date C extension documentation'
    updated_at = <Date 2017-09-22.17:39:48.955>
    user = 'https://github.com/romuald'

    bugs.python.org fields:

    activity = <Date 2017-09-22.17:39:48.955>
    actor = 'skrah'
    assignee = 'docs@python'
    closed = True
    closed_date = <Date 2017-09-22.17:39:48.956>
    closer = 'skrah'
    components = ['Documentation']
    creation = <Date 2017-09-13.09:03:10.526>
    creator = 'Romuald'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31443
    keywords = ['patch']
    message_count = 17.0
    messages = ['302038', '302074', '302364', '302365', '302386', '302388', '302460', '302642', '302643', '302646', '302650', '302711', '302713', '302746', '302750', '302752', '302753']
    nosy_count = 8.0
    nosy_names = ['amaury.forgeotdarc', 'christian.heimes', 'r.david.murray', 'skrah', 'docs@python', 'erik.bray', 'Romuald', 'lazka']
    pr_nums = ['3688', '3697']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue31443'
    versions = ['Python 3.7']

    @romuald
    Copy link
    Mannequin Author

    romuald mannequin commented Sep 13, 2017

    In the "Defining New Types documentation" basics about tp_new -> PyType_GenericNew, the doc states:

    We’d like to just assign this to the tp_new slot, but we can’t, for
    portability sake, On some platforms or compilers, we can’t statically
    initialize a structure member with a function defined in another C
    module, so, instead, we’ll assign the tp_new slot in the module
    initialization function just before calling PyType_Ready():

    But looking a python C code itself [1] does seem to do this. So I'm guessing that part of the documentation is now irrelevant and the example could just assign PyType_GenericNew to tp_new.

    [1]

    PyType_GenericNew, /* tp_new */

    @romuald romuald mannequin assigned docspython Sep 13, 2017
    @romuald romuald mannequin added docs Documentation in the Doc dir type-feature A feature request or enhancement labels Sep 13, 2017
    @amauryfa
    Copy link
    Member

    On Windows at least: I think "another C module" should be understood as "another DLL or executable".

    A user extension module is a different DLL from the core Python, so the advice is still valid.

    @romuald
    Copy link
    Mannequin Author

    romuald mannequin commented Sep 17, 2017

    I'm not sure this is relevant, but I've just made a simple test on Windows (7) with my C extension using { … .tp_new = PyType_GenericNew } and the compiler did not complain (also the code worked as expected)

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 17, 2017

    PyType_GenericNew() should be in libpython, so indeed the example in the docs seems a bit odd to me.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 17, 2017

    FWIW, I've been using

    https://github.com/python/cpython/blob/master/Modules/_decimal/_decimal.c#L689

    the static initialization on problematic platforms like Windows and
    AIX for years, without any problems.

    I'm sure this is valid C, and people here agree:

    https://stackoverflow.com/questions/6989107/cant-initialize-static-structure-with-function-pointer-from-another-translation

    Perhaps this was an issue on very old IRIX systems or similar?

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 17, 2017

    Christian, do you remember which compiler was the reason for the commit

    cbf3b5c ?

    + /* Due to cross platform compiler issues the slots must be filled
    + * here. It's required for portability to Windows without requiring
    + * C++. */
    Null_Type.tp_base = &PyBaseObject_Type;
    + Null_Type.tp_new = PyType_GenericNew;
    Str_Type.tp_base = &PyUnicode_Type;

    I've never had problems with VS 2008 or greater.

    @tiran
    Copy link
    Member

    tiran commented Sep 18, 2017

    I think it's svnmerge of this commit, which talks about VS 2005. It might have been required to support VS 2003 and 2005.

     r59290 | christian.heimes | 2007-12-03 14:47:29 +0100 (Mon, 03 Dec 2007) | 3 lines
    
      Applied my patch python/cpython#45796 with some extra fixes for VS 2005
      The new msvc9compiler module supports VS 2005 and VS 2008. I've also fixed build_ext to support PCbuild8 and PCbuild9 and backported my fix for xxmodule.c from py3k. The old code msvccompiler is still in place in case somebody likes to build an extension with VS 2003 or earlier.
      I've also updated the cygwin compiler module for VS 2005 and VS 2008. It works with VS 2005 but I'm unable to test it with VS 2008. We have to wait for a new version of cygwin.
    

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 20, 2017

    Thanks, Christian. -- I found that in the docs the culprit is Cygwin:

    db6a569

    @tiran
    Copy link
    Member

    tiran commented Sep 20, 2017

    Do we still support cygwin?

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 20, 2017

    I've just asked on python-dev if Cygwin is still broken. Not sure
    if we support it.

    @bitdancer
    Copy link
    Member

    We do not currently officially support cygwin. There are people working on getting it working again (and having a buildbot) so we can support it, so cygwin support is a goal, but not currently a requirement. I've nosied Erik Brey, who is one of the main people interested in and working on cygwin. The note might well be out of date even with regards to cygwin.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 21, 2017

    Erik, if you are interested in Cygwin, could you please check that xxmodule.c builds on Cygwin with the patch? You need to uncomment
    a couple of lines in setup.py to build 'xx'.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 21, 2017

    In fact, building _decimal should also fail on Cygwin if this
    were still an issue.

    @lazka
    Copy link
    Mannequin

    lazka mannequin commented Sep 22, 2017

    Building the following with gcc from msys2 (cygwin) worked fine here: https://bpaste.net/show/0cafd5fa8211

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 22, 2017

    Okay, thanks.

    I found that bz2module.c has also used direct initialization for ages.
    I'm going to commit the change.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 22, 2017

    New changeset ca72589 by Stefan Krah in branch 'master':
    bpo-31443: Formulate the type slot initialization rules in terms of C99. (bpo-3688)
    ca72589

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 22, 2017

    New changeset b1558a0 by Stefan Krah in branch 'master':
    bpo-31443: Update included code. (bpo-3697)
    b1558a0

    @skrah skrah mannequin added the 3.7 (EOL) end of life label Sep 22, 2017
    @skrah skrah mannequin closed this as completed Sep 22, 2017
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life docs Documentation in the Doc dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants