-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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.
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
bpo-35134: Create Include/cpython/ subdirectory #10624
Conversation
I rebased my change on top of master to retrieve PR #10626 bugfix. |
@ncoghlan, @serhiy-storchaka, @pitrou, @zooba, @ericsnowcurrently: Would you mind to review this PR? I created a new Include/unstable/ subdirectory for all "#ifndef Py_LIMITED_API" APIs. See https://bugs.python.org/issue35134 for the rationale. |
I plan to merge this PR next week. |
Include/unstable/objimpl.h
Outdated
#endif | ||
|
||
#ifndef _Py_UNSTABLE_OBJIMPL_H | ||
# error "this header file must not be included directly" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to quibble about the directory name, but given this style of inclusion (i.e. only implicitly via the public header file when Py_LIMITED_API
is not defined), I think 'unstable' is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly wrote this #error to prevent the risk of bikeshedding, LOL!
@ncoghlan: You approved the PR but proposed a different directory name. Are you ok with "unstable" name? https://bugs.python.org/issue35134#msg330261 |
Note: this PR is my second attempt, the first one was PR #10285 which wasn't properly designed. |
There's no real easy way to test it, but Better to make the change now and cause the installer build to break than to accidentally ship without necessary headers. If it doesn't work, I'll fix it up. |
@zooba: Aha, I was looking for the code of the Windows installer.
Right now, Include/internal/ is not installed (on Unix at least). I just proposed to install these headers as well: https://bugs.python.org/issue35296 Would it be possible to only update the MSI installer once this PR and PR #10665 are merged? |
I renamed the subdirectory from Include/unstable/ to Include/cpython/. |
@vstinner If you're willing to keep reminding me to do it once this is merged until it's done, sure. |
Include/*.h should be the "portable Python API", whereas Include/cpython/*.h should be the "CPython API": CPython implementation details. Changes: * Create Include/cpython/ subdirectory * "make install" now creates $prefix/include/cpython and copy Include/cpython/* to $prefix/include/cpython * Create Include/cpython/objimpl.h: move objimpl.h code surrounded by "#ifndef Py_LIMITED_API" to cpython/objimpl.h. * objimpl.h now includes cpython/objimpl.h * Windows installer (MSI) now also install Include/ subdirectories: Include/cpython/ and Include/internal/.
@vstinner While I preferred "cpython" as the directory name, I was OK with "unstable", hence the original approval. I'm happy you decided to switch the name anyway, though :) |
Include/.h should be the "portable Python API", whereas
Include/cpython/.h should be the "CPython API": CPython
implementation details.
Changes:
Include/cpython/* to $prefix/include/cpython
surrounded by "#ifndef Py_LIMITED_API" to cpython/objimpl.h.
https://bugs.python.org/issue35134