Repository: pipxproject/pipx (python)

Issue:
pipx run isn't redirecting signals to child app (?)
Hello,

What a wonderful tool, no more manual management of python apps in my system :)

About the issue, I wonder if signals are being redirected to the child app when calling `pipx run <app>`. For instance:

```bash
pipx run --spec tmonpy tmon
```

`tmon` in the absence of arguments, waits for `SIGINT` to return and print a temperature report. But it looks like `pipx run` when `SIGINT`-ed kills the app and doesn't give it a chance to return gracefully (I could be wrong though, I haven't looked at the source).

So this is what I would expect to happen:
```bash
pipx run --spec tmonpy tmon
```
wait a few seconds, press Ctrl+C and a graph should be printed in the terminal:

```
===================
Temp Monitor Report

    temp (°C) for a period of 0:00:04
       41.00  ┤
       40.86  ┤
       40.71  ┤
       40.57  ┤
       40.43  ┤
       40.29  ┤
       40.14  ┤
       40.00  ┼╮
       39.86  ┤│
       39.71  ┤│
       39.57  ┤│
       39.43  ┤│
       39.29  ┤│
       39.14  ┤│
       39.00  ┤╰───

    min: 39.1 °C
    avg: 39.2 °C
    max: 40.0 °C

    raw: /tmp/tmon-20200926@19h56m17-c1jxq4s6.txt
===================
```

Note: `tmon` is expected to work on Linux only! Don't try to run the test case on mac or win.


Pull request:
Use os.exec* for `pipx run` app execution under POSIX.
<!-- add an 'x' in the brackets below -->
* [x] I have added an entry to `docs/changelog.md`

## Summary of changes
This changes how a `pipx run` app is executed.  

For POSIX systems (AKA not Windows) it uses `os.execvpe()` to replace the current pipx process with the app in question.  From that point on pipx is not acting as a parent process and is out of the picture.

For Windows, this wasn't a reliable method, so for Windows `subprocess.run()` and `sys.exit()` are used to approximate `os.exec*`.

Because now running an app never returns control to pipx, modifications to deleting a `.cache` venv when under `--no-cache` operation had to be made.  Previously this was done after running the app.  Now, before running the app, a file named `pipx_expired_venv` is placed inside the venv as a signal to future `pipx run` invocations to delete this venv during `_remove_all_expired_venvs()`.

Testing was changed to accommodate the new behavior of exiting with an exit code instead of returning via `return`.  The exit code is verified to be correct as needed.

Closes #506 

### Incidental changes

I renamed `util.run()` to `util.run_verify()` to better describe what it is and why it is different from `run_subprocess()`.

## Test plan
<!-- provide evidence of testing, preferably with command(s) that can be copy+pasted by others -->
This should be an internal change to `pipx run`, and should only be cleaner with respect to certain cases, such as how signals are processed as described in #506 for example.  Otherwise `pipx run` should execute commands with no obvious differences than before.
```
pipx run pycowsay Mooooooo!
```


Patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index e39429a..a95436d 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -1,6 +1,7 @@
 dev
 
 - Added reinstall command for reinstalling a single venv.
