[PR PATCH] python3-prompt_toolkit: don't handle sigint

[PR PATCH] python3-prompt_toolkit: don't handle sigint

[tornaria]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

There is a new pull request by tornaria against master on the void-packages repository

https://github.com/tornaria/void-packages prompt_toolkit
https://github.com/void-linux/void-packages/pull/35730

python3-prompt_toolkit: don't handle sigint
Fixes #35712

See also: https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576

<!-- Uncomment relevant sections and delete options which are not applicable -->

#### Testing the changes
- I tested the changes in this PR: **YES**

<!--
#### New package
- This new package conforms to the [quality requirements](https://github.com/void-linux/void-packages/blob/master/Manual.md#quality-requirements): **YES**|**NO**
-->

<!-- Note: If the build is likely to take more than 2 hours, please [skip CI](https://github.com/void-linux/void-packages/blob/master/CONTRIBUTING.md#continuous-integration)
and test at least one native build and, if supported, at least one cross build.
Ignore this section if this PR is not skipping CI.
-->
<!-- 
#### Local build testing
- I built this PR locally for my native architecture, (ARCH-LIBC)
- I built this PR locally for these architectures (if supported. mark crossbuilds):
  - aarch64-musl
  - armv7l
  - armv6l-musl
-->


A patch file from https://github.com/void-linux/void-packages/pull/35730.patch is attached

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: github-pr-prompt_toolkit-35730.patch --]
[-- Type: text/x-diff, Size: 2352 bytes --]

From 3de3ead932d7944855488304610f8c2118e21e87 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gonzalo=20Tornar=C3=ADa?= <tornaria@cmat.edu.uy>
Date: Mon, 21 Feb 2022 10:52:16 -0300
Subject: [PATCH] python3-prompt_toolkit: don't handle sigint

Fixes #35712

See also: https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576
---
 .../patches/dont-handle-sigint.patch          | 27 +++++++++++++++++++
 srcpkgs/python3-prompt_toolkit/template       |  2 +-
 2 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 srcpkgs/python3-prompt_toolkit/patches/dont-handle-sigint.patch

diff --git a/srcpkgs/python3-prompt_toolkit/patches/dont-handle-sigint.patch b/srcpkgs/python3-prompt_toolkit/patches/dont-handle-sigint.patch
new file mode 100644
index 000000000000..60ce287a63d1
--- /dev/null
+++ b/srcpkgs/python3-prompt_toolkit/patches/dont-handle-sigint.patch
@@ -0,0 +1,27 @@
+This makes handle_sigint default to False
+
+See:
+ - https://github.com/void-linux/void-packages/issues/35712
+ - https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576
+ - https://trac.sagemath.org/ticket/33360#comment:3
+
+--- a/prompt_toolkit/application/application.py	2022-02-11 05:06:37.000000000 -0300
++++ b/prompt_toolkit/application/application.py	2022-02-21 10:43:13.828058001 -0300
+@@ -634,7 +634,7 @@
+         self,
+         pre_run: Optional[Callable[[], None]] = None,
+         set_exception_handler: bool = True,
+-        handle_sigint: bool = True,
++        handle_sigint: bool = False,
+         slow_callback_duration: float = 0.5,
+     ) -> _AppResult:
+         """
+@@ -859,7 +859,7 @@
+         self,
+         pre_run: Optional[Callable[[], None]] = None,
+         set_exception_handler: bool = True,
+-        handle_sigint: bool = True,
++        handle_sigint: bool = False,
+         in_thread: bool = False,
+     ) -> _AppResult:
+         """
diff --git a/srcpkgs/python3-prompt_toolkit/template b/srcpkgs/python3-prompt_toolkit/template
index 0d625dc249fa..26bcb33e82bc 100644
--- a/srcpkgs/python3-prompt_toolkit/template
+++ b/srcpkgs/python3-prompt_toolkit/template
@@ -1,7 +1,7 @@
 # Template file for 'python3-prompt_toolkit'
 pkgname=python3-prompt_toolkit
 version=3.0.28
-revision=1
+revision=2
 wrksrc="prompt_toolkit-${version}"
 build_style=python3-module
 hostmakedepends="python3-setuptools"

Re: python3-prompt_toolkit: don't handle sigint

[ahesford]

