Skip to content

fix custom signal handling, which was broken after #166 - #216

Merged
dimpase merged 3 commits into
sagemath:mainfrom
tornaria:custom-signal
Jan 14, 2025
Merged

fix custom signal handling, which was broken after #166#216
dimpase merged 3 commits into
sagemath:mainfrom
tornaria:custom-signal

Conversation

@tornaria

Copy link
Copy Markdown
Member

There was a typo in #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 #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

Here's one way to trigger the bug without sagemath:

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

I added a test in tests/test_custom_signals.py that checks the bug is fixed.

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

Copy link
Copy Markdown
Member Author

@dimpase @tobiasdiez: do you know why the cygwin test is failing? It stops with

ModuleNotFoundError: No module named 'mesonpy'

but earlier

Successfully installed Cython-3.0.11 Pygments-2.19.1 Sphinx-7.4.7 alabaster-0.7.16 babel-2.16.0 build-1.2.2.post1 charset-normalizer-3.4.1 docutils-0.21.2 exceptiongroup-1.2.2 flake8-7.1.1 importlib-metadata-8.5.0 mccabe-0.7.0 meson-1.6.1 meson-python-0.17.1 pluggy-1.5.0 pycodestyle-2.12.1 pyflakes-3.2.0 pyproject-metadata-0.9.0 pyproject_hooks-1.2.0 pytest-8.3.4 sphinxcontrib-applehelp-2.0.0 sphinxcontrib-devhelp-2.0.0 sphinxcontrib-htmlhelp-2.1.0 sphinxcontrib-jsmath-1.0.1 sphinxcontrib-qthelp-2.0.0 sphinxcontrib-serializinghtml-2.0.0 tomli-2.2.1 zipp-3.21.0

says meson-python-0.17.1 is installed.

@dimpase

dimpase commented Jan 14, 2025

Copy link
Copy Markdown
Member

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

@tornaria

Copy link
Copy Markdown
Member Author

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:
https://github.com/sagemath/cysignals/actions/runs/12239853593/job/34569737654
https://github.com/sagemath/cysignals/actions/runs/12200523630/job/34469785701

@dimpase

dimpase commented Jan 14, 2025

Copy link
Copy Markdown
Member

tested this on the current Gentoo (applied https://github.com/sagemath/cysignals/pull/216.patch), all good. @orlitzky

@dimpase

dimpase commented Jan 14, 2025

Copy link
Copy Markdown
Member

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: https://github.com/sagemath/cysignals/actions/runs/12239853593/job/34569737654 https://github.com/sagemath/cysignals/actions/runs/12200523630/job/34469785701

That's from the pre-meson era.
Has it ever worked with meson? I suspect something is off with, like two different pythons at play, one a non-cygwin, the other cygwin.

@dimpase

dimpase commented Jan 14, 2025

Copy link
Copy Markdown
Member

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
https://github.com/sagemath/cysignals/actions/runs/12773544430/job/35605683764

@dimpase
dimpase merged commit ad3ecea into sagemath:main Jan 14, 2025
@tornaria
tornaria deleted the custom-signal branch January 14, 2025 23:45
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.

3 participants