BLD,Cygwin: Include Python.h first in various C++ files #27821
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Python.h needs to be included before any system includes. This is easiest if it is the first include. Interestingly, pybind11/pybind11.h does not include Python, possibly depending on downstream projects including Python.h beforehand.
Needs to be included before system headers to set visibility macros for pybind11
pybind11 assumes Python.h is included first.
|
Checked all files instead of going one-by-one using check_python_h_first.py adapted from scipy/scipy#18049. |
8000
Cygwin pytest recently updated, and only installs pytest-3.9
Cygwin pytest only installs pytest-3.9. I should possibly suggest using alternatives for that and pip.
tacaswell
reviewed
Feb 25, 2024
ksunden
approved these changes
Feb 28, 2024
3 tasks
|
I think we should just consider |
DWesl
commented
Mar 6, 2024
…ind11.h includes pybind11 seems decent about including Python.h before any system headers, once you chase three layers of includes in to see that.
tacaswell
approved these changes
Mar 6, 2024
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR summary
Python.h needs to be included before any system includes. This is easiest if it is the first include.
EDIT:
pybind11/pybind11.hincludes Python three headers in. There are two system headers included beforehand, if debugging with MSVC. I didn't know that when I made this, and can removePython.hfrom beforepybind11/pybind11.hif desired.Should fix the Cygwin problem noted in #27552
PR checklist