8000 Mnt py310 compat by tacaswell · Pull Request #636 · pycurl/pycurl · GitHub
[go: up one dir, main page]
Skip to content

Mnt py310 compat #636

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
Closed
wants to merge 3 commits into from
Closed

Mnt py310 compat #636

wants to merge 3 commits into from

Conversation

tacaswell
Copy link

There was a change in the CPython c-api to change Py_TYPE from a macro to an inline function (python/cpython#20290). This requires a change to using Py_SET_TYPE which was introduced on py39a4.

tacaswell added 2 commits May 29, 2020 14:48
@tacaswell
MNT: ignore egg-info file
729fd54
@tacaswell
MNT: support py310
a5312df 
There was a change in the CPython c-api to change `Py_TYPE` from a
macro to an inline
function (python/cpython#20290).  This
requires a change to using `Py_SET_TYPE` which was introduced on
py39a4.
@p
Copy link
Member
p commented May 30, 2020

Thank you for the patch. The code in question probably dates back to ancient pythons, is there a way to get equivalent functionality using modern c api methods that work in python 3.5+?

@tacaswell
Copy link
Author

With this patch (which includes a compatibility macro) it should work with all py35+ so in the most trivial sense I think the answer is yes.

If the question is how to refactor this code to avoid using Py_SET_TYPE I am unfortunately in over my head to answer.

@p
Copy link
Member
p commented Jun 2, 2020

I do not think it is appropriate for pycurl to define macros provided by python.

If python provides a standard API that replaces Py_SET_TYPE, pycurl could potentially use that. If Py_SET_TYPE is available by including another file, pycurl could do that. But copying python implementation into pycurl requires significant justification.

@tacaswell
Copy link
Author

Python only provides this macro for py39a4 . Defining the macro for earlier versions of Python means that you can put the version check in exactly one place in your code. I guess another option is to wrap the three offending lines in the #if.. block and just have the same code twice (once in the new way once in the old way) like

#if PY_VERSION_HEX >= 0x030900a4
   Py_SET_TYPE(&Curl_Type, &PyType_Type); 
   Py_SET_TYPE(&CurlMulti_Type,  &PyType_Type); 
   Py_SET_TYPE(&CurlShare_Type,  &PyType_Type);
#else 
   Py_TYPE(&Curl_Type) = &PyType_Type;
   Py_TYPE(&CurlMulti_Type) = &PyType_Type;
   Py_TYPE(&CurlShare_Type) = &PyType_Type;
#endif

but I am not sure that is clearer. Using a compatibilty macro like I proposed is what both numpy and cython have done.

@p
Copy link
Member
p commented Jun 2, 2020 •
edited
Loading

Please find out a public Python API that accomplishes the desired effect in all supported Python versions. If this is not possible please link to a discussion/statement from Python developers where they are stating that the only way to write a Python extension is to use Python version checks as proposed in this PR.

@tacaswell
Copy link
Author

Please see the link in my initial post and the links back to bpo.

This is how @vstinner (core CPython dev) fixed the issue in cython cython/cython@d8e93b3 and how it was fixed in numpy numpy/numpy#16417

I guess you could go with the method Victor describes in MagicStack/immutables#46 as well.

With out this patch pycurl does not compile with cpython master and will be broken with py310.

@p
Copy link
Member
p commented Jun 3, 2020

Okay. My web search is not returning any results for Py_TYPE, therefore I suppose the next step is to figure out what that macro does and why pycurl uses it.

@vstinner
Copy link
vstinner commented Jun 3, 2020

If you want to use Py_SET_SIZE() on all Python versions, you can define it on older Python versions using:

#if PY_VERSION_HEX < 0x030900A4
#  define Py_SET_TYPE(obj, size) do { Py_TYPE(obj) = (size); } while (0)
#endif

vstinner
vstinner reviewed Jun 3, 2020
View reviewed changes
@tacaswell @vstinner
MNT: tweak marco
c2639fa 
Co-authored-by: Victor Stinner <vstinner@python.org>
@tacaswell
Copy link
Author

Why wrap the call in a do...while? Should I apply that change to numpy as well?

@vstinner
Copy link
vstinner commented Jun 3, 2020

Why wrap the call in a do...while? Should I apply that change to numpy as well?

It's an old habbit that I have, it's to reduce the risks of https://gcc.gnu.org/onlinedocs/cpp/Macro-Pitfalls.html

It helps when using a macro in an if or while block without { ... }.

For this specific macro, maybe it's safe without it.

@p
Copy link
Member
p commented Jun 3, 2020

What does Py_SET_TYPE do and in what circumstances would one use it?

@larsoner larsoner mentioned this pull request Jun 9, 2020
Closed
@p
Copy link
Member
p commented Oct 27, 2020

Merged in #660.

@p p closed this Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tacaswell @p @vstinner
0