classification
Title: Move internal headers to Include/internal/
Type: Stage: patch review
Components: Interpreter Core Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: atuining, eric.snow, izbyshev, lemburg, serhiy.storchaka, tim.peters, vstinner
Priority: normal Keywords: patch

Created on 2018-10-27 00:42 by vstinner, last changed 2018-11-13 11:52 by vstinner.

Pull Requests
URL Status Linked Edit
PR 10143 closed vstinner, 2018-10-27 00:44
PR 10238 closed vstinner, 2018-10-30 12:18
PR 10239 merged vstinner, 2018-10-30 12:22
PR 10240 merged vstinner, 2018-10-30 13:28
PR 10249 merged vstinner, 2018-10-30 19:31
PR 10263 merged vstinner, 2018-10-31 19:48
PR 10266 merged vstinner, 2018-10-31 21:19
PR 10271 merged vstinner, 2018-11-01 01:03
PR 10272 open vstinner, 2018-11-01 01:11
PR 10273 merged vstinner, 2018-11-01 01:42
PR 10274 closed vstinner, 2018-11-01 02:22
PR 10275 merged vstinner, 2018-11-01 02:38
PR 10276 open vstinner, 2018-11-01 02:52
PR 10277 closed vstinner, 2018-11-01 03:06
PR 10362 merged vstinner, 2018-11-06 14:26
PR 10363 merged vstinner, 2018-11-06 15:11
PR 10371 merged vstinner, 2018-11-06 22:00
PR 10416 merged p-ganssle, 2018-11-08 15:47
PR 10507 merged vstinner, 2018-11-13 08:52
Messages (30)
msg328619 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-27 00:42
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:

* Rename Include/internal/ to Include/pycore/
* In this subdirectory, rename xxx.h to pycore_xxx.h to avoid any risk of confusion
* Automatically include pycore_xxx.h in xxx.h if Py_BUILD_CORE is defined: this should avoid the need of explicit #include "internal/xxx.h" in C files

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:

* https://pythoncapi.readthedocs.io/split_include.html
* https://pythoncapi.readthedocs.io/

Next steps:

* Move all code surrounded by #ifdef Py_BUILD_CORE from Include/*.h into Include/pycore/*.h: see the second commit of my PR for an example
* Move core surrounded by #ifndef Py_LIMITED_API from Include/*.h into Include/limited/ (example of filename: Include/limited/limited_object.h)


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.
msg328620 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-27 00:50
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.
msg328623 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-27 00:50
See also bpo-35059 which converts C macros to static inline functions.
msg328854 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-29 19:30
Discussion on python-dev:
https://mail.python.org/pipermail/python-dev/2018-October/155587.html
msg328920 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-30 14:13
New changeset 9204fb8623896cc5f68ae350784ee25e8a7b2184 by Victor Stinner in branch 'master':
bpo-35081: Cleanup pystate.c and pystate.h (GH-10240)
https://github.com/python/cpython/commit/9204fb8623896cc5f68ae350784ee25e8a7b2184
msg328921 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-30 14:14
New changeset 31368a4f0e531c19affe2a1becd25fc316bc7501 by Victor Stinner in branch 'master':
bpo-35081: Move Include/pyatomic.c to Include/internal/ (GH-10239)
https://github.com/python/cpython/commit/31368a4f0e531c19affe2a1becd25fc316bc7501
msg328922 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-30 14:15
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:
https://github.com/python/cpython/pull/10239#issuecomment-434289361
msg328923 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-30 14:31
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.
msg328942 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-30 19:59
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"
msg329006 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-31 19:19
New changeset 2be00d987d37682a55db67c298e82c405d01b868 by Victor Stinner in branch 'master':
bpo-35081: Move Py_BUILD_CORE code to internal/mem.h (GH-10249)
https://github.com/python/cpython/commit/2be00d987d37682a55db67c298e82c405d01b868
msg329009 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-31 19:50
> 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".

I proposed to rename internal header files, add an "internal_" prefix, to avoid this issue: PR 10263.
msg329026 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-31 23:52
New changeset 27e2d1f21975dfb8c0ddcb192fa0f45a51b7977e by Victor Stinner in branch 'master':
bpo-35081: Add pycore_ prefix to internal header files (GH-10263)
https://github.com/python/cpython/commit/27e2d1f21975dfb8c0ddcb192fa0f45a51b7977e
msg329029 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-01 00:51
New changeset 50b48572d9a90c5bb36e2bef6179548ea927a35a by Victor Stinner in branch 'master':
bpo-35081: Add _PyThreadState_GET() internal macro (GH-10266)
https://github.com/python/cpython/commit/50b48572d9a90c5bb36e2bef6179548ea927a35a
msg329030 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-01 01:30
New changeset e281f7d80ce2584a7e6a36acffb5a9cd796a0fe2 by Victor Stinner in branch 'master':
bpo-35081: Move accu.h to Include/internal/pycore_accu.h (GH-10271)
https://github.com/python/cpython/commit/e281f7d80ce2584a7e6a36acffb5a9cd796a0fe2
msg329031 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-01 01:31
Copy of my comment on PR 10271:
https://github.com/python/cpython/pull/10271#issuecomment-434897408

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"
#endif

