Skip to content

Conversation

@stefanv
Copy link
Member

@stefanv stefanv commented Nov 8, 2024

Otherwise, the subprocess is aborted by the parent process. This change makes spin run python -i ... usable.

Closes #235

/cc @adrinjalali

@adrinjalali
Copy link

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

@stefanv
Copy link
Member Author

stefanv commented Nov 8, 2024

OK, will dig a bit further.

@stefanv stefanv added the type: Bug fix Something isn't working label Nov 9, 2024
@stefanv
Copy link
Member Author

stefanv commented Nov 9, 2024

Hopefully got it this time!

@adrinjalali
Copy link

Tests supposed to be failing?

@stefanv
Copy link
Member Author

stefanv commented Nov 12, 2024

Tests supposed to be failing?

No, it's on Windows only, will see why; Windows probably doesn't support preexec_fn.

@adrinjalali
Copy link

But I can confirm this fixes the issue nicely in linux at least.

@stefanv
Copy link
Member Author

stefanv commented Nov 17, 2024

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()

@adrinjalali
Copy link

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 😁

@stefanv
Copy link
Member Author

stefanv commented Nov 17, 2024

I just need to fire up my Windows test machine, then I can see if it works on that platform as well.

@stefanv stefanv force-pushed the avoid-capturing-sigint branch from 45d2cb6 to 5c0a79d Compare November 17, 2024 20:23
@stefanv
Copy link
Member Author

stefanv commented Nov 17, 2024

Tested on Windows, and seems like the behavior is as intended without any changes, so reverted (on that platform).

@stefanv stefanv merged commit 06dbe73 into scientific-python:main Nov 17, 2024
@jarrodmillman jarrodmillman added this to the 0.13 milestone Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: Bug fix Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spin catches SIGINT and send SIGTERM to subprocess

3 participants