Move internal headers to Include/internal/ #79262
Comments
|
Currently, #include "pymem.h" may include Include/pymem.h or Include/internal/pymem.h depending where is the C file (.c) and if Include/internal/ is in the header search path or not. I propose to:
For example, Include/internal/pystate.h becomes Include/pycore/pycore_pystate.h. Attached PR implements this idea. I chose to rename "internal" subdirectory to "pycore" to prepare the addition of other subdirectories. See: Next steps:
This change should be backward compatible since Include/internal/ must not be used outside CPython core. If someone does that, well, be ready for breakage :-) It's not supported. |
vstinner
added
3.8
|
I decided to work on this issue while trying to convert the _PyObject_GC_TRACK() macro into a "static inline" function. Currently, the macro uses _PyGC_generation0 which is defined earlier by "extern PyGC_Head *_PyGC_generation0;". Problem: _PyGC_generation0 no longer exists... Include/internal/mem.h now defines: "#define _PyGC_generation0 _PyRuntime.gc.generation0". Include/internal/mem.h includes Include/objimpl.h, but Include/objimpl.h requires Include/internal/mem.h. The include order matters here, many header files are inter-dependent, and have two header files with the same name in Include/ and Include/internal/ causes issues depending where the #include is done... My PR renames mem.h to pycore_objimpl.h and include pycore_objimpl.h at "the right place" in objimpl.h. Since objimpl.h controls where pycore_objimpl.h is imported, it's simpler to handle inter-dependencies simpler. Some inter-dependencies issues are hidden by the fact the C macros don't really require functions, macros and variables in the right order. They "just work". But converting C macros to proper "static inline" functions expose many issues. |
|
See also bpo-35059 which converts C macros to static inline functions. |
|
Discussion on python-dev: |
|
New changeset 31368a4 by Victor Stinner in branch 'master': |
|
Serhiy Storchaka asked: "Would not be better to move files with the content fully surrounded by #ifdef Py_BUILD_CORE out of the Include/ directory?" See his comment and my answer in the PR 10239: |
|
My changes move Py_BUILD_CORE to Include/internal/. I'm not sure of the effect on the backward compatibility. Since Python 3.7, many "Py_BUILD_CORE" functions rely on Include/internal/, like PyThreadState_GET() which uses _PyRuntime.gilstate.tstate_current. On my Fedora 28, the python3-devel package doesn't proide Include/internal/ headers, only Include/*.h in /usr/include/python3.7m/. It seems like Include/internal/ is not usable by 3rd party modules on Fedora at least. I understand that even if a 3rd party C extension used the Py_BUILD_CORE API, Python 3.7 already broke these extensions. I don't want C extensions to use Py_BUILD_CORE: Py_BUILD_CORE API is really designed to only be used inside Python. If this API is used outside Python, we cannot modify the API anymore since it would break extensions. But I want to make sure that we can break this API for different reasons. In Python 3.7, pyatomic.h is included by Python.h. In Python 3.7.0, pyatomic.h content wasn't surrounded by Py_BUILD_CORE and this header file caused multiple compilation issues: see bpo-23644 and bpo-25150. The content is now restricted to Py_BUILD_CORE since Python 3.7.1. It allows us to more easily change the implementation. |
|
Include/internal/pystate.h uses #include "pystate.h" to include Include/pystate.h, but it tries to include itself (Include/internal/pystate.h) which does nothing because of "#ifndef Py_INTERNAL_PYSTATE_H #define Py_INTERNAL_PYSTATE_H ... #endif". Remove the #ifndef #define to see the bug: diff --git a/Include/internal/pystate.h b/Include/internal/pystate.h
index 38845d32ec..2ef023a9a5 100644
--- a/Include/internal/pystate.h
+++ b/Include/internal/pystate.h
@@ -1,5 +1,3 @@
-#ifndef Py_INTERNAL_PYSTATE_H
-#define Py_INTERNAL_PYSTATE_H
#ifdef __cplusplus
extern "C" {
#endif
@@ -222,4 +220,3 @@ PyAPI_FUNC(void) _PyInterpreterState_DeleteExceptMain(void);
#ifdef __cplusplus
}
#endif
-#endif /* !Py_INTERNAL_PYSTATE_H */Compilation fails with: In file included from ./Include/internal/pystate.h:5,
from ./Include/internal/pystate.h:5,
from ./Include/internal/pystate.h:5,
from ./Include/internal/pystate.h:5,
from ./Include/internal/pystate.h:5,
from ./Include/internal/pystate.h:5,
from ./Include/internal/pystate.h:5,
...
./Include/internal/pystate.h:5:21: error: #include nested too deeply
#include "pystate.h" |
I proposed to rename internal header files, add an "internal_" prefix, to avoid this issue: PR 10263. |
|
New changeset e281f7d by Victor Stinner in branch 'master': |
|
Copy of my comment on PR 10271: I tried to enforce to require Py_BUILD_CORE in pycore_accu.h to be defined using: #ifndef Py_BUILD_CORE
# error "Py_BUILD_CORE must be defined to include this header"
#endifBut the compilation of the _json module failed, because it isn't compiled with Py_BUILD_CORE. Moreover, _json.c contains: /* Core extension modules are built-in on some platforms (e.g. Windows). */
#ifdef Py_BUILD_CORE
#define Py_BUILD_CORE_BUILTIN
#undef Py_BUILD_CORE
#endif |
vstinner
changed the title
I tried to add the Py_BUILD_CORE guard in pycore_pathconfig.h: +#ifndef Py_BUILD_CORE But it breaks the compilation of _testcapimodule.c: get_coreconfig() uses _Py_wstrlist_as_pylist(), and _Py_wstrlist_as_pylist() is defined in pycore_pathconfig.h. IMHO _testcapi should be compiled with Py_BUILD_CORE defined. I wrote PR 10274 but then _testcapi fails because of datetime.h: this bug should be fixed by PR 10238. |
|
I created bpo-35134: Move !Py_LIMITED_API to Include/pycapi/. |
|
Please don't change Include/datetime.h without consulting with original authors of this code (see bpo-876130). |
FYI I tried to be very careful on each change to never modify the *public* C API. But I modified the Py_BUILD_CORE API. This is fine since this API can change anytime, and it must not be used outside Python. |
|
Victor, you moved declarations of some functions to other headers, but didn't include the new headers into files that implement the functions in some cases. For example, _PyGILState_Init was moved into Include/internal/pycore_lifecycle.h in a1c249c, but it's implemented in Python/pystate.c, which doesn't include the new header. This may lead to subtle problems because the compiler can't check that signatures of the declaration and the implementation match. I suggest to use -Wmissing-prototypes and -Wmissing-declarations to detect such situations: ../../cpython/Python/pystate.c: At top level: Sadly, there are many other similar issues in Python now, but you can at least compare the number of warnings before and after your changes. |
Oh, I never saw this warning before. It seems to not be included in -Wall. Would you mind to open a new issue to discuss it? I will try to fix the regressions that I introduced, but I'm interested by a more general discussion on this issue. |
|
Status: only pyport. and tupleobject.h use Py_BUILD_CORE, and only tupleobject.h uses "#ifdef Py_BUILD_CORE" (contains code specific to internals). $ grep Py_BUILD_CORE Include/*.h
Include/pyport.h:# if defined(Py_BUILD_CORE) || defined(Py_BUILD_CORE_BUILTIN)
Include/pyport.h:# else /* Py_BUILD_CORE */
Include/pyport.h:# endif /* Py_BUILD_CORE */
Include/pyport.h:#if defined(Py_BUILD_CORE) || defined(Py_BUILD_CORE_BUILTIN)
Include/pyport.h:#endif /* Py_BUILD_CORE */
Include/tupleobject.h:#ifdef Py_BUILD_COREtupleobject.c: #ifdef Py_BUILD_CORE
# define _PyTuple_ITEMS(op) ((((PyTupleObject *)(op))->ob_item))
#endifI added this macro in bpo-35199, to prepare the C code for a new C API hiding implementation details: see bpo-35206. |
|
Hum, this issue is much harder than what I expected. Status:
TODO:
|
Victor, I've opened bpo-35258 as you suggested. |
|
Making _PyGC_FINALIZED() internal broke Cython (cython/cython#2721). It's used in the finaliser implementation (https://github.com/cython/cython/blob/da657c8e326a419cde8ae6ea91be9661b9622504/Cython/Compiler/ModuleNode.py#L1442-L1456), to determine if an object for which tp_dealloc() is called has already been finalised or whether we have to do it. I'm not sure how to deal with this on our side now. Any clue? |
Aha, interesting. Would you mind to open a new dedicated issue? So we can discuss how much of the GC details we want to leak into the API :-) |
|
TODO: add a NEWS entry for all these changes. Copy of Nick Coghlan's comment: @vstinner For folks consuming source archives (Linux distros, anyone embedding Python in a larger application, etc) rather than prebuilt binaries, the build process is something that can break, even for Py_BUILD_CORE only changes. For those folks, when they pull a new source archive, their builds may break, especially if they're applying additional downstream patches the way Linux distros do. The NEWS entry for this header file rearrangement should go in the Build section (https://github.com/python/cpython/tree/master/Misc/NEWS.d/next/Build) rather than the C API section (since these headers aren't part of the public API at all), but it should still exist. |
|
Stefan Behnel:
I wrote PR 10626 to add _PyGC_FINALIZED() back to the C API. My intent was only to remove _PyObject_GC_TRACK(o) and _PyObject_GC_UNTRACK(o) from the public C API. I didn't expect that anyone would use _PyGC_FINALIZED() :-) |
|
Please review PR 10624 of bpo-35134: Create Include/unstable/ subdirectory. It's the second part of my plan :-) |
|
New changeset bcda8f1 by Victor Stinner in branch 'master': |
|
New changeset 4ac5328 by Victor Stinner in branch 'master': |
|
I close the issue, it seems like all sub-tasks have been completed! Summary of the change:
There are other issues to track following steps:
|
|
In bpo-35296, I modified "make install" to also install Include/internal/. |
|
New changeset ec13b93 by Victor Stinner in branch 'master': |
|
I reopen the issue since there is still activity on it. |
|
New changeset 063abd9 by Victor Stinner in branch 'main': |
|
On Fri, Oct 15, 2021 at 3:56 AM STINNER Victor <report@bugs.python.org> wrote:
FYI, _xxsubinterpretersmodule is supposed to be an extension module |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: