-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
fix custom signal handling, which was broken after #166 #216
Conversation
This test triggers the following bug on cysignals 1.12.0 - 1.12.2: ``` Python 3.13.1 (main, Dec 14 2024, 12:44:03) [GCC 13.2.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> import cypari2 >>> from cysignals.alarm import alarm >>> try: ... alarm(0.5) ... while True: pass ... except: ... pass ... >>> cypari2.Pari() Traceback (most recent call last): File "<python-input-3>", line 1, in <module> cypari2.Pari() ~~~~~~~~~~~~^^ File "cypari2/pari_instance.pyx", line 471, in cypari2.pari_instance.Pari.__cinit__ File "cypari2/closure.pyx", line 138, in cypari2.closure._pari_init_closure cysignals.signals.AlarmInterrupt ```
There was a typo in sagemath#181: when the pari sigint handling was converted to a general mechanism, the line ``` PARI_SIGINT_pending = 0; ``` got translated into ``` custom_signal_unblock(); ``` instead of the correct ``` custom_set_pending_signal(0); ``` This error didn't take effect until sagemath#166 removed the pari sigint handling. This causes some doctest failures in sagemath: ``` src/sage/coding/linear_code.py src/sage/geometry/integral_points.pxi src/sage/rings/integer.pyx src/sage/rings/polynomial/polynomial_element.pyx ``` related to mishandling of AlarmInterrupt. See: https://github.com/sagemath/cysignals/pull/181/files#r1904885037
@dimpase @tobiasdiez: do you know why the cygwin test is failing? It stops with
but earlier
says |
I'm having a look, but cygwin is more dead than alive, IMHO. We should not worry too much about it. meson-python isn't even listed as a package here: https://cygwin.com/cgit/cygwin-infra/pkg-maint/plain/cygwin-pkg-maint |
Sure, but if it's broken then maybe it should be dropped from CI. Note it was working not so long ago: |
tested this on the current Gentoo (applied https://github.com/sagemath/cysignals/pull/216.patch), all good. @orlitzky |
That's from the pre-meson era. |
I created #218 to try to fix cygwin CI, basically by pip-installing the requirements.txt in the same place as building the package. It goes a bit further, but still fails quickly with a cryptic error in ninja, see |
There was a typo in #181: when the pari sigint handling was converted
to a general mechanism, the line
got translated into
instead of the correct
This error didn't take effect until #166 removed the pari sigint
handling.
This causes some doctest failures in sagemath:
related to mishandling of AlarmInterrupt.
See: https://github.com/sagemath/cysignals/pull/181/files#r1904885037
Here's one way to trigger the bug without sagemath:
I added a test in
tests/test_custom_signals.py
that checks the bug is fixed.