Skip to content

Consider bumping C standard in meson.build from C99 to C17 #28977

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.

Sign up for GitHub

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

Jump to bottom
Closed
ngoldbaum opened this issue May 8, 2024 · 2 comments
Closed

Consider bumping C standard in meson.build from C99 to C17 #28977

ngoldbaum opened this issue May 8, 2024 · 2 comments
Labels
Bug free-threading PRs and issues related to support for free-threaded CPython (a.k.a. nogil or no-GIL, PEP 703)

Comments

@ngoldbaum
Copy link
Contributor

Describe the bug

Currently, trying to build scikit-learn with the python 3.13 free-threaded build leads to a compilation error related to usage of static_assert in CPython internals. This leaks into public code via cython's adding #include "internal/pycore_frame.h" to module init code.

See scipy/scipy#20515 where scipy made a similar change for similar reasons.

C17 is well-supported by downstream compilers, including MSVC. CPython itself is built with C11, which is a superset of C17.

Opening this as an issue instead of just making a pull request to see if there are good reasons besides inertia why meson.build specifies C99.

Steps/Code to Reproduce

python -m pip install -v . --no-build-isolation

Expected Results

successful build

Actual Results

  FAILED: sklearn/_loss/_loss.cpython-313t-darwin.so.p/meson-generated_sklearn__loss__loss.pyx.c.o
  ccache cc -Isklearn/_loss/_loss.cpython-313t-darwin.so.p -Isklearn/_loss -I../sklearn/_loss -I/Users/goldbaum/.pyenv/versions/3.13-dev-nogil/include/python3.13t -fvisibility=hidden -fdiagnostics-color=always -Wall -Winvalid-pch -std=c99 -O0 -g -Wno-unused-but-set-variable -Wno-unused-function -Wno-conversion -Wno-misleading-indentation -MD -MQ sklearn/_loss/_loss.cpython-313t-darwin.so.p/meson-generated_sklearn__loss__loss.pyx.c.o -MF sklearn/_loss/_loss.cpython-313t-darwin.so.p/meson-generated_sklearn__loss__loss.pyx.c.o.d -o sklearn/_loss/_loss.cpython-313t-darwin.so.p/meson-generated_sklearn__loss__loss.pyx.c.o -c sklearn/_loss/_loss.cpython-313t-darwin.so.p/sklearn/_loss/_loss.pyx.c
  In file included from sklearn/_loss/_loss.cpython-313t-darwin.so.p/sklearn/_loss/_loss.pyx.c:174712:
  In file included from /Users/goldbaum/.pyenv/versions/3.13-dev-nogil/include/python3.13t/internal/pycore_frame.h:13:
  /Users/goldbaum/.pyenv/versions/3.13-dev-nogil/include/python3.13t/internal/pycore_code.h:493:15: error: expected parameter declarator
  static_assert(COLD_EXIT_INITIAL_VALUE > ADAPTIVE_COOLDOWN_VALUE,
                ^
  /Users/goldbaum/.pyenv/versions/3.13-dev-nogil/include/python3.13t/internal/pycore_backoff.h:113:33: note: expanded from macro 'COLD_EXIT_INITIAL_VALUE'
  #define COLD_EXIT_INITIAL_VALUE 64
                                  ^
  In file included from sklearn/_loss/_loss.cpython-313t-darwin.so.p/sklearn/_loss/_loss.pyx.c:174712:
  In file included from /Users/goldbaum/.pyenv/versions/3.13-dev-nogil/include/python3.13t/internal/pycore_frame.h:13:
  /Users/goldbaum/.pyenv/versions/3.13-dev-nogil/include/python3.13t/internal/pycore_code.h:493:15: error: expected ')'
  /Users/goldbaum/.pyenv/versions/3.13-dev-nogil/include/python3.13t/internal/pycore_backoff.h:113:33: note: expanded from macro 'COLD_EXIT_INITIAL_VALUE'
  #define COLD_EXIT_INITIAL_VALUE 64
                                  ^
  /Users/goldbaum/.pyenv/versions/3.13-dev-nogil/include/python3.13t/internal/pycore_code.h:493:14: note: to match this '('
  static_assert(COLD_EXIT_INITIAL_VALUE > ADAPTIVE_COOLDOWN_VALUE,
               ^
  /Users/goldbaum/.pyenv/versions/3.13-dev-nogil/include/python3.13t/internal/pycore_code.h:493:1: warning: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
  static_assert(COLD_EXIT_INITIAL_VALUE > ADAPTIVE_COOLDOWN_VALUE,
  ^
  int

Versions

Current `main` branch.
@ngoldbaum ngoldbaum added Bug Needs Triage Issue requires triage labels May 8, 2024
@adrinjalali
Copy link
Member

But seeing C99 makes me happy cause I grew up with it 😅

Otherwise, we have a bunch of code which we don't really maintain much (they're basically vendored). If bumping doesn't break those, I don't see why we wouldn't.

I think opening PR to check if things would move smoothly would be welcomed :)

@ngoldbaum ngoldbaum mentioned this issue May 8, 2024
Merged
@lesteve lesteve added the free-threading PRs and issues related to support for free-threaded CPython (a.k.a. nogil or no-GIL, PEP 703) label May 13, 2024
@glemaitre
Copy link
Member

I assume that we can close this issue since #28980 has been merged, isn't it?

@glemaitre glemaitre removed the Needs Triage Issue requires triage label May 13, 2024
@ogrisel ogrisel mentioned this issue May 14, 2024
Closed
@lesteve lesteve mentioned this issue Jul 15, 2024
Closed
@tylerjereddy tylerjereddy mentioned this issue Aug 5, 2024
Closed
tylerjereddy added a commit to tylerjereddy/mdanalysis that referenced this issue Aug 5, 2024
@tylerjereddy
BLD: use C11 standard
8466710 
* Fixes MDAnalysisgh-4651

* The reason that bumping to the C11 standard for the C language
helps us build with free-threaded CPython `3.13.0rc1` is described
at: scikit-learn/scikit-learn#28977
@tylerjereddy tylerjereddy mentioned this issue Aug 5, 2024
Merged
5 tasks
hmacdope pushed a commit to MDAnalysis/mdanalysis that referenced this issue Aug 6, 2024
@tylerjereddy
BLD: use C11 standard (#4652)
f6a2c29 
* Fixes gh-4651

* The reason that bumping to the C11 standard for the C language
helps us build with free-threaded CPython `3.13.0rc1` is described
at: scikit-learn/scikit-learn#28977
@tschoonj tschoonj mentioned this issue Sep 8, 2024
Closed
matthew-brett added a commit to nipy/nipy that referenced this issue Oct 5, 2024
@matthew-brett
BLD: update C standard
4d2f0c9 
May avoid build error on Python 3.13.  See:
scikit-learn/scikit-learn#28977
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug free-threading PRs and issues related to support for free-threaded CPython (a.k.a. nogil or no-GIL, PEP 703)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@adrinjalali @lesteve @ngoldbaum @glemaitre