-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MAINT Upgrade C standard to C17 again #29481
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
Conversation
See discussion on scikit-learn#29015 and scikit-learn#28980
9fadb4f
to
693a724
Compare
Apologies for the force push; I wanted to modify the commit message to include the wheel builds and see if CI is green. No more amendments here. |
I need to update the conda lock files to bump the Meson version there and fix the Azure Pipelines errors ( |
I bumped all the conda-related lockfiles with the command |
I found that the Docker command does not work on |
Thanks a lot for the PR, and great effort with the lock-files, even more though because lock-files are something that even scikit-learn maintainers are slightly afraid to touch sometimes 😉! I have to say I am a bit reluctant to require the very latest version of meson as I imagine it has downsides and I am not sure about the upsides but I'd love to hear about the latter if there are some. My understanding of #28977 (comment) was that CPython is compiled with c11 so I think we should be fine with c11 until there is a CPython version that is compiled with c17. The main downside I can think of is for Linux distribution packagers that may not have the very latest version of meson available or similarly people that use their system meson. For example Ubuntu 24.04 has Meson 1.3.2 according to this. Having said that I am actually not sure that they use their system meson to build the scikit-learn package, a quick look at Fedora and Ubuntu seems to indicate that they don't (i.e. there is no build dependency on meson)? For completeness, right now we require meson >= 1.1.0 (released April 2023). 1.5.0 has been released in July 2024. |
On build dependencies, we haven't cared much about system versions. For instance, we have required very recent cython versions at times to fix issues in the repo. So I'm okay with requiring a very recent meson version here. |
Thanks for the inputs! Part of why I wish to, and the only upside in bumping the The associated page in the |
I added f7b6895 triggers the wheel builder and the Pyodide CI job; let's see if they pass. |
I guess my main question is "what do we gain by using c_std=c17" in As such I feel there is no strong advantage and there are some potential use cases that are going to be affected negatively (because we need to require meson>=1.5.0 for Pyodide) even if how much these use cases exist is somewhat uncertain and probably not that likely. |
Ah, I understand your question better now; thanks for the clarification, @lesteve. One straightforward benefit is that you can use the latest and greatest features across compiled code when writing it and aim to reduce technical debt, of course, and I've found that compiler versions in various Linux distros have support as well: Ubuntu 20.04 with GCC 9.4 + Clang 10; Ubuntu 22.04 with GCC 11.4 + Clang 14, Debian 10 with GCC 8.3 + Clang 7, Rocky Linux 8 with GCC 8.5 + Clang 16, NVIDIA Jetson Linux has GCC 9.3 (unsure about Clang there). Visual Studio 2019 (MSVC 16.8) and later support it for Windows. As for the downsides or use cases that can be affected, I know that there can be some across Python packaging land, such as a probable bump to the macOS deployment target to 10.14 from the current 10.9, but this too depends on how much of the language features you use, and is not a problem anyway because these macOS versions will be outdated very soon if they haven't been already. But other than that, I'm not sure – it does look like it's relevant for the Py3.13t free-threaded builds based on the issue description in gh-28977 🤔 I'll tag @ngoldbaum as the author of said issue, who would know of the advantages and impacts more and better than I do :) |
The reason this is related to free threading is technically a bug in Cython IMO but fixing it will be hard. Recently CPython started using This could be fixed by CPython using a different portable macro or by cython no longer including CPython internals. We've generally found the easiest thing to do is bump from C99 to C11 or C17. CPython itself uses C11 these days. |
Thanks @ngoldbaum for the details, this matched my understanding that we needed Line 9 in 379a2f1
So with my worried maintainer hat on, not doing anything for now still feels like the most cost effective option: "if it's not broken don't fix it". So far I haven't really seen any argument that using |
I agree with that. If c11 is enough for the free-threaded build I would stick with it, and reconsider later if it allows significant improvements in the existing code base or new features. |
OK let's close this one and revisit if/when there is a good reason to require Meson>=1.5 or requiring c17. And by the way looking back at this PR, the timeout in the no-OpenMP build with Meson 1.5 was already here, see #29546 for the ongoing investigation. Thanks anyway for the PR @agriyakhetarpal even if we did not end up merging it, really appreciated 🙏! |
No worries, thanks for all the context! I agree this should be done when C17 is required, and I'll be happy to contribute in other ways. |
Reference Issues/PRs
Please refer to earlier discussions around this area in gh-29015 and gh-28980.
What does this implement/fix? Explain your changes.
The C standard was previously bumped to C17 and later reverted to C11 to work around an issue related to Emscripten not recognising it. This should be fixed with the new release of Meson (version 1.5.0) now out, whose minimum version has been bumped as well in
meson.build
.Any other comments?
Here are some links for additional context: