Skip to content

Commit

Permalink
refacto(cli): change restart by subprocess
Browse files Browse the repository at this point in the history
because of Windows OS not being able to restart process we relly on calling a subprocess instead.

See python/cpython#63323
  • Loading branch information
MrLixm committed Oct 19, 2024
1 parent f31667f commit 64aee6f
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 22 deletions.
22 changes: 17 additions & 5 deletions knots_hub/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import json
import logging
import os
import subprocess
import sys
import webbrowser
from typing import Type
Expand Down Expand Up @@ -117,6 +118,11 @@ def execute(self):
local_install_path = self._config.local_install_path
is_runtime_local = is_runtime_from_local_install(local_install_path)

# we restart to the local install for performance consideration.
# And we force to start by the remote as it's always the latest version,
# while local may be an older version which will have issue reading more
# up-to date configs and similar.

if not is_runtime_local:

# an empty installer path imply the hub is never installed/updated locally
Expand Down Expand Up @@ -285,11 +291,17 @@ def _restart_hub(self, exe: str):
restarted_amount=self._restarted,
)

LOGGER.info(f"restarting hub '{exe}' ...")
# safety mentioned in the doc
sys.stdout.flush()
LOGGER.debug(f"os.execv({exe},{argv})")
os.execv(exe, argv)
command = [exe] + argv[1:]

# this is an undocumented env var only used for internal testing
asshell: bool = bool(os.getenv("KNOTS_HUB_RESTART_AS_SHELL", False))

LOGGER.info(f"restarting hub to '{exe}' (asshell={asshell}) ...")
LOGGER.debug(f"subprocess.run({command})")
# XXX: we use subprocess instead of os.execv because the implementation on Windows
# is not a real restart https://github.com/python/cpython/issues/63323.
result = subprocess.run(command, shell=asshell)
sys.exit(result.returncode)


class _Parser(BaseParser):
Expand Down
35 changes: 18 additions & 17 deletions tests/test_end2end.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def test__main__full(monkeypatch, data_dir, tmp_path, caplog):
with vendor_config_path.open("w") as file:
json.dump(vendor_config, file, indent=4)

class ExecvPatcher:
class SubprocessPatcher:

exe: Path = None
args: List[str] = None
Expand All @@ -90,16 +90,16 @@ def clear(cls):
cls.called = False

@classmethod
def _patch_os_execv(cls, exe, args):
exe = Path(exe)
def _patch(cls, command, *args, **kwargs):
exe = Path(command[0])
cls.called = True
cls.exe = exe
cls.args = args
cls.args = command[1:]

def _patch_sys_argv():
return sys.argv.copy() + ["UNWANTEDARG"]

monkeypatch.setattr(os, "execv", ExecvPatcher._patch_os_execv)
monkeypatch.setattr(subprocess, "run", SubprocessPatcher._patch)
monkeypatch.setattr(sys, "argv", _patch_sys_argv)

monkeypatch.setenv(knots_hub.Environ.USER_INSTALL_PATH, str(install_dir))
Expand All @@ -122,8 +122,8 @@ def _patch_sys_argv():

expected_local_exe = install_dir / exe_name
assert expected_local_exe.exists()
assert ExecvPatcher.called is True
assert ExecvPatcher.exe == expected_local_exe
assert SubprocessPatcher.called is True
assert SubprocessPatcher.exe == expected_local_exe

# check the vendor install system

Expand Down Expand Up @@ -164,29 +164,29 @@ def _patched_is_runtime_from_local_install(*args):
_patched_is_runtime_from_local_install,
)

argv = ExecvPatcher.args.copy()[1:]
ExecvPatcher.clear()
argv = SubprocessPatcher.args.copy()[1:]
SubprocessPatcher.clear()
with pytest.raises(SystemExit):
knots_hub.__main__.main(argv=argv)

assert ExecvPatcher.called is False
assert SubprocessPatcher.called is False
assert InstallRezPatcher.called is True
assert InstallPythonPatcher.called is True
assert vendor_install_dir.exists()
assert rez_install_dir.exists()

# check no install

ExecvPatcher.clear()
SubprocessPatcher.clear()
InstallRezPatcher.called = False
InstallPythonPatcher.called = False

argv = ["--restarted", "1"]
ExecvPatcher.clear()
SubprocessPatcher.clear()
with pytest.raises(SystemExit):
knots_hub.__main__.main(argv=argv)

assert ExecvPatcher.called is False
assert SubprocessPatcher.called is False
assert InstallRezPatcher.called is False
assert InstallPythonPatcher.called is False

Expand All @@ -206,16 +206,16 @@ def _patched_is_runtime_from_local_install(*args):
with vendor_config_path.open("w") as file:
json.dump(vendor_config, file, indent=4)

ExecvPatcher.clear()
SubprocessPatcher.clear()
InstallRezPatcher.called = False
InstallPythonPatcher.called = False

argv = ["--restarted", "1"]
ExecvPatcher.clear()
SubprocessPatcher.clear()
with pytest.raises(SystemExit):
knots_hub.__main__.main(argv=argv)

assert ExecvPatcher.called is False
assert SubprocessPatcher.called is False
assert InstallRezPatcher.called is True
assert InstallPythonPatcher.called is True

Expand All @@ -228,7 +228,8 @@ def _patched_is_runtime_from_local_install(*args):
# check uninstall

def _patch_execv(exe_: str, argv_: List[str]):
subprocess.run([exe_] + argv_[1:], check=True)
# we patch subprocess.run above so use a different function
subprocess.check_call([exe_] + argv_[1:])

monkeypatch.setattr(os, "execv", _patch_execv)

Expand Down

0 comments on commit 64aee6f

Please sign in to comment.