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

Cython complaints about place of nogil #174

Closed
wants to merge 4 commits into from
Closed

Conversation

malb
Copy link
Member

@malb malb commented Aug 4, 2023

No description provided.

@malb
Copy link
Member Author

malb commented Aug 4, 2023

Those failures look more like a problem with the testing code than this PR?

malb added a commit to fplll/g6k that referenced this pull request Aug 10, 2023
We force 0.29.36 until CySignals is fixed: sagemath/cysignals#174
@malb malb reopened this Aug 10, 2023
@dimpase
Copy link
Member

dimpase commented Aug 16, 2023

oops, sorry, it needs more work. It's the usual Python packaging mess :-(

@dimpase
Copy link
Member

dimpase commented Aug 17, 2023

in https://github.com/sagemath/cysignals/actions/runs/5885338281/job/15961620534?pr=174
there is a compiler error, check it out (it's macOS with Python 3.11/clang)

@dimpase
Copy link
Member

dimpase commented Aug 17, 2023

On Ubuntu, it builds, but test fails:

Compiling cysignals_example.pyx because it changed.
[1/1] Cythonizing cysignals_example.pyx
running build_ext
building 'cysignals_example' extension
creating build
creating build/temp.linux-x86_64-cpython-311
gcc -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/runner/work/cysignals/cysignals/tmp/site-packages/cysignals -I/opt/hostedtoolcache/Python/3.11.4/x64/include/python3.11 -c cysignals_example.cpp -o build/temp.linux-x86_64-cpython-311/cysignals_example.o
cysignals_example.cpp:3657:14: warning: ‘long int* __pyx_f_17cysignals_example_safe_range_long(long int)’ defined but not used [-Wunused-function]
 3657 | static long *__pyx_f_17cysignals_example_safe_range_long(long __pyx_v_count) {
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/cysignals/pselect.pyx
creating build/lib.linux-x86_64-cpython-311
g++ -shared -Wl,--rpath=/opt/hostedtoolcache/Python/3.11.4/x64/lib -Wl,--rpath=/opt/hostedtoolcache/Python/3.11.4/x64/lib build/temp.linux-x86_64-cpython-311/cysignals_example.o -L/opt/hostedtoolcache/Python/3.11.4/x64/lib -o build/lib.linux-x86_64-cpython-311/cysignals_example.cpython-311-x86_64-linux-gnu.so
src/cysignals/pysignals.pyx
src/cysignals/signals.pyx
src/cysignals/tests.pyx
Doctest src/cysignals/tests.pyx terminated. Timeout limit exceeded (>600s)
make[1]: *** [Makefile:97: check-prefix-doctest] Error 1

@dimpase
Copy link
Member

dimpase commented Aug 18, 2023

@malb - also locally, make check-all fails with

...

export PYTHONUSERBASE="`pwd`/tmp/user" && python3 -B rundoctests.py src/cysignals/*.pyx
Doctesting 5 files.
...
src/cysignals/tests.pyx
**********************************************************************
File "src/cysignals/tests.pyx", line 958, in tests.pyx
Failed example:
    try:
        test_sig_occurred_live_exception()
    except RuntimeError:
        pass
Expected:
    RuntimeError: Aborted
Got nothing

@malb
Copy link
Member Author

malb commented Aug 18, 2023

Ah, right, so it's a bigger job :(

@dimpase
Copy link
Member

dimpase commented Aug 19, 2023

@malb - the main branch exhibits the same error in check-all.

@tornaria
Copy link
Contributor

Could you just merge the first commit and bump version? It seems very safe and it really solves a compatibility issue (I think cysignals built with 0.29 will work with cython 3, but with a warning).

It's possible to build cysignals with cython 3 using legacy_implicit_noexcept=True (see https://github.com/void-linux/void-packages/blob/master/srcpkgs/python3-cysignals/patches/cython3-legacy.patch). However that option won't work on cython 2. In order to support both cython 0.29 and cython 3 it is necessary to add explicit noexcept clauses everywhere it's needed.

@mkoeppe
Copy link

mkoeppe commented Sep 21, 2023

Let's close this as a duplicate of #176

@mkoeppe mkoeppe closed this Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants