-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
BLD: default to C17 rather than C99 #20515
Conversation
Okay, nothing is ever a one line diff it seems .... the Windows failure is real (only with numpy 1.23.5):
|
So f2py C17 support in numpy seems to need some work/testing, it seems |
I tested combinations of
numpy 1.26.x was the first minor version to be built with I have not yet checked if we can work around the problem somehow. |
The root cause is a bug in older versions of |
This update should be a safe bump, because C17 is a pure maintenance update of C11 with no new features; and C11 is mostly a maintenance/bugfix update of C99 with a few new features. Our [toolchain roadmap](http://scipy.github.io/devdocs/dev/toolchain.html#c-compilers), which was updated just days ago, says that we can use C17 given our minimum compiler versions. I also note that in numpy#25072, the numpy default was updated from C99 to C11 and that went fine (the one change needed was a Cython bug fix, which went into Cython 3.0.6 - and our minimum version is currently 3.0.8). Finally, another motivation to do this now is that under the `--disable-gil` build of CPython 3.13, Cython currently generates code that uses `static_assert`, which is a C11 feature. Hence our current default of `-std=c99` doesn't build with that nogil build.
…build] Due to numpy#24761, which we hit when building against numpy versions <1.26.1
181872c
to
88d5d55
Compare
Okay, this is all happy now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Ralf!
Over in scikit-learn, we hit an issue building under pyiodide after moving to C17: scikit-learn/scikit-learn#29013. From what I can tell, scipy currently doesn't build under pyodide (#15290) due to missing fortran support, so scipy doesn't have a similar issue. However, if pyodide doesn't grow C17 support before that happens, then scipy specifying C17 in Just a heads' up! |
Thanks for the heads up. Looking at getting that fixed in Meson (pyodide/pyodide#4762).
SciPy does build in Pyodide in-tree, it's at 1.12.0 right now with an update to 1.13.0 in progress. C17 support will become needed for 1.14.0 - we should have this fix included in a new Meson release by then. |
Fix in progress: mesonbuild/meson#13221 |
This update should be a safe bump, because C17 is a pure maintenance update of C11 with no new features; and C11 is mostly a maintenance/bugfix update of C99 with a few new features. Our toolchain roadmap, which was updated just days ago, says that we can use C17 given our minimum compiler versions.
I also note that in numpy#25072, the numpy default was updated from C99 to C11 and that went fine (the one change needed was a Cython bug fix, which went into Cython 3.0.6 - and our minimum version is currently 3.0.8).
Finally, another motivation to do this now is that under the
--disable-gil
build of CPython 3.13, Cython currently generates code that usesstatic_assert
, which is a C11 feature. Hence our current default of-std=c99
doesn't build with that nogil build. (hat tip to @ngoldbaum for pointing out this problem). EDIT: PEP 7 is also relevant here, since it specifies what C standard version is used by CPython (see https://peps.python.org/pep-0007/#c-dialect). To be safe, be should be using >= the version used by CPython.For a description of changes in C17, see https://gustedt.wordpress.com/2018/04/17/c17/.
Note: opening as draft until I've checked the CI logs for (the absence of) new warnings.