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

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

Merged
merged 3 commits into from
Jan 14, 2025

Conversation

tornaria
Copy link
Contributor

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
Contributor 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
Copy link
Member

dimpase commented Jan 14, 2025

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
Contributor 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
Copy link
Member

dimpase commented Jan 14, 2025

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

@dimpase
Copy link
Member

dimpase commented Jan 14, 2025

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
Copy link
Member

dimpase commented Jan 14, 2025

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
23 of 24 checks passed
@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