[-- Attachment #1: Type: text/plain, Size: 286 bytes --]

New comment by ahesford on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1046921213

Comment:
We at least need to wait until upstream comments on tyour proposed fix before adopting it here. Presumably they merged the change for a reason.

Re: python3-prompt_toolkit: don't handle sigint

[ahesford]

[-- Attachment #1: Type: text/plain, Size: 285 bytes --]

New comment by ahesford on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1046921213

Comment:
We at least need to wait until upstream comments on your proposed fix before adopting it here. Presumably they merged the change for a reason.

Re: python3-prompt_toolkit: don't handle sigint

[tornaria]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

New comment by tornaria on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1046944957

Comment:
> We at least need to wait until upstream comments on your proposed fix before adopting it here. Presumably they merged the change for a reason.

I understand, you can see their reasons here: https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1537

However let me stress that this breaks sagemath very badly. If I'm in the middle of a computation session and I happen to run a command that takes a very long time, then Ctrl-C is broken and I can't get back to my session (I can Ctrl-Z and kill but then I will loose the session -- a non-obvious workaround is to send SIGALRM instead of SIGTERM which allows to recover).

The alternative to this patch is to revert prompt_toolkit to 3.0.24 until upstream says something. Is there anything requiring a more recent version of prompt_toolkit?

Re: [PR PATCH] [Updated] python3-prompt_toolkit: don't handle sigint

[tornaria]

[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]

There is an updated pull request by tornaria against master on the void-packages repository

https://github.com/tornaria/void-packages prompt_toolkit
https://github.com/void-linux/void-packages/pull/35730

python3-prompt_toolkit: don't handle sigint
Fixes #35712

See also: https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576

<!-- Uncomment relevant sections and delete options which are not applicable -->

#### Testing the changes
- I tested the changes in this PR: **YES**

<!--
#### New package
- This new package conforms to the [quality requirements](https://github.com/void-linux/void-packages/blob/master/Manual.md#quality-requirements): **YES**|**NO**
-->

<!-- Note: If the build is likely to take more than 2 hours, please [skip CI](https://github.com/void-linux/void-packages/blob/master/CONTRIBUTING.md#continuous-integration)
and test at least one native build and, if supported, at least one cross build.
Ignore this section if this PR is not skipping CI.
-->
<!-- 
#### Local build testing
- I built this PR locally for my native architecture, (ARCH-LIBC)
- I built this PR locally for these architectures (if supported. mark crossbuilds):
  - aarch64-musl
  - armv7l
  - armv6l-musl
-->


A patch file from https://github.com/void-linux/void-packages/pull/35730.patch is attached

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: github-pr-prompt_toolkit-35730.patch --]
[-- Type: text/x-diff, Size: 3644 bytes --]

From 7de146d4a03fac4adae8c2e10397ab61274e9611 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gonzalo=20Tornar=C3=ADa?= <tornaria@cmat.edu.uy>
Date: Mon, 21 Feb 2022 10:52:16 -0300
Subject: [PATCH] python3-prompt_toolkit: restore sigint handlers

Fixes #35712

See also: https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576
---
 .../patches/restore-sigint-handler.patch      | 57 +++++++++++++++++++
 srcpkgs/python3-prompt_toolkit/template       |  2 +-
 2 files changed, 58 insertions(+), 1 deletion(-)
 create mode 100644 srcpkgs/python3-prompt_toolkit/patches/restore-sigint-handler.patch

diff --git a/srcpkgs/python3-prompt_toolkit/patches/restore-sigint-handler.patch b/srcpkgs/python3-prompt_toolkit/patches/restore-sigint-handler.patch
new file mode 100644
index 000000000000..9e0fbdbfe03f
--- /dev/null
+++ b/srcpkgs/python3-prompt_toolkit/patches/restore-sigint-handler.patch
@@ -0,0 +1,57 @@
+When setting a sigint handler, save and restore the previous handler,
+since add_signal_handler and remove_signal_handler don't.
+
+Use cysignals if available so that the os-level handlers (struct
+sigaction) are saved as well. If cysignals is not available, fallback
+to saving only the python handlers.
+
+See:
+ - https://github.com/void-linux/void-packages/issues/35712
+ - https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576
+ - https://trac.sagemath.org/ticket/33360#comment:3
+
+--- a/prompt_toolkit/application/application.py	2022-02-11 05:06:37.000000000 -0300
++++ b/prompt_toolkit/application/application.py	2022-02-22 15:31:02.248681343 -0300
+@@ -98,6 +98,10 @@
+ except ImportError:
+     import prompt_toolkit.eventloop.dummy_contextvars as contextvars  # type: ignore
+ 
++try:
++    from cysignals import pysignals
++except ImportError:
++    from . import dummy_pysignals as pysignals
+ 
+ __all__ = [
+     "Application",
+@@ -805,6 +809,9 @@
+                 loop = get_event_loop()
+ 
+                 if handle_sigint:
++                    # save sigint handlers
++                    _sigint = signal.getsignal(signal.SIGINT)
++                    _sigint_os = pysignals.getossignal(signal.SIGINT)
+                     loop.add_signal_handler(
+                         signal.SIGINT,
+                         lambda *_: loop.call_soon_threadsafe(
+@@ -843,6 +850,8 @@
+ 
+                     if handle_sigint:
+                         loop.remove_signal_handler(signal.SIGINT)
++                        # restore python and os-level signal handlers
++                        pysignals.setsignal(signal.SIGINT, _sigint, _sigint_os)
+ 
+                     # Reset slow_callback_duration.
+                     loop.slow_callback_duration = original_slow_callback_duration
+--- a/prompt_toolkit/application/dummy_pysignals.py	1969-12-31 21:00:00.000000000 -0300
++++ b/prompt_toolkit/application/dummy_pysignals.py	2022-02-22 15:31:26.432386296 -0300
+@@ -0,0 +1,10 @@
++# fallback in case cysignals is not available
++# these functions don't save the os level handlers
++
++import signal
++
++def getossignal(sig):
++    return None
++
++def setsignal(sig, action, osaction=None):
++    return signal.signal(sig, action)
diff --git a/srcpkgs/python3-prompt_toolkit/template b/srcpkgs/python3-prompt_toolkit/template
index 0d625dc249fa..26bcb33e82bc 100644
--- a/srcpkgs/python3-prompt_toolkit/template
+++ b/srcpkgs/python3-prompt_toolkit/template
@@ -1,7 +1,7 @@
 # Template file for 'python3-prompt_toolkit'
 pkgname=python3-prompt_toolkit
 version=3.0.28
-revision=1
+revision=2
 wrksrc="prompt_toolkit-${version}"
 build_style=python3-module
 hostmakedepends="python3-setuptools"

Re: python3-prompt_toolkit: don't handle sigint

[tornaria]

[-- Attachment #1: Type: text/plain, Size: 303 bytes --]

New comment by tornaria on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1050260116

Comment:
@ahesford  a different approach, explained here: https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576#issuecomment-1050257999. What do you think?

Re: python3-prompt_toolkit: don't handle sigint

[ahesford]

[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]

New comment by ahesford on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1050972894

Comment:
Between the two options, I suppose I favor just disabling the SIGINT handler by default. Adding `cysignals` awareness to `prompt_toolkit` seems too special-purpose a fix; most people will not be using `cysignals` to add low-level signal handlers. It seems to me that the breakage here is with `cysignals`, which doesn't seem to be properly overriding the existing handler when it injects its own.

What I would like to see from upstream are some guidance about the following questions:
1. Is disabling SIGINT handling by default, and having xonsh turn it on explicitly, the right course of action?
2. If not, should this be fixed in IPython by providing a configuration option that can override the default SIGINT handler?
3. Is this better addressed (or even addressable at all) in the signal-handling logic of `cysignals`?

If upstream doesn't respond soon, we can pick up the original patch in this PR.

Re: python3-prompt_toolkit: don't handle sigint

[tornaria]

[-- Attachment #1: Type: text/plain, Size: 4316 bytes --]

New comment by tornaria on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1051294639

Comment:
 > Between the two options, I suppose I favor just disabling the SIGINT handler by default. Adding `cysignals` awareness to `prompt_toolkit` seems too special-purpose a fix; most people will not be using `cysignals` to add low-level signal handlers.

My last patch is *not* adding `cysignals` awareness. On the contrary, the strategy is to save and restore the sigint handlers so that the signal state is preserved after each run of `prompt_toolkit` (instead of being reverted to the python default each time `prompt_toolkit` runs).

This is a generic fix that would work for any program that sets the sigint handler, either just the python sigint handler or the whole os-level sigint handler..


> It seems to me that the breakage here is with `cysignals`, which doesn't seem to be properly overriding the existing handler when it injects its own.

I don't think so.  The way `cysignals` works is, at import time install a sigint handler using `sigaction(2)` as well as a python sigint handler using `signal.signal()` from python stdlib, but it expects that these handlers are not overriden -- if they are `cysignals` will not work anymore.

The way `prompt_toolkit` implements the sigint handling is: each time the prompt is run, the sigint handler is added (using `loop.add_signal_handler()`) and when the prompt returns (i.e. the user hit enter to dispatch the command) the sigint handler is removed (using `loop.remove_signal_handler()`). However, "remove" means "set the handler back to python default" which disables `cysignals`.

Now `prompt_toolkit` only needs its own `sigint` handler while running (i.e. while editing the prompt); and `cysignals` only needs its own `sigint` handler while running extension code so it's not a problem that `prompt_toolkit` installs a custom `sigint` handler as long as it is later restored. There should be no conflict at all, and nothing there that is specific to cysignals.


The question is: how to save and restore the sigint handler? Python stdlib offers `signal.getsignal()` (to save) and `signal.signal()` (to restore), but these are NOT a complete solution as they only save and restore the python signal handlers -- the os-level signal handler will be reverted back to the standard python signal handler which is *not* good enough for `cysignals`.

How to save and restore the os-level signal handler? Using `sigaction(2)`, however that needs C code but `prompt_toolkit` is python only afaict. It turns out `cysignals` has a submodule called `cysignals.pysignals` which contains functions to save and restore os-level signal handlers so this seemed like the easiest solution. This adds no dependency on `cysignals`: if not available, only the python-level signal handlers will be saved and restored.


> What I would like to see from upstream are some guidance about the following questions:
> 
>     1. Is disabling SIGINT handling by default, and having xonsh turn it on explicitly, the right course of action?

I later convinced myself that this is not a good solution. In fact, ipython itself *also* crashes when receiving a `SIGINT`. Their fix also fixes ipython so it is a good one (except for not restoring the sigint handler which only affects python programs that change the sigint handler, e.g. those using `cysignals`).

>     2. If not, should this be fixed in IPython by providing a configuration option that can override the default SIGINT handler?

No, because ipython itself knows nothing at all about `cysignals` or `sigint` handlers, or any signal handler at all. IMHO the only proper solution is that `prompt_toolkit` restores the sigint handlers that it changes (for a good reason) to their previous values.

>     3. Is this better addressed (or even addressable at all) in the signal-handling logic of `cysignals`?

I don't think there's any way to fix the signal-handling logic of `cysignals`: if their sigint handler is removed then `cysignals` won't catch SIGINT at all, nothing in their logic will fix that.

> If upstream doesn't respond soon, we can pick up the original patch in this PR.

As I explained above, I think the second patch is better.

Re: python3-prompt_toolkit: don't handle sigint

[ahesford]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

New comment by ahesford on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1051312288

Comment:
I misunderstood when `prompt_toolkit` installed and restored the signal handler; it certainly is problematic that the handler is overridden and restored repeatedly.

The "`cysignals` awareness" that I mention is your dependence on the `cysignals` package to save and restore signals set with `sigaction`. It's not clear to me that upstream would accept an (optional) dependence on `cysignals` to manage this. Furthermore, if managing signals set with `sigaction` is the right thing to do, it shouldn't be done only if the user chooses to install `cysignals`. Thus, either `cysignals` should become a hard requirement of `prompt_toolkit` or it should have its own method to save and restore these signals.

We really need some guidance from upstream about whether they intend to care about this problem at all and, if so, how they intend to solve it. I don't want to maintain indefinitely a custom `prompt_toolkit` patch that pulls in `cysignals` to manage these signals. If upstream wants to depend on it, so be it. If they don't care about this problem at all, we'll need another solution.

Re: python3-prompt_toolkit: don't handle sigint

[tornaria]

[-- Attachment #1: Type: text/plain, Size: 3863 bytes --]

New comment by tornaria on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1053637263

Comment:
I agree that we should wait for comments for upstream, although meanwhile sagemath is broken in a very annoying way.

About your comment on the optional use of `cysignals` here's my take:
 - pure python offers a way to install signal handlers via `signal.signal()`, and so for a pure python program that installs a custom sigint handler, saving and restoring python-level signals is necessary and good enough (no need to use `sigaction`). For example:
```python
$ ipython
Python 3.10.2 (main, Jan 15 2022, 03:11:32) [GCC 10.2.1 20201203]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.0.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: def h(x,y): print("Our custom handler received a sigint", x, y); raise KeyboardInterrupt

In [2]: import signal

In [3]: signal.signal(signal.SIGINT, h)
Out[3]: <cyfunction python_check_interrupt at 0x7f12316bcba0>

In [4]: while True: pass
^COur custom handler received a sigint 2 <frame at 0x55c760ab21c0, file '<ipython-input-4-b16dc615ea65>', line 1, code <module>>
---------------------------------------------------------------------------
KeyboardInterrupt                         Traceback (most recent call last)
Input In [4], in <module>
----> 1 while True: pass

Input In [1], in h(x, y)
----> 1 def h(x,y): print("Our custom handler received a sigint", x, y); raise KeyboardInterrupt

KeyboardInterrupt: 

In [5]: 
```
The above snippet is broken with the version of `prompt_toolkit` that we ship in void, but it works with my patch (even without `cysignals`).
 - when a python program uses `sigaction` to install a custom signal handler, then yes, saving and restoring with `sigaction` is the only way to fix this issue. Unfortunately python doesn't offer any standard way to do that. The only package I know uses `sigaction` to change the sigint handler is `cysignals`. But if `cysignals` is being used to change the sigint handler, then for sure `cysignals` is available to save and restore so we are good!

> Furthermore, if managing signals set with `sigaction` is the right thing to do, it shouldn't be done only if the user chooses to install `cysignals`.


> Thus, either `cysignals` should become a hard requirement of `prompt_toolkit` or it should have its own method to save and restore these signals.

I don't think either is necessary and presumably `cysignals` supports less architectures than `prompt_toolkit` (indeed `sigaction` only makes sense on posix); having its own method requires C code, that would be very simple to copy from `cysignals.pysignals` except `prompt_toolkit` doesn't compile any C code so why make building it more complicated for no good reason).

In case another program or python package installs a custom sigint handler and suffers of this issue, we can figure out how to fix it (for instance, we make *that* package depend on python3-cysignals). Right now there aren't many packages in void that use prompt_toolkit. In any case, if they break because of this they are already broken, my fix won't make them more broken but possibly less broken.

> We really need some guidance from upstream about whether they intend to care about this problem at all and, if so, how they intend to solve it.

Sure.
 
> I don't want to maintain indefinitely a custom `prompt_toolkit` patch that pulls in `cysignals` to manage these signals. If upstream wants to depend on it, so be it. If they don't care about this problem at all, we'll need another solution.

I can help to maintain the patch, at least for the time being while the issue is (hopefully) resolved upstream, and I can help to figure out another solution if this one doesn't work in the end.

Re: python3-prompt_toolkit: don't handle sigint

[ahesford]

[-- Attachment #1: Type: text/plain, Size: 2911 bytes --]

New comment by ahesford on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1053693604

Comment:
I understand the isssue and how your patch fixes it. Unless upstream merges your proposed handler changes, I will not accept a Void patch that does the same. The two options I *will* accept before upstream makes a decision are:
1. Your original patch to change to change the default SIGINT handling behavior from `True` to `False`, effectively restoring prior behavior and leaving any custom handlers untouched; or
2. A modification of your proposal that drops the conditional `cysignals` dependency and simply saves and restores Python signals in the prompt_toolkit event loop. (This will *not* fix your custom SIGINT problem in C extensions and probably is probably worse than #1 from your perspective.)

Option #1 may cause IPython to crash when it receives SIGINT, but that is functionally no worse than a revert to a prior version of `prompt_toolkit` and I can live with it.

Whether `cysignals` supports fewer platforms than `prompt_toolkit` is irrelevant to the discussion of whether it should be a hard dependency of `prompt_toolkit`. Python dependencies can be specified per-platform, so `cysignals` can always be restricted to the platforms where it can be built. However, I suspect that the need to maintain (and test) a platform-specific dependence on `cysignals` may make upstream less likely to accept this approach. For the same reason, I imagine upstream may be disinclined to incorporate its own compiled extension to manage `sigaction`.

The optional dependency on `cysignals` is unattractive because there are many different ways to set handlers via `sigaction` with Python: by compiling a C extension directly, by doing it directly in a Cython module that may have broader purposes or by using some FFI like the built-in `ctypes`. The population of people who rely on `cysignals` is likely to be small; having `prompt_toolkit` take special branches in every iteration of the event loop on the off chance that an arbitrary package is installed does not seem to be the right approach and certainly doesn't cover these alternatives.

Upstream needs to decide whether they even care about this problem and, if so, how to solve it. They might
- Declare `cysignals` as the one true way to manage `sigaction` and add a hard dependency on platforms that can use it
- Implement their own approach to save/restore as they see fit
- Find some other means to save/restore signals
- Accept your conditional approach, which solves the `sigaction` problem for some subset of people who may have actually defined a custom handler
I'm not going to decide among these for upstream.

If you do PR your optional approach upstream, you should probably define a package option to declare the dependency so that the package can advertise the possibility.

Re: python3-prompt_toolkit: don't handle sigint

[ahesford]

[-- Attachment #1: Type: text/plain, Size: 2917 bytes --]

New comment by ahesford on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1053693604

Comment:
I understand the isssue and how your patch fixes it. Unless upstream merges your proposed handler changes, I will not accept a Void patch that does the same. The two options I *will* accept before upstream makes a decision are:
1. Your original patch to change to change the default SIGINT handling behavior from `True` to `False`, effectively restoring prior behavior and leaving any custom handlers untouched; or
2. A modification of your proposal that drops the conditional `cysignals` dependency and simply saves and restores Python signals in the prompt_toolkit event loop. (This will *not* fix your custom SIGINT problem in C extensions and probably is probably worse than Option 1 from your perspective.)

Option #1 may cause IPython to crash when it receives SIGINT, but that is functionally no worse than a revert to a prior version of `prompt_toolkit` and I can live with it.

Whether `cysignals` supports fewer platforms than `prompt_toolkit` is irrelevant to the discussion of whether it should be a hard dependency of `prompt_toolkit`. Python dependencies can be specified per-platform, so `cysignals` can always be restricted to the platforms where it can be built. However, I suspect that the need to maintain (and test) a platform-specific dependence on `cysignals` may make upstream less likely to accept this approach. For the same reason, I imagine upstream may be disinclined to incorporate its own compiled extension to manage `sigaction`.

The optional dependency on `cysignals` is unattractive because there are many different ways to set handlers via `sigaction` with Python: by compiling a C extension directly, by doing it directly in a Cython module that may have broader purposes or by using some FFI like the built-in `ctypes`. The population of people who rely on `cysignals` is likely to be small; having `prompt_toolkit` take special branches in every iteration of the event loop on the off chance that an arbitrary package is installed does not seem to be the right approach and certainly doesn't cover these alternatives.

Upstream needs to decide whether they even care about this problem and, if so, how to solve it. They might
- Declare `cysignals` as the one true way to manage `sigaction` and add a hard dependency on platforms that can use it
- Implement their own approach to save/restore as they see fit
- Find some other means to save/restore signals
- Accept your conditional approach, which solves the `sigaction` problem for some subset of people who may have actually defined a custom handler
I'm not going to decide among these for upstream.

If you do PR your optional approach upstream, you should probably define a package option to declare the dependency so that the package can advertise the possibility.

Re: python3-prompt_toolkit: don't handle sigint

[ahesford]

[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]

New comment by ahesford on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1053693604

Comment:
I understand the isssue and how your patch fixes it. Unless upstream merges your proposed handler changes, I will not accept a Void patch that does the same. The two options I *will* accept before upstream makes a decision are:
1. Your original patch to change to change the default SIGINT handling behavior from `True` to `False`, effectively restoring prior behavior and leaving any custom handlers untouched; or
2. A modification of your proposal that drops the conditional `cysignals` dependency and simply saves and restores Python signals in the prompt_toolkit event loop. (This will *not* fix your custom SIGINT problem in C extensions and probably is probably worse than Option 1 from your perspective.)

Option 1 may cause IPython to crash when it receives SIGINT, but that is functionally no worse than a revert to a prior version of `prompt_toolkit` and I can live with it.

Whether `cysignals` supports fewer platforms than `prompt_toolkit` is irrelevant to the discussion of whether it should be a hard dependency of `prompt_toolkit`. Python dependencies can be specified per-platform, so `cysignals` can always be restricted to the platforms where it can be built. However, I suspect that the need to maintain (and test) a platform-specific dependence on `cysignals` may make upstream less likely to accept this approach. For the same reason, I imagine upstream may be disinclined to incorporate its own compiled extension to manage `sigaction`.

The optional dependency on `cysignals` is unattractive because there are many different ways to set handlers via `sigaction` with Python: by compiling a C extension directly, by doing it directly in a Cython module that may have broader purposes or by using some FFI like the built-in `ctypes`. The population of people who rely on `cysignals` is likely to be small; having `prompt_toolkit` take special branches in every iteration of the event loop on the off chance that an arbitrary package is installed does not seem to be the right approach and certainly doesn't cover these alternatives.

Upstream needs to decide whether they even care about this problem and, if so, how to solve it. They might
- Declare `cysignals` as the one true way to manage `sigaction` and add a hard dependency on platforms that can use it
- Implement their own approach to save/restore as they see fit
- Find some other means to save/restore signals
- Accept your conditional approach, which solves the `sigaction` problem for some subset of people who may have actually defined a custom handler
I'm not going to decide among these for upstream.

If you do PR your optional approach upstream, you should probably define a package option to declare the dependency so that the package can advertise the possibility.

Re: [PR PATCH] [Updated] python3-prompt_toolkit: don't handle sigint

[tornaria]

[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]

There is an updated pull request by tornaria against master on the void-packages repository

https://github.com/tornaria/void-packages prompt_toolkit
https://github.com/void-linux/void-packages/pull/35730

python3-prompt_toolkit: don't handle sigint
Fixes #35712

See also: https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576

<!-- Uncomment relevant sections and delete options which are not applicable -->

#### Testing the changes
- I tested the changes in this PR: **YES**

<!--
#### New package
- This new package conforms to the [quality requirements](https://github.com/void-linux/void-packages/blob/master/Manual.md#quality-requirements): **YES**|**NO**
-->

<!-- Note: If the build is likely to take more than 2 hours, please [skip CI](https://github.com/void-linux/void-packages/blob/master/CONTRIBUTING.md#continuous-integration)
and test at least one native build and, if supported, at least one cross build.
Ignore this section if this PR is not skipping CI.
-->
<!-- 
#### Local build testing
- I built this PR locally for my native architecture, (ARCH-LIBC)
- I built this PR locally for these architectures (if supported. mark crossbuilds):
  - aarch64-musl
  - armv7l
  - armv6l-musl
-->


A patch file from https://github.com/void-linux/void-packages/pull/35730.patch is attached

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: github-pr-prompt_toolkit-35730.patch --]
[-- Type: text/x-diff, Size: 2352 bytes --]

From ccb29d45b22cecb2b69e83c2a091a7a97598a940 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gonzalo=20Tornar=C3=ADa?= <tornaria@cmat.edu.uy>
Date: Mon, 21 Feb 2022 10:52:16 -0300
Subject: [PATCH] python3-prompt_toolkit: don't handle sigint

Fixes #35712

See also: https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576
---
 .../patches/dont-handle-sigint.patch          | 27 +++++++++++++++++++
 srcpkgs/python3-prompt_toolkit/template       |  2 +-
 2 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 srcpkgs/python3-prompt_toolkit/patches/dont-handle-sigint.patch

diff --git a/srcpkgs/python3-prompt_toolkit/patches/dont-handle-sigint.patch b/srcpkgs/python3-prompt_toolkit/patches/dont-handle-sigint.patch
new file mode 100644
index 000000000000..60ce287a63d1
--- /dev/null
+++ b/srcpkgs/python3-prompt_toolkit/patches/dont-handle-sigint.patch
@@ -0,0 +1,27 @@
+This makes handle_sigint default to False
+
+See:
+ - https://github.com/void-linux/void-packages/issues/35712
+ - https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576
+ - https://trac.sagemath.org/ticket/33360#comment:3
+
+--- a/prompt_toolkit/application/application.py	2022-02-11 05:06:37.000000000 -0300
++++ b/prompt_toolkit/application/application.py	2022-02-21 10:43:13.828058001 -0300
+@@ -634,7 +634,7 @@
+         self,
+         pre_run: Optional[Callable[[], None]] = None,
+         set_exception_handler: bool = True,
+-        handle_sigint: bool = True,
++        handle_sigint: bool = False,
+         slow_callback_duration: float = 0.5,
+     ) -> _AppResult:
+         """
+@@ -859,7 +859,7 @@
+         self,
+         pre_run: Optional[Callable[[], None]] = None,
+         set_exception_handler: bool = True,
+-        handle_sigint: bool = True,
++        handle_sigint: bool = False,
+         in_thread: bool = False,
+     ) -> _AppResult:
+         """
diff --git a/srcpkgs/python3-prompt_toolkit/template b/srcpkgs/python3-prompt_toolkit/template
index 0d625dc249fa..26bcb33e82bc 100644
--- a/srcpkgs/python3-prompt_toolkit/template
+++ b/srcpkgs/python3-prompt_toolkit/template
@@ -1,7 +1,7 @@
 # Template file for 'python3-prompt_toolkit'
 pkgname=python3-prompt_toolkit
 version=3.0.28
-revision=1
+revision=2
 wrksrc="prompt_toolkit-${version}"
 build_style=python3-module
 hostmakedepends="python3-setuptools"

Re: python3-prompt_toolkit: don't handle sigint

[tornaria]

[-- Attachment #1: Type: text/plain, Size: 1938 bytes --]

New comment by tornaria on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1053793760

Comment:
>     1. Your original patch to change to change the default SIGINT handling behavior from `True` to `False`, effectively restoring prior behavior and leaving any custom handlers untouched; or

Let's do this, I've force-pushed my original patch. This is completely acceptable for sagemath.


> Option 1 may cause IPython to crash when it receives SIGINT, but that is functionally no worse than a revert to a prior version of `prompt_toolkit` and I can live with it.

I can live with this too. I hadn't even noticed this before I looked at the xonsh bug report.

> [...] 

>having `prompt_toolkit` take special branches in every iteration of the event loop on the off chance that an arbitrary package is installed does not seem to be the right approach and certainly doesn't cover these alternatives.

Just for the record, assuming I understand the way python imports work, my patch doesn't add branches in the event loop.  It tries to import cysignals.pysignals ONCE when prompt_toolkit is imported for the first time. If that fails, it will import instead alternative pure python functions (not using cysignals / not preserving the os-level sigint handler).  These functions are unconditionally called once when prompt starts (to save the sigint handler) and once when the prompt finishes (to restore the sigint handler). The event loop runs all the prompt editing, completion, etc, etc, but this will not mess with signals at all.


> If you do PR your optional approach upstream, you should probably define a package option to declare the dependency so that the package can advertise the possibility.

I'll wait for comments first before doing a PR. I might ask you for help then, since my understanding of python packaging and hard vs optional dependencies is almost nil.

Re: [PR PATCH] [Merged]: python3-prompt_toolkit: don't handle sigint

[ahesford]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

There's a merged pull request on the void-packages repository

python3-prompt_toolkit: don't handle sigint
https://github.com/void-linux/void-packages/pull/35730

Description:
Fixes #35712

See also: https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576

<!-- Uncomment relevant sections and delete options which are not applicable -->

#### Testing the changes
- I tested the changes in this PR: **YES**

<!--
#### New package
- This new package conforms to the [quality requirements](https://github.com/void-linux/void-packages/blob/master/Manual.md#quality-requirements): **YES**|**NO**
-->

<!-- Note: If the build is likely to take more than 2 hours, please [skip CI](https://github.com/void-linux/void-packages/blob/master/CONTRIBUTING.md#continuous-integration)
and test at least one native build and, if supported, at least one cross build.
Ignore this section if this PR is not skipping CI.
-->
<!-- 
#### Local build testing
- I built this PR locally for my native architecture, (ARCH-LIBC)
- I built this PR locally for these architectures (if supported. mark crossbuilds):
  - aarch64-musl
  - armv7l
  - armv6l-musl
-->

Re: python3-prompt_toolkit: don't handle sigint

[ahesford]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]

New comment by ahesford on void-packages repository

https://github.com/void-linux/void-packages/pull/35730#issuecomment-1053809782

Comment:
Thanks for entertaining my debate. 

If you think that I can provide assistance, feel free to tag me in a PR or ping me on IRC.