+- Changed `pipx run` on non-Windows systems to actually replace pipx process with the app process instead of running it as a subprocess.  (Now using python's `os.exec*`)
 
 0.15.6.0
 
diff --git a/src/pipx/commands/run.py b/src/pipx/commands/run.py
index a9b812e..cd3148b 100644
--- a/src/pipx/commands/run.py
+++ b/src/pipx/commands/run.py
@@ -1,7 +1,6 @@
 import datetime
 import hashlib
 import logging
-import subprocess
 import time
 import urllib.parse
 import urllib.request
@@ -16,12 +15,15 @@ from pipx.emojies import hazard
 from pipx.util import (
     WINDOWS,
     PipxError,
+    exec_app,
     get_pypackage_bin_path,
     rmdir,
     run_pypackage_bin,
 )
 from pipx.venv import Venv
 
+VENV_EXPIRED_FILENAME = "pipx_expired_venv"
+
 
 def run(
     app: str,
@@ -33,7 +35,7 @@ def run(
     pypackages: bool,
     verbose: bool,
     use_cache: bool,
-):
+) -> None:
     """Installs venv to temporary dir (or reuses cache), then runs app from
     package
     """
@@ -47,10 +49,8 @@ def run(
         logging.info("Detected url. Downloading and executing as a Python file.")
 
         content = _http_get_request(app)
-        try:
-            return subprocess.run([str(python), "-c", content]).returncode
-        except KeyboardInterrupt:
-            return 1
+        # This never returns
+        exec_app([str(python), "-c", content])
 
     elif which(app):
         logging.warning(
@@ -68,7 +68,8 @@ def run(
         logging.info(
             f"Using app in local __pypackages__ directory at {str(pypackage_bin_path)}"
         )
-        return run_pypackage_bin(pypackage_bin_path, app_args)
+        # This never returns
+        run_pypackage_bin(pypackage_bin_path, app_args)
     if pypackages:
         raise PipxError(
             f"'--pypackages' flag was passed, but {str(pypackage_bin_path)!r} was "
@@ -84,10 +85,12 @@ def run(
 
     if bin_path.exists():
         logging.info(f"Reusing cached venv {venv_dir}")
-        retval = venv.run_app(app, app_args)
+        # This never returns
+        venv.run_app(app, app_args)
     else:
         logging.info(f"venv location is {venv_dir}")
-        retval = _download_and_run(
+        # This never returns
+        _download_and_run(
             Path(venv_dir),
             package_or_url,
             app,
@@ -95,13 +98,10 @@ def run(
             python,
             pip_args,
             venv_args,
+            use_cache,
             verbose,
         )
 
-    if not use_cache:
-        rmdir(venv_dir)
-    return retval
-
 
 def _download_and_run(
     venv_dir: Path,
@@ -111,8 +111,9 @@ def _download_and_run(
     python: str,
     pip_args: List[str],
     venv_args: List[str],
+    use_cache: bool,
     verbose: bool,
-):
+) -> None:
     venv = Venv(venv_dir, python=python, verbose=verbose)
     venv.create_venv(venv_args, pip_args)
 
@@ -139,7 +140,13 @@ def _download_and_run(
             "Available executable scripts: "
             f"{', '.join(b for b in apps)}"
         )
-    return venv.run_app(app, app_args)
+
+    if not use_cache:
+        # Let future _remove_all_expired_venvs know to remove this
+        (venv_dir / VENV_EXPIRED_FILENAME).touch()
+
+    # This never returns
+    venv.run_app(app, app_args)
 
 
 def _get_temporary_venv_path(
@@ -164,7 +171,7 @@ def _is_temporary_venv_expired(venv_dir: Path):
     current_time_sec = time.mktime(datetime.datetime.now().timetuple())
     age = current_time_sec - created_time_sec
     expiration_threshold_sec = 60 * 60 * 24 * TEMP_VENV_EXPIRATION_THRESHOLD_DAYS
-    return age > expiration_threshold_sec
+    return age > expiration_threshold_sec or (venv_dir / VENV_EXPIRED_FILENAME).exists()
 
 
 def _prepare_venv_cache(venv: Venv, bin_path: Path, use_cache: bool):
diff --git a/src/pipx/main.py b/src/pipx/main.py
index a53081c..cccf01c 100644
--- a/src/pipx/main.py
+++ b/src/pipx/main.py
@@ -141,7 +141,8 @@ def run_pipx_command(args: argparse.Namespace) -> int:  # noqa: C901
             else args.app_with_args[0]
         )
         use_cache = not args.no_cache
-        return commands.run(
+        # This never returns
+        commands.run(
             args.app_with_args[0],
             package_or_url,
             args.app_with_args[1:],
@@ -152,6 +153,8 @@ def run_pipx_command(args: argparse.Namespace) -> int:  # noqa: C901
             verbose,
             use_cache,
         )
+        # error, we should never reach it
+        return 1
     elif args.command == "install":
         return commands.install(
             None,
diff --git a/src/pipx/shared_libs.py b/src/pipx/shared_libs.py
index 93c3268..d4b0280 100644
--- a/src/pipx/shared_libs.py
+++ b/src/pipx/shared_libs.py
@@ -1,13 +1,13 @@
+import datetime
 import logging
+import time
 from pathlib import Path
 from typing import List
-import time
-import datetime
 
 from pipx.animate import animate
 from pipx.constants import PIPX_SHARED_LIBS, WINDOWS
 from pipx.interpreter import DEFAULT_PYTHON
-from pipx.util import get_site_packages, get_venv_paths, run
+from pipx.util import get_site_packages, get_venv_paths, run_verify
 
 SHARED_LIBS_MAX_AGE_SEC = datetime.timedelta(days=30).total_seconds()
 
@@ -32,7 +32,7 @@ class _SharedLibs:
     def create(self, pip_args: List[str], verbose: bool = False):
         if not self.is_valid:
             with animate("creating shared libraries", not verbose):
-                run([DEFAULT_PYTHON, "-m", "venv", "--clear", self.root])
+                run_verify([DEFAULT_PYTHON, "-m", "venv", "--clear", self.root])
             # ignore installed packages to ensure no unexpected patches from the OS vendor
             # are used
             self.upgrade(["--ignore-installed"] + pip_args, verbose)
@@ -70,7 +70,7 @@ class _SharedLibs:
             _pip_args.append("-q")
         try:
             with animate("upgrading shared libraries", not verbose):
-                run(
+                run_verify(
                     [
                         self.python_path,
                         "-m",
diff --git a/src/pipx/util.py b/src/pipx/util.py
index 03709c4..3b7a2b7 100644
--- a/src/pipx/util.py
+++ b/src/pipx/util.py
@@ -6,6 +6,7 @@ import sys
 from pathlib import Path
 from typing import List, Optional, Sequence, Tuple, Union
 
+from pipx.animate import show_cursor
 from pipx.constants import WINDOWS
 
 
@@ -41,7 +42,7 @@ def get_pypackage_bin_path(binary_name: str) -> Path:
     )
 
 
-def run_pypackage_bin(bin_path: Path, args: List[str]) -> int:
+def run_pypackage_bin(bin_path: Path, args: List[str]) -> None:
     def _get_env():
         env = dict(os.environ)
         env["PYTHONPATH"] = os.path.pathsep.join(
@@ -50,12 +51,7 @@ def run_pypackage_bin(bin_path: Path, args: List[str]) -> int:
         )
         return env
 
-    try:
-        return subprocess.run(
-            [str(bin_path.resolve())] + args, env=_get_env()
-        ).returncode
-    except KeyboardInterrupt:
-        return 1
+    exec_app([str(bin_path.resolve())] + args, env=_get_env())
 
 
 if WINDOWS:
@@ -84,15 +80,7 @@ def get_site_packages(python: Path) -> Path:
     return path
 
 
-def run_subprocess(
-    cmd: Sequence[Union[str, Path]],
-    capture_stdout: bool = True,
-    capture_stderr: bool = True,
-    log_cmd_str: Optional[str] = None,
-) -> subprocess.CompletedProcess:
-    """Run arbitrary command as subprocess, capturing stderr and stout"""
-    env = dict(os.environ)
-
+def _fix_subprocess_env(env):
     # Remove PYTHONPATH because some platforms (macOS with Homebrew) add pipx
     #   directories to it, and can make it appear to venvs as though pipx
     #   dependencies are in the venv path (#233)
@@ -107,6 +95,18 @@ def run_subprocess(
     env["PYTHONIOENCODING"] = "utf-8"
     # Make sure we install package to venv, not userbase dir
     env["PIP_USER"] = "0"
+    return env
+
+
+def run_subprocess(
+    cmd: Sequence[Union[str, Path]],
+    capture_stdout: bool = True,
+    capture_stderr: bool = True,
+    log_cmd_str: Optional[str] = None,
+) -> subprocess.CompletedProcess:
+    """Run arbitrary command as subprocess, capturing stderr and stout"""
+    env = dict(os.environ)
+    env = _fix_subprocess_env(env)
 
     if log_cmd_str is None:
         log_cmd_str = " ".join(str(c) for c in cmd)
@@ -123,17 +123,48 @@ def run_subprocess(
     )
 
 
-def run(cmd: Sequence[Union[str, Path]], check=True) -> int:
-    """Run arbitrary command as subprocess"""
+def run_verify(cmd: Sequence[Union[str, Path]]) -> None:
+    """Run arbitrary command as subprocess, raise PipxError if error exit code"""
 
     returncode = run_subprocess(
         cmd, capture_stdout=False, capture_stderr=False
     ).returncode
 
-    if check and returncode:
+    if returncode:
         cmd_str = " ".join(str(c) for c in cmd)
         raise PipxError(f"{cmd_str!r} failed")
-    return returncode
+
+
+def exec_app(cmd: Sequence[Union[str, Path]], env=None) -> None:
+    """Run command, do not return
+
+    POSIX: replace current processs with command using os.exec*()
+    Windows: Use subprocess and sys.exit() to run command
+    """
+
+    if env is None:
+        env = dict(os.environ)
+    env = _fix_subprocess_env(env)
+
+    # make sure we show cursor again before handing over control
+    show_cursor()
+    sys.stderr.flush()
+
+    logging.info("exec_app: " + " ".join([str(c) for c in cmd]))
+
+    if WINDOWS:
+        sys.exit(
+            subprocess.run(
+                cmd,
+                env=env,
+                stdout=None,
+                stderr=None,
+                encoding="utf-8",
+                universal_newlines=True,
+            ).returncode
+        )
+    else:
+        os.execvpe(str(cmd[0]), [str(x) for x in cmd], env)
 
 
 def full_package_description(package: str, package_spec: str) -> str:
diff --git a/src/pipx/venv.py b/src/pipx/venv.py
index c2a4ed7..b5f5ec7 100644
--- a/src/pipx/venv.py
+++ b/src/pipx/venv.py
@@ -16,12 +16,13 @@ from pipx.pipx_metadata_file import PackageInfo, PipxMetadata
 from pipx.shared_libs import shared_libs
 from pipx.util import (
     PipxError,
+    exec_app,
     full_package_description,
     get_site_packages,
     get_venv_paths,
     rmdir,
-    run,
     run_subprocess,
+    run_verify,
 )
 
 venv_metadata_inspector_raw = pkgutil.get_data("pipx", "venv_metadata_inspector.py")
@@ -33,8 +34,7 @@ VENV_METADATA_INSPECTOR = venv_metadata_inspector_raw.decode("utf-8")
 
 
 class VenvContainer:
-    """A collection of venvs managed by pipx.
-    """
+    """A collection of venvs managed by pipx."""
 
     def __init__(self, root: Path):
         self._root = root
@@ -46,16 +46,14 @@ class VenvContainer:
         return str(self._root)
 
     def iter_venv_dirs(self) -> Generator[Path, None, None]:
-        """Iterate venv directories in this container.
-        """
+        """Iterate venv directories in this container."""
         for entry in self._root.iterdir():
             if not entry.is_dir():
                 continue
             yield entry
 
     def get_venv_dir(self, package: str) -> Path:
-        """Return the expected venv path for given `package`.
-        """
+        """Return the expected venv path for given `package`."""
         return self._root.joinpath(package)
 
     def verify_shared_libs(self):
@@ -134,7 +132,7 @@ class Venv:
     def create_venv(self, venv_args: List[str], pip_args: List[str]) -> None:
         with animate("creating virtual environment", self.do_animation):
             cmd = [self.python, "-m", "venv", "--without-pip"]
-            run(cmd + venv_args + [str(self.root)])
+            run_verify(cmd + venv_args + [str(self.root)])
         shared_libs.create(pip_args, self.verbose)
         pipx_pth = get_site_packages(self.python_path) / PIPX_SHARED_PTH
         # write path pointing to the shared libs site-packages directory
@@ -333,12 +331,8 @@ class Venv:
         pip_list = json.loads(cmd_run.stdout.strip())
         return set([x["name"] for x in pip_list])
 
-    def run_app(self, app: str, app_args: List[str]) -> int:
-        cmd = [str(self.bin_path / app)] + app_args
-        try:
-            return run(cmd, check=False)
-        except KeyboardInterrupt:
-            return 130  # shell code for Ctrl-C
+    def run_app(self, app: str, app_args: List[str]) -> None:
+        exec_app([str(self.bin_path / app)] + app_args)
 
     def _upgrade_package_no_metadata(self, package: str, pip_args: List[str]) -> None:
         with animate(
@@ -372,8 +366,8 @@ class Venv:
             suffix=suffix,
         )
 
-    def _run_pip(self, cmd: List[str]) -> int:
+    def _run_pip(self, cmd: List[str]) -> None:
         cmd = [str(self.python_path), "-m", "pip"] + cmd
         if not self.verbose:
             cmd.append("-q")
-        return run(cmd)
+        run_verify(cmd)


Test patch:
diff --git a/tests/test_run.py b/tests/test_run.py
index b9b84ca..546ddcf 100644
--- a/tests/test_run.py
+++ b/tests/test_run.py
@@ -7,6 +7,7 @@ from unittest import mock
 import pytest  # type: ignore
 
 import pipx.main
+import pipx.util
 from helpers import run_pipx_cli
 
 
@@ -20,30 +21,54 @@ def test_help_text(pipx_temp_env, monkeypatch, capsys):
     assert "Download the latest version of a package" in captured.out
 
 
+def execvpe_mock(cmd_path, cmd_args, env):
+    return_code = subprocess.run(
+        [str(x) for x in cmd_args],
+        env=env,
+        stdout=None,
+        stderr=None,
+        encoding="utf-8",
+        universal_newlines=True,
+    ).returncode
+    sys.exit(return_code)
+
+
+def run_pipx_cli_exit(pipx_cmd_list, assert_exit=None):
+    with pytest.raises(SystemExit) as sys_exit:
+        run_pipx_cli(pipx_cmd_list)
+    if assert_exit is not None:
+        assert sys_exit.type == SystemExit
+        assert sys_exit.value.code == assert_exit
+
+
+@mock.patch("os.execvpe", new=execvpe_mock)
 def test_simple_run(pipx_temp_env, monkeypatch, capsys):
-    run_pipx_cli(["run", "pycowsay", "--help"])
+    run_pipx_cli_exit(["run", "pycowsay", "--help"])
     captured = capsys.readouterr()
     assert "Download the latest version of a package" not in captured.out
 
 
+@mock.patch("os.execvpe", new=execvpe_mock)
 def test_cache(pipx_temp_env, monkeypatch, capsys, caplog):
-    run_pipx_cli(["run", "pycowsay", "cowsay", "args"])
+    run_pipx_cli_exit(["run", "pycowsay", "cowsay", "args"])
     caplog.set_level(logging.DEBUG)
-    assert not run_pipx_cli(["run", "--verbose", "pycowsay", "cowsay", "args"])
+    run_pipx_cli_exit(["run", "--verbose", "pycowsay", "cowsay", "args"], assert_exit=0)
     assert "Reusing cached venv" in caplog.text
 
-    run_pipx_cli(["run", "--no-cache", "pycowsay", "cowsay", "args"])
+    run_pipx_cli_exit(["run", "--no-cache", "pycowsay", "cowsay", "args"])
     assert "Removing cached venv" in caplog.text
 
 
+@mock.patch("os.execvpe", new=execvpe_mock)
 def test_run_script_from_internet(pipx_temp_env, capsys):
-    assert not run_pipx_cli(
+    run_pipx_cli_exit(
         [
             "run",
             "https://gist.githubusercontent.com/cs01/"
             "fa721a17a326e551ede048c5088f9e0f/raw/"
             "6bdfbb6e9c1132b1c38fdd2f195d4a24c540c324/pipx-demo.py",
-        ]
+        ],
+        assert_exit=0,
     )
 
 
@@ -100,25 +125,32 @@ def test_run_ensure_null_pythonpath():
 
 # packages listed roughly in order of increasing test duration
 @pytest.mark.parametrize(
-    "package, package_or_url, app_appargs",
+    "package, package_or_url, app_appargs, skip_win",
     [
-        ("pycowsay", "pycowsay", ["pycowsay", "hello"]),
-        ("shell-functools", "shell-functools", ["filter", "--help"]),
-        ("black", "black", ["black", "--help"]),
-        ("pylint", "pylint", ["pylint", "--help"]),
-        ("kaggle", "kaggle", ["kaggle", "--help"]),
-        ("ipython", "ipython", ["ipython", "--version"]),
-        ("cloudtoken", "cloudtoken", ["cloudtoken", "--help"]),
-        ("awscli", "awscli", ["aws", "--help"]),
+        ("pycowsay", "pycowsay", ["pycowsay", "hello"], False),
+        ("shell-functools", "shell-functools", ["filter", "--help"], True),
+        ("black", "black", ["black", "--help"], False),
+        ("pylint", "pylint", ["pylint", "--help"], False),
+        ("kaggle", "kaggle", ["kaggle", "--help"], False),
+        ("ipython", "ipython", ["ipython", "--version"], False),
+        ("cloudtoken", "cloudtoken", ["cloudtoken", "--help"], True),
+        ("awscli", "awscli", ["aws", "--help"], True),
         # ("ansible", "ansible", ["ansible", "--help"]), # takes too long
     ],
 )
+@mock.patch("os.execvpe", new=execvpe_mock)
 def test_package_determination(
-    caplog, pipx_temp_env, package, package_or_url, app_appargs
+    caplog, pipx_temp_env, package, package_or_url, app_appargs, skip_win
 ):
+    if sys.platform.startswith("win") and skip_win:
+        # Skip packages with 'scripts' in setup.py that don't work on Windows
+        pytest.skip()
+
     caplog.set_level(logging.INFO)
 
-    run_pipx_cli(["run", "--verbose", "--spec", package_or_url, "--"] + app_appargs)
+    run_pipx_cli_exit(
+        ["run", "--verbose", "--spec", package_or_url, "--"] + app_appargs
+    )
 
     assert "Cannot determine package name" not in caplog.text
     assert f"Determined package name: {package}" in caplog.text