[Python-Dev] Rename Include/internals/ to Include/pycore/

[Eric Snow]

On Sun, Oct 28, 2018 at 10:20 AM Victor Stinner <vstinner at redhat.com> wrote:
> Python C API has no strict separation between the 3 levels of API:
>
> * core: Py_BUILD_CORE define
> * stable: Py_LIMITED_API define (it has a value)
> * regular: no define
>
> IMHO the current design of header files is done backward: by default,
> everything is public. To exclude an API from core or stable, "#ifdef
> Py_BUILD_CORE" and "#ifndef Py_LIMITED_API" are used. This design
> caused issues in the past: functions, variables or something else
> exposed whereas they were supposed to be "private".

This is a great point.

> I propose a practical solution for that: Include/*.h files would only
> be be public API.

As we've already discussed, I'm entirely in favor of this. :)  In
fact, I was thinking along those same lines when I originally created
"Include/internal", when working on the runtime state consolidation.
When you originally shared your idea with me (2 years ago?) it made
perfect sense. :)

> The "core" API would live in a new subdirectory:
> Include/pycore/*.h.

I'm mostly -0 on this.  "pycore" is fine I suppose, but I think
"internal" is more obvious to people looking through the repo.  I can
imagine folks getting confused by having "core" in the directory name.

> Moreover, files of this subdirectory would have
> the prefix "pycore_". For example, Include/objimpl.h would have a
> twin: Include/pycore/pycore_objimpl.h which extend the public API with
> "core" APIs.

I'm not sure why this is necessary.  When I created Include/internal I
ran into some issues with relative includes.  However, I solved them
by doing the following (at Benjamin's recommendation, IIRC):  always
explicitly specify "internal/<...>.h" in includes, even in
Include/internal/*.h.

For example:

https://github.com/python/cpython/blob/master/Include/internal/pystate.h#L11
    #include "internal/ceval.h"
Note that a few lines above in that file I include the public header file:
    #include "pystate.h"

Taking this approach there were no issues with "relative" includes.
Incidentally, I originally tried to solve the problem of relative
includes with a prefix ("_" IIRC), but that proved to introduce other
issues.  Switching to using the explicit "internal/" worked great and
was more clear.

> I also propose to automatically load the twin: Include/objimpl.h would
> load Include/pycore/pycore_objimpl.h if Py_BUILD_CORE is defined:
>
> #ifdef Py_BUILD_CORE
> #  include "pycore/pycore_objimpl.h"
> #endif

During the runtime state consolidation I took a similar approach
initially, though at a less granular scale, which proved to be a
headache.  At first I added Include/internal/Python.h and then tried
to conditionally include it in Include/Python.h.  That ended up
confusing, problematic, and unnecessary.  At Benjamin's suggestion I
switched to explicitly including "internal/<...>.h" in .c files (only
where necessary).  The result is simpler and more clear, identifying
dependencies in source files more tightly.  It's also a bit more
idiomatic for well-written C code.

Here's an example of what I did:

https://github.com/python/cpython/blob/master/Python/ceval.c#L13
    #include "internal/pystate.h"

Your proposal is similar, though more granular.  And I suppose it is
reasonable as a short-term step for moving internal API out of the
public header files.  However, I agree with Benjamin that the ideal
would be to not have the public headers rely at all on the internal
ones.  I realize that I somewhat introduced the problem when I added
the internal headers, so I certainly don't have a lot of room to ask
anything here. :)  Regardless, an intermediate step of conditionally
including the private headers in the public ones might be okay if we
have a long-term plan on how to not include the private headers at
all.  I'd be interested in discussing that.

FWIW, in the existing internal headers I include some of the public
header files.  I don't recall if I did anything the other direction
(i.e. include internal headers conditionally in the public ones).

> Only in some rare cases, you would have to explicitly use: #include
> "pycore/pycore_pygetopt.h". This header is fully private, there is no
> public header in Include/pygetopt.h. Or maybe we should modify
> Include/Python.h to also include "pycore/pycore_pygetopt.h" if
> Py_BUILD_CORE is defined? Well, that's just a detail.

As noted above, I tried that and it didn't work out well.

> First milestone:
>
> * Create Include/pycore/

-0 (less clear)

> * Move Py_BUILD_CORE specific code into Include/pycore/pycore_*.h

+1

> * Automatically include pycore files from Include/*.h files (#ifdef Py_BUILD_CORE)

-0 (maybe okay as an intermediate fix)

> Second milestone:
>
> * Find a solution for Py_LIMITED_API :-)

+1 :)

-eric