bpo-35081: Remove Py_BUILD_CORE from datetime.h

[pganssle]

This is a new version of @vstinner's PR #10238. Victor closed it because he was a bit busy to deal with the last minute details, but gave me permission over e-mail to pick it up and run with it.

I think it is a very valuable reorganization and I think it is the last include file that still has PY_BUILD_CORE.

This is the only change I have made since PR #10238:

diff --git a/Include/datetime.h b/Include/datetime.h
index 4da4ef0d8b..0a0efafa51 100644
--- a/Include/datetime.h
+++ b/Include/datetime.h
@@ -180,8 +180,8 @@ typedef struct {
 #define PyDateTime_CAPSULE_NAME "datetime.datetime_CAPI"
 
 
-/* When datetime.h is included from _datetimemodule.c,
-   the macros are defined in _datetimemodule.c. */
+/* This block is only used as part of the public API and should not be
+ * included in _datetimemodule.c, which does not use the C API capsule. */
 #ifndef _PY_DATETIME_IMPL
 /* Define global variable for the C API and a macro for setting it. */
 static PyDateTime_CAPI *PyDateTimeAPI = NULL;

I have also pre-squashed this because the commit message is an important part of it and I don't want that to get lost in a squash merge.

CC @abalkin @tim-one @serhiy-storchaka

https://bugs.python.org/issue35081

[vstinner]

I added "skip news": this change doesn't change the C API.

But you may mention that this change fix the header when datetime.h is included in a C file compiled with Py_BUILD_CORE.

[pganssle]

@vstinner Thanks! I added a news blurb before you did that, so I've just modified it to mention that it fixes a bug. Let me know if you think that mischaracterizes the bug that was fixed.

[serhiy-storchaka]

I still don't understand what issue this PR fixes. If I understood correctly, including datetime.h with Py_BUILD_CORE didn't worked in third-party extensions. And I think this is correct, because Py_BUILD_CORE is for use only in CPython internally. What did I miss?

[pganssle]

@serhiy-storchaka I also think that if I understand correctly, to the extent that this fixes a "bug", it's basically a bug in an unsupported workflow. I think the most likely scenario in which this bug would manifest is if some other module in CPython that does build with PY_BUILD_CORE were to start depending on datetime.h. This could happen if we add a separate module for time zones, for example. In that case we would see this problem.

But really all the _PY_DATETIME_IMPL macro definition is doing here is avoiding the unnecessary definition of a few symbols in _datetimemodules.c and making it more convenient to redefine the macros. The reason I think that this is a valuable addition is mainly because of the refactoring - these _datetimemodules.c-only macro re-definitions really should live in _datetimemodules.c. Having a section of _datetimemodules.c defined in datetime.h is weird. Ideally we would also not use _PY_DATETIME_IMPL, but the problem is the static PyDatetimeCAPI variable, which is unused in _datetimemodules.c. I see three ways forward for this:

1. Continue with _PY_DATETIME_IMPL, which loosens the coupling between _datetimemodules.c's implementation and datetime.h, but doesn't completely fix it.
2. Remove _PY_DATETIME_IMPL and use the #undef method to redefine the macros in _datetimemodules.c and just allow the unecessary variable to be defined in _datetimemodules.c.
3. Do 2, but remove the static keyword from PyDatetimeCAPI.

I think 3 could be a breaking change, so I'm not really in favor of that right now. I don't know what the consequences of 2 are - maybe that's a viable option?

There are also a number of hybrid approaches that we can take, like wrapping only the problematic PyDatetimeCAPI _PY_DATETIME_IMPL.

I think we should start with 1, though, because it's already done and it doesn't in any way preclude switching over to 2 or 3. It also avoids the problem (which may indeed be a bug) that something else in CPython that does build with PY_BUILD_CORE may start depending on datetime.h,

[vstinner]

LGTM... my review is unfair since I wrote the initial change :-)

I suggested to remove the NEWS entry. Well, the NEWS entry is good, but it might confuse people who are not used to CPython internals.

@abalkin: Would you mind to review this change?

[pganssle]

Should we remove the news blurb?

[pganssle]

I rebased against master and added a commit removing the news blurb. If you want to keep the news blurb, just revert that commit.

Whoever merges, please make sure the commit message from the first commit is retained.

[pganssle]

@vstinner I've updated the comments and moved the rest of the capsule-related macros under the #ifndef. Looks good?

[vstinner]

@vstinner I've updated the comments and moved the rest of the capsule-related macros under the #ifndef. Looks good?

/* [bpo-35081](https://bugs.python.org/issue35081): Defining this prevents including the C API capsule;
 * internal versions of the  Py*_Check macros which do not require
 * the capsule are defined below */

Ah yes, it seems like we finally reached the root issue! The problem is not "public" vs "private" API. The problem is specific to the "C API capsule". The comment now explains the root issue very well, much better than my first attempt.

[vstinner]

I removed "A future refactoring that obviates the need for _PY_DATETIME_IMPL would be welcome if it could be done in a backwards-compatible way." from the commit message. There is an known option to avoid the define: have 3 header files rather than one. Honestly, I don't think that it's worth it or that it would make the code easier to review.

[pganssle]

Yes, now that it's clear that the stuff under the macro is basically "everything that uses the capsule", it feels much neater to me than when it was "Just remove the unused variable to make gcc happy, and also remove all the macros that _datetimemodules.c happens to need..."

Thanks everyone!