Skip to content

MAINT: Don't wrap #include <Python.h> with extern "C" - #27986

Merged
seberg merged 1 commit into
numpy:mainfrom
hawkinsp:externc
Dec 12, 2024
Merged

seberg merged 1 commit into
numpy:mainfrom
hawkinsp:externc

Conversation

@hawkinsp

Copy link
Copy Markdown
Contributor

This exposed a small latent bug in CPython
(python/cpython#127772), but it's probably not a good practice in general to wrap other code's headers with extern guards.

This extern block was recently moved, which exposed a latent bug in CPython
(python/cpython#127772), but it's probably not a
good practice in general to wrap other code's headers with extern
guards.
@hawkinsp

Copy link
Copy Markdown
Contributor Author

This was found to cause a build failure in Google's monorepo. It's unlikely to cause issues elsewhere, since you need to have a particular combination of includes from a C++ build to notice the problem. Still, we may as well fix it.

@seberg

seberg commented Dec 12, 2024

Copy link
Copy Markdown
Member

Harmless enough and I don't mind just merging. Is that really a best-practice -- considering Python is a C-API -- to rely on them having the extern wrapping?

@WarrenWeckesser

WarrenWeckesser commented Dec 12, 2024

Copy link
Copy Markdown
Member

Is that really a best-practice -- considering Python is a C-API -- to rely on them having the extern wrapping?

The Python C API docs say they include the appropriate extern "C" statements:

C++ users should note that although the API is defined entirely using C, the header files properly declare the entry points to be extern "C". As a result, there is no need to do anything special to use the API from C++.

@hawkinsp

hawkinsp commented Dec 12, 2024

Copy link
Copy Markdown
Contributor Author

Is that really a best-practice -- considering Python is a C-API -- to rely on them having the extern wrapping?

The Python C API docs say they include the appropriate extern "c" statements:

C++ users should note that although the API is defined entirely using C, the header files properly declare the entry points to be extern "C". As a result, there is no need to do anything special to use the API from C++.

(As it happens, they did not. But I sent a PR fixing that in CPython, see above.)

@seberg seberg changed the title [BUG] Don't wrap #include <Python.h> with extern "C". MAINT: Don't wrap #include <Python.h> with extern "C". Dec 12, 2024
@seberg seberg changed the title MAINT: Don't wrap #include <Python.h> with extern "C". MAINT: Don't wrap #include <Python.h> with extern "C" Dec 12, 2024
@seberg

seberg commented Dec 12, 2024

Copy link
Copy Markdown
Member

Not sure I am convinced yet that it is a fix, but since NumPy is happy, I don't mind just putting it in :).

Do you need a backport of this because the real fix in Python is slower to backport?

@seberg
seberg merged commit c1e8a0e into numpy:main Dec 12, 2024
@charris

charris commented Dec 12, 2024

Copy link
Copy Markdown
Member

Do you need a backport of this

Seems reasonable. NumPy 2.2 supports 3.10 - 3.13, that is a lot of Python releases to rely on.

@charris charris added 03 - Maintenance 09 - Backport-Candidate PRs tagged should be backported labels Dec 12, 2024
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants