-
Notifications
You must be signed in to change notification settings - Fork 20
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
Let subprocess launched by run handle its own sigint #246
Let subprocess launched by run handle its own sigint #246
Conversation
This simply ignores the signal though. We'd want to delegate it to the subprocess. With this PR, in numpy's repo, I get: $ spin run python
$ /tmp/spin/bin/python3 vendored-meson/meson/meson.py compile -C build
$ /tmp/spin/bin/python3 vendored-meson/meson/meson.py install --only-changed -C build --destdir ../build-install
Python 3.12.7 (main, Oct 1 2024, 11:15:50) [GCC 14.2.1 20240910] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import time
>>> time.sleep(10)
^C^C^C>>> while with a simple python process you'd get: $ python
Python 3.12.7 (main, Oct 1 2024, 11:15:50) [GCC 14.2.1 20240910] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import time
>>> time.sleep(10)
^CTraceback (most recent call last):
File "<stdin>", line 1, in <module>
KeyboardInterrupt
>>> |
OK, will dig a bit further. |
Hopefully got it this time! |
Tests supposed to be failing? |
No, it's on Windows only, will see why; Windows probably doesn't support preexec_fn. |
But I can confirm this fixes the issue nicely in linux at least. |
Alternative approach to try: try:
process.wait()
except KeyboardInterrupt:
print("Terminating subprocess...")
parent = psutil.Process(process.pid)
for child in parent.children(recursive=True):
child.kill()
process.wait() |
I think I like this as is now, we shouldn't decide for the process that sigint should kill it. If the process wants to do a dance on that signal, it's free to do so 😁 |
I just need to fire up my Windows test machine, then I can see if it works on that platform as well. |
Otherwise, the subprocess is aborted by the parent process. This change makes `spin run python -i ...` usable. Closes scientific-python#235
45d2cb6
to
5c0a79d
Compare
Tested on Windows, and seems like the behavior is as intended without any changes, so reverted (on that platform). |
Otherwise, the subprocess is aborted by the parent process. This change makes
spin run python -i ...
usable.Closes #235
/cc @adrinjalali