Skip to content
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

BUG: Include Python.h before system headers. #20505

Merged
merged 4 commits into from
Apr 19, 2024
Merged

Conversation

DWesl
Copy link
Contributor

@DWesl DWesl commented Apr 17, 2024

Reference issue

Closes #20491

What does this implement/fix?

Python.h needs to be included before any system header: _fmm_core.hpp does, but is not first.

Additional information

The script from #20149 would not catch this. I might need to add pybind11:: to the regex.

@github-actions github-actions bot added scipy.io C/C++ Items related to the internal C/C++ code base defect A clear bug or issue that prevents SciPy from being installed or used as expected labels Apr 17, 2024
@j-bowhay
Copy link
Member

cc @alugowski

@tylerjereddy tylerjereddy added this to the 1.14.0 milestone Apr 17, 2024
Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glancing at the includes at the top of fast_matrix_market/types.hpp this seems obviously correct since https://docs.python.org/3/c-api/intro.html#include-files clearly notes:

Since Python may define some pre-processor definitions which affect the standard headers on some systems, you must include Python.h before any standard headers are included.

Since the author of this C++ source actively helps us and has been pinged I'll leave it open for a bit.

@DWesl
Copy link
Contributor Author

DWesl commented Apr 17, 2024

Found a few more trying to get everything built on Cygwin.

@DWesl
Copy link
Contributor Author

DWesl commented Apr 18, 2024

DWesl#5 has a script to check for this and sets up a CI job to do so. I've dropped the most likely false positives, leaving one instance in highspy that has since been removed in the upstream repository. Should I add those changes to this PR, replace this PR with that one (possibly after telling it to ignore highspy), squash-merge that PR onto the branch for this one, or something else?

This includes all changes DWesl#7 needed to get SciPy installed and most tests passing on the 1.13 branch.

@rgommers
Copy link
Member

Thanks @DWesl! Could you please rebase?

Cc @steppi @izaid for visibility, since most of the issues were recently introduced in scipy.special. Please keep in mind that Python.h must always be included before any system headers.

@ev-br
Copy link
Member

ev-br commented Apr 19, 2024

Just bumped into this recently: clang-format reorders the includes, and puts Python.h somewhere in the middle. At least with the .clang-format file from scope/special/special

@izaid
Copy link
Contributor

izaid commented Apr 19, 2024

Just bumped into this recently: clang-format reorders the includes, and puts Python.h somewhere in the middle. At least with the .clang-format file from scope/special/special

Yep, unless there is a line break between Python.h and other includes. Then it doesn't get reordering.

@rgommers
Copy link
Member

Something is still unhappy:

[125/1443] Compiling C++ object scipy/special/_special_ufuncs.cp310-win_amd64.pyd.p/_special_ufuncs.cpp.obj
FAILED: scipy/special/_special_ufuncs.cp310-win_amd64.pyd.p/_special_ufuncs.cpp.obj 
"c++" "-Iscipy\special\_special_ufuncs.cp310-win_amd64.pyd.p" "-Iscipy\special" "-I..\scipy\special" "-Iscipy\_lib" "-I..\scipy\_lib" "-I..\scipy\_build_utils\src" "-IC:\hostedtoolcache\windows\Python\3.10.11\x64\lib\site-packages\numpy\core\include" "-IC:\hostedtoolcache\windows\Python\3.10.11\x64\Include" "-fvisibility=hidden" "-fvisibility-inlines-hidden" "-fdiagnostics-color=always" "-D_GLIBCXX_ASSERTIONS=1" "-D_FILE_OFFSET_BITS=64" "-Wall" "-Winvalid-pch" "-std=c++17" "-O2" "-g" "-D__USE_MINGW_ANSI_STDIO=1" "-DMS_WIN64=" "-DNPY_NO_DEPRECATED_API=NPY_1_9_API_VERSION" "-DSP_SPECFUN_ERROR" -MD -MQ scipy/special/_special_ufuncs.cp310-win_amd64.pyd.p/_special_ufuncs.cpp.obj -MF "scipy\special\_special_ufuncs.cp310-win_amd64.pyd.p\_special_ufuncs.cpp.obj.d" -o scipy/special/_special_ufuncs.cp310-win_amd64.pyd.p/_special_ufuncs.cpp.obj "-c" ../scipy/special/_special_ufuncs.cpp
In file included from ../scipy/special/_special_ufuncs.cpp:6:
../scipy/special/ufunc.h:19:13: error: redefinition of 'bool SpecFun_Initialize()'

@DWesl
Copy link
Contributor Author

DWesl commented Apr 19, 2024

Redefinition of function errors tend to mean a header gets executed twice. I removed the second include from the mentioned file and stuck a header guard in in case there's another.

The recent trend seems to be toward #pragma once as a header guard, rather than #ifndef SPECIAL_UFUNC_H/#define SPECIAL_UFUNC_H, correct? Does the style guide prefer one over the other?

@izaid
Copy link
Contributor

izaid commented Apr 19, 2024

Redefinition of function errors tend to mean a header gets executed twice. I removed the second include from the mentioned file and stuck a header guard in in case there's another.

The recent trend seems to be toward #pragma once as a header guard, rather than #ifndef SPECIAL_UFUNC_H/#define SPECIAL_UFUNC_H, correct? Does the style guide prefer one over the other?

Yes, that's correct. I don't know where ufunc.h is getting included twice, but if putting #pragma once at the top fixes it then great.

@DWesl
Copy link
Contributor Author

DWesl commented Apr 19, 2024

I don't know where ufunc.h is getting included twice, but if putting #pragma once at the top fixes it then great.

It was special/_special_ufuncs.cpp, but that's gone now. I could likely adapt the script in DWesl#5 to check for duplicate includes.

@charris
Copy link
Member

charris commented Apr 19, 2024

clang-format reorders the includes

NumPy .clang-format deals with putting the includes in the correct order. It is messy.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and CI is all green now - in it goes. Thanks @DWesl & reviewers.

@rgommers rgommers merged commit 561376c into scipy:main Apr 19, 2024
31 checks passed
@DWesl DWesl deleted the patch-3 branch April 19, 2024 17:22
@tylerjereddy tylerjereddy added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Apr 20, 2024
@tylerjereddy tylerjereddy removed this from the 1.14.0 milestone May 2, 2024
@tylerjereddy tylerjereddy added this to the 1.13.1 milestone May 2, 2024
@tylerjereddy tylerjereddy mentioned this pull request May 2, 2024
10 tasks
@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C/C++ Items related to the internal C/C++ code base defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Cannot find OpenBLAS on Cygwin
7 participants