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

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

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 :)

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

No branches or pull requests

4 participants