But 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
msg329035 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-01 02:16
New changeset a1c249c40517917d2e0971d55aea8d14a44b2cc8 by Victor Stinner in branch 'master':
bpo-35081: And pycore_lifecycle.h and pycore_pathconfig.h (GH-10273)
https://github.com/python/cpython/commit/a1c249c40517917d2e0971d55aea8d14a44b2cc8
msg329037 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-01 02:25
> bpo-35081: And pycore_lifecycle.h and pycore_pathconfig.h (GH-10273)

I tried to add the Py_BUILD_CORE guard in pycore_pathconfig.h:

+#ifndef Py_BUILD_CORE
+#  error "Py_BUILD_CORE must be defined to include this header"
+#endif

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.
msg329062 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-01 13:06
I created bpo-35134: Move !Py_LIMITED_API to Include/pycapi/.
msg329082 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-01 17:23
Please don't change Include/datetime.h without consulting with original authors of this code (see issue876130).
msg329086 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-01 18:24
> Please don't change Include/datetime.h without consulting with original authors of this code (see issue876130).

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.
msg329362 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-06 14:59
New changeset 5ed6995675b084fe583b71f96fdde4413bb2a77b by Victor Stinner in branch 'master':
bpo-35081: Add _PyCoreConfig_AsDict() (GH-10362)
https://github.com/python/cpython/commit/5ed6995675b084fe583b71f96fdde4413bb2a77b
msg329398 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-06 23:44
New changeset 9fc57a384825530635ef5ec093a31d864ea14f7c by Victor Stinner in branch 'master':
bpo-35081: Add pycore_fileutils.h (GH-10371)
https://github.com/python/cpython/commit/9fc57a384825530635ef5ec093a31d864ea14f7c
msg329509 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-09 12:03
New changeset 130893debfd97c70e3a89d9ba49892f53e6b9d79 by Victor Stinner in branch 'master':
bpo-35081: Internal headers require Py_BUILD_CORE (GH-10363)
https://github.com/python/cpython/commit/130893debfd97c70e3a89d9ba49892f53e6b9d79
msg329608 - (view) Author: Alexey Izbyshev (izbyshev) * Date: 2018-11-10 13:19
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 a1c249c40517917d2e0971d55aea8d14a44b2cc8, 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:
../../cpython/Python/pystate.c:968:1: warning: no previous prototype for ‘_PyGILState_Init’ [-Wmissing-prototypes]
 _PyGILState_Init(PyInterpreterState *i, PyThreadState *t)
 ^~~~~~~~~~~~~~~~
../../cpython/Python/pystate.c:988:1: warning: no previous prototype for ‘_PyGILState_Fini’ [-Wmissing-prototypes]
 _PyGILState_Fini(void)
 ^~~~~~~~~~~~~~~~

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.
msg329693 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-11 23:30
> Python/pystate.c:968:1: warning: no previous prototype for ‘_PyGILState_Init’ [-Wmissing-prototypes]
 _PyGILState_Init(PyInterpreterState *i, PyThreadState *t)
 ^~~~~~~~~~~~~~~~

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.
msg329739 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-12 15:53
New changeset 621cebe81b1e6c8de10425955bf532d31ee4df42 by Victor Stinner in branch 'master':
bpo-35081: Rename internal headers (GH-10275)
https://github.com/python/cpython/commit/621cebe81b1e6c8de10425955bf532d31ee4df42
msg329812 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-13 08:02
New changeset 0d12672b30b8c6c992bef7564581117ae83e11ad by Victor Stinner (Paul Ganssle) in branch 'master':
bpo-35081: Remove Py_BUILD_CORE from datetime.h (GH-10416)
https://github.com/python/cpython/commit/0d12672b30b8c6c992bef7564581117ae83e11ad
msg329814 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-13 08:07
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_CORE

tupleobject.c:

#ifdef Py_BUILD_CORE
#  define _PyTuple_ITEMS(op) ((((PyTupleObject *)(op))->ob_item))
#endif

I added this macro in bpo-35199, to prepare the C code for a new C API hiding implementation details: see bpo-35206.
msg329819 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-13 09:05
Hum, this issue is much harder than what I expected.

Status:

* Include/internal/ header files have been renamed to get a new "pycore_" prefix which avoids conflicts with Include/ header files. Example: "pycore_pystate.h" vs "pystate.h".
* pyatomic.c has been moved to Include/internal/ and it is no longer 
included in Python.h.
* Except of _PyTuple_ITEMS() and _PyObject_GC_TRACK(), all internal APIs have been moved to Include/internal/. Many new header files have been created, like pycore_lifecycle.h and pycore_pathconfig.h.

TODO:

* Move _PyObject_GC_TRACK() to Include/internal/
* msg329608 describes bugs like: pystate.c:968:1: warning: no previous prototype for ‘_PyGILState_Init’ [-Wmissing-prototypes]
* bpo-35134 now manages moving unstable API to a new separated Include/ subdirectory
* bpo-35059 converts macros to static inline functions
msg329830 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-13 11:52
New changeset 1a6be91e6fd65ce9cb88cbbbb193db7e92ec6076 by Victor Stinner in branch 'master':
bpo-35081: Make some _PyGC macros internal (GH-10507)
https://github.com/python/cpython/commit/1a6be91e6fd65ce9cb88cbbbb193db7e92ec6076
History
Date User Action Args
2018-11-13 11:52:21vstinnersetmessages: + msg329830
2018-11-13 09:05:00vstinnersetmessages: + msg329819
2018-11-13 08:52:27vstinnersetpull_requests: + pull_request9767
2018-11-13 08:07:49vstinnersetmessages: + msg329814
2018-11-13 08:02:38vstinnersetmessages: + msg329812
2018-11-12 15:53:43vstinnersetmessages: + msg329739
2018-11-11 23:30:30vstinnersetmessages: + msg329693
2018-11-10 13:19:14izbyshevsetnosy: + izbyshev
messages: + msg329608
2018-11-09 12:03:41vstinnersetmessages: + msg329509
2018-11-08 15:47:52p-gansslesetpull_requests: + pull_request9696
2018-11-06 23:44:06vstinnersetmessages: + msg329398
2018-11-06 22:00:05vstinnersetpull_requests: + pull_request9672
2018-11-06 15:11:02vstinnersetpull_requests: + pull_request9664
2018-11-06 14:59:57vstinnersetmessages: + msg329362
2018-11-06 14:26:29vstinnersetpull_requests: + pull_request9662
2018-11-01 18:24:19vstinnersetmessages: + msg329086
2018-11-01 17:23:54serhiy.storchakasetnosy: + atuining, serhiy.storchaka, lemburg, tim.peters
messages: + msg329082
2018-11-01 13:06:31vstinnersetmessages: + msg329062
2018-11-01 03:06:47vstinnersetpull_requests: + pull_request9588
2018-11-01 02:52:33vstinnersetpull_requests: + pull_request9587
2018-11-01 02:38:41vstinnersetpull_requests: + pull_request9586
2018-11-01 02:25:24vstinnersetmessages: + msg329037
2018-11-01 02:22:34vstinnersetpull_requests: + pull_request9585
2018-11-01 02:16:01vstinnersetmessages: + msg329035
2018-11-01 01:42:00vstinnersetpull_requests: + pull_request9584
2018-11-01 01:32:06vstinnersettitle: Rename Include/internals/ to Include/pycore/ -> Move internal headers to Include/internal/
2018-11-01 01:31:22vstinnersetmessages: + msg329031
2018-11-01 01:30:41vstinnersetmessages: + msg329030
2018-11-01 01:11:22vstinnersetpull_requests: + pull_request9583
2018-11-01 01:03:55vstinnersetpull_requests: + pull_request9582
2018-11-01 00:51:44vstinnersetmessages: + msg329029
2018-10-31 23:52:31vstinnersetmessages: + msg329026
2018-10-31 23:49:47eric.snowsetnosy: + eric.snow
2018-10-31 21:19:40vstinnersetpull_requests: + pull_request9577
2018-10-31 19:50:00vstinnersetmessages: + msg329009
2018-10-31 19:48:53vstinnersetpull_requests: + pull_request9574
2018-10-31 19:19:33vstinnersetmessages: + msg329006
2018-10-30 19:59:10vstinnersetmessages: + msg328942
2018-10-30 19:31:58vstinnersetpull_requests: + pull_request9563
2018-10-30 14:31:17vstinnersetmessages: + msg328923
2018-10-30 14:15:14vstinnersetmessages: + msg328922
2018-10-30 14:14:31vstinnersetmessages: + msg328921
2018-10-30 14:13:25vstinnersetmessages: + msg328920
2018-10-30 13:28:00vstinnersetpull_requests: + pull_request9553
2018-10-30 12:22:33vstinnersetpull_requests: + pull_request9552
2018-10-30 12:18:30vstinnersetpull_requests: + pull_request9551
2018-10-29 19:30:22vstinnersetmessages: + msg328854
2018-10-27 00:50:52vstinnersetmessages: + msg328623
2018-10-27 00:50:05vstinnersetmessages: + msg328620
2018-10-27 00:44:01vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request9470
2018-10-27 00:42:24vstinnercreate