CI: Check whether Python.h is included first in a file #20536
Conversation
DWesl
requested review from
larsoner,
andyfaff,
rgommers,
person142 and
steppi
as code owners
github-actions
bot
added
scipy.special
scipy.io
scipy.optimize
Build issues
That sounds sensible |
|
The Accelerate failure is on me. Should be resolved in gh-20542. |
|
Wait, that include order actually was important? I could change the script to find and allow headers that included no other headers. |
|
It was important, but it should not have been haha. My PR should make it so that it no longer matters. |
Based on script from scipy#20149. Doesn't know about Boost::Python yet. DEV: Print name of file check_python_h_first is checking DEV: Add numpy/ndarrayobject.h to list of Python-including headers One file includes this first.
BLD: Move python-including file earlier in io/_fast_matrix_market/src/_fmm_core.cpp BLD: Move stdlib header after python-including headers in optimize/_pava/pava_pybind.cpp BLD:BUG: Move python-including file to top of special/_special_ufuncs.cpp DEV: Move python-including file earlier in wrap_g77_abi.c Wasn't actually a problem, but this makes it easier for my script to check. DEV: Move python including file earlier in wrap_dummy_g77_abi.c Not actually a problem, but this makes it easier for my script to check. STY: Remove duplicate headers at start of special/sf_error.cc
Hopefully this reduces false-positives.
It does include python first, and this will note that.
The file including the header should have already included Python.h (and whatever file for the specific constructs, or the compilation would fail everywhere)
This can cause errors if the header has no guard, or just slow things a bit if it does have a guard. CI: Exclude boost_math and highs from header check. Entirely too many false positives. CI: Don't check fast_float It seems to be a bunch of headers concatenated, which naturally results in many duplicate includes.
This reverts commit bd118fa.
Not sure what `re.compile` is doing before the matching that takes as long as the regex
|
I couldn't reproduce the Accelerate job's build issue locally, so pushed a rebase on top of the now-merged gh-20542. |
Also add cxx/hxx extensions, and handle the resulting new false positives.
These got found by the new file types.
[skip ci] Co-authored-by: DWesl <22566757+DWesl@users.noreply.github.com>
[skip cirrus] [skip circle]
| @@ -2,7 +2,8 @@ | |||
| // Use of this source code is governed by the BSD 2-clause license found in the LICENSE.txt file. | |||
| // SPDX-License-Identifier: BSD-2-Clause | |||
|
|
|||
| #include <Python.h> | |||
There was a problem hiding this comment.
This doesn't look correct, since there is direct usage of Python C API in this file. The linter also seems happy with the original version of the file. Unless I'm missing something here, I think you can revert this?
There was a problem hiding this comment.
_fmm_core.hpp is included next, and includes Python.h first, which the checker script knows. Either way would work.
There was a problem hiding this comment.
I think the rationale for removing this where I kept the others was that I was the one to add this include, and recently.
There was a problem hiding this comment.
Ah okay, fair enough. I think it's technically more correct to keep it, but both work - whatever you prefer.
| @@ -1,3 +1,6 @@ | |||
| #include "ckdtree_decl.h" | |||
There was a problem hiding this comment.
These are all correct indeed - necessary because "ckdtree_decl.h" includes npy_common.h
| @@ -2,7 +2,8 @@ | |||
| // Use of this source code is governed by the BSD 2-clause license found in the LICENSE.txt file. | |||
| // SPDX-License-Identifier: BSD-2-Clause | |||
|
|
|||
| #include <Python.h> | |||
There was a problem hiding this comment.
I think the rationale for removing this where I kept the others was that I was the one to add this include, and recently.
[skip circle] [skip cirrus] Co-authored-by: DWesl <22566757+DWesl@users.noreply.github.com>
This makes some allowance for including headers that include python as well
Reference issue
#20149 proposed this CI job, #20491 motivated actually getting this working.
What does this implement/fix?
There is a script, adapted from #20149, to check whether a given file includes system headers before Python.h. This expands that to check headers before source files, and to check the source files for python constructs implying either an indirect path to Python.h or a new extension beyond numpy and pybind. This then adds a CI job to perform that check on the contents of the SciPy repository.
Additional information
I made a brief attempt to flag duplicate includes, but stopped when I noticed there were reasons for doing that (one header includes
sys/time.hon every platform but Windows, another includesmutexunder two orthogonal conditions).A few of the changes aren't necessary to include Python.h first, but do make it easier to automate the checking process.
A NumPy maintainer mentioned it is possible to do something similar with clang-format, but I don't know how to do that.