Add pythoncapi_compat.h header

[vstinner]

Don't access directly PyFrameObject and PyThreadState structure
members but use getter functions instead:

• Replace frame->f_code with _PyFrame_GetCodeBorrow(frame)
• Replace tstate->frame with _PyThreadState_GetFrameBorrow(tstate)
• Replace frame->f_back with _PyFrame_GetBackBorrow(frame)

Copy pythoncapi_compat.h from the
https://github.com/pythoncapi/pythoncapi_compat project to have these
getter functions. It supports Python 2.7-3.10 and PyPy 2.7-3.7.

detail/common.h now includes pythoncapi_compat.h rather than Python.h
and frameobject.h. Use detail/common.h where Python.h or
pythoncapi_compat.h is needed.

Description

Suggested changelog entry:

[vstinner]

This PR is part of my draft PEP 620 which tries to hide implementation details from the C API to prepare CPython for optimizations which changing structures of the Python C API: https://www.python.org/dev/peps/pep-0620/

[vstinner]

I just added PyPy support to pythoncapi_compat.h for pybind11 ;-)

[rwgk]

Hi @vstinner, that looks great, thanks! Just some small things:

• I think the new include file also needs to be added to the top-level CMakeLists.txt. (I was thinking the CI will flag it but apparently not.)
• How do you feel about moving the new file to the detail subdirectory? — Mainly because it's not meant to be part of the public pybind11 API.
• I see the include guard is not pybind11 specific. What if pybind11 is used in combination with another project that has an older or newer copy of pythoncapi_compat.h? Could it be safer to use #pragma once, which is also what's being used in all other include files here.

[henryiii]

I think the idea is that this is a vendored copy of cpython_capi. To indicate this, what about putting it in a "vendored" subdirectory? That's what pip and a lot of other projects do; it will make it more obvious we need to copy a new version in for fixes instead of maintaining it ourselves (except in emergencies). If not, it should be at least in detail.

I was thinking the CI will flag it but apparently not

This is mostly just to make this play nice with IDEs, it doesn't have an affect on the normal generation, so I think that's why this didn't get flagged. (IIRC, could be wrong).

What if pybind11 is used in combination with another project that has an older or newer copy

But would it be safe to then mix them? I'd think probably not, they will be defining mostly the same things?

[rwgk]

I think the idea is that this is a vendored copy of cpython_capi. To indicate this, what about putting it in a "vendored" subdirectory? That's what pip and a lot of other projects do; it will make it more obvious we need to copy a new version in for fixes instead of maintaining it ourselves (except in emergencies). If not, it should be at least in detail.

+1

I was thinking the CI will flag it but apparently not

This is mostly just to make this play nice with IDEs, it doesn't have an affect on the normal generation, so I think that's why this didn't get flagged. (IIRC, could be wrong).

2cents: our setup is fine as is. Easy to fix if overlooked.

What if pybind11 is used in combination with another project that has an older or newer copy

But would it be safe to then mix them? I'd think probably not, they will be defining mostly the same things?

I'm actually pretty worried about this one. In my role as pybind11 maintainer for Google's internal and external needs, I have to think large scale. Versions will get mixed for sure, at a large scale. Thinking ahead could safe a lot of grief later. Question: how can we make sure in pybind11 we get our own version, even if another version was included previously, but not clash? Is that even possible without using namepaces? — @vstinner has this question been discussed previously? Another thought: baking versioning into pythoncapi_compat.h?

[rwgk]

Another thought/concern: here we have manipulations that need to happen before Python.h is included:

pybind11/include/pybind11/detail/common.h

Line 105 in 1259db6

/* Don't let Python.h #define (v)snprintf as macro because they are implemented

First idea: replace the existing #include <Python.h> in common.h with including this header.

[henryiii]

There are actually several reasons to control Python.h's include order, including this one, but not all of them are even pybind11 specific. By the way, note on the header - you shouldn't need extern C when including Python.h, it explicitly supports inclusion from C++.

[Skylion007]

Any reason for this not to be a submodule? Would make it much easier to update it for newer Python versions and such?

[henryiii]

We had a few submodules for a while and subdued in a library intended to be a submodule doesn’t go well. Mostly made gitlab users mad. ;) Everyone was happy to see them go.

It’s harder to fork and customize in emergencies, too; changing the url can lead to issues.

I love submodules, but probably not for pybind11 itself.

A single file is not bad, git subtree isn’t bad either.

[vstinner]

I modified my PR to use detail/common.h where Python.h or pythoncapi_compat.h is needed. common.h wraps Python.h, frameobject.h and pythoncapi_compat.h headers.

I also squashed commits to modify the commit message. I updated pythoncapi_compat.h to get the fix for PyPy 3.7.

[vstinner]

@Skylion007:

Any reason for this not to be a submodule? Would make it much easier to update it for newer Python versions and such?

IMO it's just more convenient to simply copy the file. Unless you need a new function added to pythoncapi_compat.h, the only reason to update it would to get a fix to support a new Python version (ex: my latest update for PyPy 3.7, or another fix for Python 2.7 on Windows). Usually, you don't have to update this header file.

@rwgk:

I see the include guard is not pybind11 specific. What if pybind11 is used in combination with another project that has an older or newer copy of pythonc

@henryiii:

But would it be safe to then mix them? I'd think probably not, they will be defining mostly the same things?

pythoncapi_compat.h is not designed to be included twice, that's why the whole header is surrounded by this guard:

#ifndef PYTHONCAPI_COMPAT
#define PYTHONCAPI_COMPAT
...
#endif  // PYTHONCAPI_COMPAT
...

For example, the only guard deciding if PyFrame_GetCode() should be defined or not is the Python version. You must not define the same function twice.

// bpo-40421 added PyFrame_GetCode() to Python 3.9.0b1
#if PY_VERSION_HEX < 0x030900B1
static inline PyCodeObject*
PyFrame_GetCode(PyFrameObject *frame)
{
    assert(frame != NULL);
    assert(frame->f_code != NULL);
    return (PyCodeObject*)Py_NewRef(frame->f_code);
}
#endif

If pybind11 is used in a project which uses a newer pythoncapi_compat.h, but pybind11 includes its older pythoncapi_compat.h, the project can fail to build if it needs a new function. There are different options to fix this issue:

• the project should include its own pythoncapi_compat.h copy before pybind11.h
• pybind11 pythoncapi_compat.h copy should be updated
• pythoncapi_compat.h should become a system header file, the system copy should preferred and be the most up to date version. I'm not sure that this option is the best since Linux distributions tend to pick one version and then no longer update it before a new version of the Linux distribution.

Honestly, this issue is annoying, and I'm not sure which option is the most convenient in practice.

A simpler approach is to avoid copying pythoncapi_compat.h into applications using pybind11, since pybind11 already uses it, no?

[vstinner]

Another alternative would be to redesign pythoncapi_compat.h to put guards on every function to allow including two versions of the header file. For example, including an old versions will define 10 functions, and then including the new version defines a 11th function. If you want, I can try to redesign pythoncapi_compat.h for that. But is it really required?

As I wrote, the most simple option is to avoid copying pythoncapi_compat.h in applications using pybind11, since pybind11 already includes pythoncapi_compat.h.

[rwgk]

Another alternative would be to redesign pythoncapi_compat.h to put guards on every function to allow including two versions of the header file. For example, including an old versions will define 10 functions, and then including the new version defines a 11th function. If you want, I can try to redesign pythoncapi_compat.h for that. But is it really required?

I think that's a great idea, and it's essential.
It must come with a strong promise: what's inside a given include guard must never change. — Is that a promise you think you could make?

Thoughts that led me to the above:

• Spreading copies will inevitably create multi-diamond-dependency issues, sooner or later.
• For that reason everybody is usually using versioning (pip). Following that time-tested mainstream approach seems safest.
• But I guess many people would choose to keep their current ifdefs rather than having to manage the extra dependency.
• So we're essentially cutting corners by copying directly. Therefore it needs extra thought.
• Otherwise I think this will happen: initially a few projects makes copies and all is fine, all copies are identical. Time passes, more copies, modifications creep in. People fiddle with include order, with limited success. Over time the problem gets worse. People back out the copies and go back to the ifdefs they had before.

[vstinner]

It must come with a strong promise: what's inside a given include guard must never change. — Is that a promise you think you could make?

What's inside a given include guard did change recently, because I had to change this include guard. Example of change:

+#if !defined(PYPY_VERSION)
 static inline PyFrameObject*
 _PyFrame_GetBackBorrow(PyFrameObject *frame)
 {
     return (PyFrameObject *)_Py_XStealRef(PyFrame_GetBack(frame));
 }
+#endif

_PyFrame_GetBackBorrow() is no longer defined on PyPy, since the PyFrameObject structure has no f_back member on PyPy.

But I'm not sure that it is your concern. Previously, the header file didn't work on PyPy.

[rwgk]

But I'm not sure that it is your concern. Previously, the header file didn't work on PyPy.

Yes, that's what my concern is about. For example

• pybind11 has the old copy.
• scipy the new.
• I want to start supporting PyPy in mypackage.
• I find I need to include scipy before pybind11, which didn't matter before.
• Time passes. pybind11 is updated and comes with a newer-than-scipy copy of pythoncapi_compat.h.
• One morning I find a PR shuffling the include order in mypackage because anotherpackage needs the newest pythoncapi_compat.h.
• The next morning I find an Issue someone opened because the shuffling broke them, and for some strong reason they cannot update pybind11, while also being forced to update mypackage.

I guess there'll (almost) always be a way out if people coordinate enough, but my point is mainly that spreading unversioned copies invites diamond dependency conflicts, sooner or later, and that there will be a lot of lost time dealing with them.

[henryiii]

What's the status of this? Just curious.

Also, is there a way to detect usage of deprecated structs in CPython? Just thinking about #3368, where the changed internals in 3.11 alerted us to the fact we should have been using new public API since Python 3.9. But it was usage of struct, not a function, so we didn't know we were using something that now had a public method for access.

[vstinner]

I didn't update yet pythoncapi_compat to support inclusion of different versions of the same file without having conflicts. Help would be welcomed to implement this use case.