From 6a62af0e74869875d49c7660103196d2da1dc5a5 Mon Sep 17 00:00:00 2001
From: ttimo <ttimo@ttimo.net>
Date: Thu, 31 Aug 2023 11:56:45 -0500
Subject: [PATCH] support capturing output of child processes and routing
 through the status window. will make situations similar to the previous fix
 easier to diagnose

---
 client/devkit_client/__init__.py       | 51 +++++++++++++---------
 client/devkit_client/captured_popen.py | 59 ++++++++++++++++++++++++++
 client/devkit_client/gui2/gui2.py      | 20 ++++++---
 3 files changed, 104 insertions(+), 26 deletions(-)
 create mode 100644 client/devkit_client/captured_popen.py

diff --git a/client/devkit_client/__init__.py b/client/devkit_client/__init__.py
index 820d3a1..6a12cf0 100644
--- a/client/devkit_client/__init__.py
+++ b/client/devkit_client/__init__.py
@@ -60,6 +60,7 @@ import appdirs
 
 import paramiko
 import devkit_client.zeroconf as zeroconf
+import devkit_client.captured_popen as captured_popen
 
 try:
     import devkit_client.version
@@ -120,6 +121,7 @@ g_remote_debuggers = None
 g_external_tools = None
 g_lock = threading.Lock()
 
+g_captured_popen_factory = captured_popen.CapturedPopenFactory()
 
 # This burned me twice now .. https://twitter.com/TTimo/status/1582509449838989313
 from os import getenv as os_getenv
@@ -1325,26 +1327,35 @@ Start-Sleep -Seconds 3
             commands = [powershell_path, '-ExecutionPolicy', 'Bypass', batch.name]
         # ensures we get a separate console when running out of a shell with pipenv
         creationflags=subprocess.CREATE_NEW_CONSOLE
-    else:
-        matched = False
-        for terminal_prefix in (
-            ['konsole', '-e'],
-            ['gnome-terminal', '--'],
-            ['xterm', '-e'],
-        ):
-            shell_path = shutil.which(terminal_prefix[0])
-            if shell_path is not None:
-                commands = [ shell_path, ] + terminal_prefix[1:] + commands
-                logger.info(f'Open terminal: {commands!r}')
-                matched = True
-                break
-        if not matched:
-            raise Exception('Could not find a suitable terminal to run command!')
+        # we cannot use captured output here, we'd actually capture the shell stdout and break the terminal
+        logger.info(f'Run in terminal, cwd {cwd!r}: {" ".join(commands)}')
+        p = subprocess.Popen(
+            commands,
+            cwd=cwd,
+            creationflags=creationflags,
+        )
+        return p
+
+    # Linux
+    matched = False
+    for terminal_prefix in (
+        ['konsole', '-e'],
+        ['gnome-terminal', '--'],
+        ['xterm', '-e'],
+    ):
+        shell_path = shutil.which(terminal_prefix[0])
+        if shell_path is not None:
+            commands = [ shell_path, ] + terminal_prefix[1:] + commands
+            logger.info(f'Open terminal: {commands!r}')
+            matched = True
+            break
+    if not matched:
+        raise Exception('Could not find a suitable terminal to run command!')
     logger.info(f'Run in terminal, cwd {cwd!r}: {" ".join(commands)}')
-    p = subprocess.Popen(
+    p = g_captured_popen_factory.Popen(
         commands,
-        creationflags=creationflags,
-        cwd=cwd,
+        cwd,
+        creationflags
     )
     return p
 
@@ -1462,7 +1473,7 @@ def gpu_trace(args):
             raise Exception(f'Invalid GPU Vis path - does not exist: {args.gpuvis_path}')
         gpuvis_cmd = [args.gpuvis_path, local_trace_file]
         logger.info(' '.join(gpuvis_cmd))
-        subprocess.Popen(gpuvis_cmd)
+        g_captured_popen_factory.Popen(gpuvis_cmd)
 
 
 def rgp_capture(args):
@@ -1504,7 +1515,7 @@ def rgp_capture(args):
         if not os.path.exists(args.rgp_path):
             raise Exception(f'Invalid Radeon GPU Profiler path - does not exist: {args.rgp_path}')
         profiler_cmd = [args.rgp_path, local_path]
-        subprocess.Popen(profiler_cmd)
+        g_captured_popen_factory.Popen(profiler_cmd)
 
 
 def config_steam_wrapper_flags(devkit, enable, disable):
diff --git a/client/devkit_client/captured_popen.py b/client/devkit_client/captured_popen.py
new file mode 100644
index 0000000..5f592f3
--- /dev/null
+++ b/client/devkit_client/captured_popen.py
@@ -0,0 +1,59 @@
+import logging
+import subprocess
+import threading
+
+logger = logging.getLogger(__name__)
+
+# Even on Linux the stderr/stdout of child processes is often silenced or not accessible,
+# this enables a capture of the combined stderr/stdout output to the status window, via the logging facilities
+# similar to Popen.communicate, but with threads
+class CapturedPopenFactory:
+    def __init__(self):
+        self._enabled = True
+        self.fds = []
+
+    @property
+    def enabled(self):
+        return self._enabled
+
+    @enabled.setter
+    def enabled(self, v):
+        self._enabled = v
+
+    def on_shutdown_signal(self, **kwargs):
+        # Closing when we exit since we won't be polling anymore, to avoid blocking if the pipes fill up
+        if len(self.fds) == 0:
+            return
+        logger.info(f'CapturedPopenFactory closing {len(self.fds)} child process output streams.')
+        for f in self.fds:
+            f.close()
+
+    def set_shutdown_signal(self, s):
+        s.connect(self.on_shutdown_signal)
+
+    def _thread_read(self, f):
+        for l in f.readlines():
+            logger.info(l.strip('\n'))
+        self.fds.remove(f)
+
+    def Popen(self, cmd, cwd=None, creationflags=0):
+        if not self.enabled:
+            return subprocess.Popen(
+                cmd,
+                cwd=cwd,
+                creationflags=creationflags,
+            )
+
+        p = subprocess.Popen(
+            cmd,
+            cwd=cwd,
+            creationflags=creationflags,
+            text=True,
+            bufsize=1,
+            stdin=subprocess.DEVNULL,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT,
+        )
+        self.fds.append(p.stdout)
+        threading.Thread(target=self._thread_read, args=(p.stdout, ), daemon=True).start()
+        return p
diff --git a/client/devkit_client/gui2/gui2.py b/client/devkit_client/gui2/gui2.py
index 52ba5c2..5bb89f9 100644
--- a/client/devkit_client/gui2/gui2.py
+++ b/client/devkit_client/gui2/gui2.py
@@ -529,7 +529,7 @@ class DevkitCommands:
         if filezilla is None or not os.path.exists(filezilla):
             raise Exception('FileZilla not found. Please install in order to use this feature.')
         cmd = [filezilla, '-l', 'ask', f'sftp://{devkit.machine.login}@{devkit.machine.address}']
-        subprocess.Popen(cmd)
+        devkit_client.g_captured_popen_factory.Popen(cmd)
 
     def browse_files(self, *args):
         return self.executor.submit(self._browse_files, *args)
@@ -2691,7 +2691,7 @@ class RenderDocCapture(SubTool):
             #       additionally, remoteaccess just doesn't work.
             rdoc_cmd = [self.settings[self.RDOC_KEY]] #, "--remoteaccess", machine.address, "--replayhost", machine.address]
             logger.info(' '.join(rdoc_cmd))
-            subprocess.Popen(rdoc_cmd)
+            devkit_client.g_captured_popen_factory.Popen(rdoc_cmd)
 
 class ProtonLogs(SubTool):
     BUTTON_NAME = 'Sync Proton Logs'
@@ -3296,20 +3296,24 @@ def main():
     parser.add_argument(
         '--verbose', required=False, action='store',
         default='INFO', const='DEBUG', nargs='?',
-        help='Logging verbosity'
+        help='Set logging verbosity.'
     )
     parser.add_argument(
         '--logfile', required=False, action='store',
-        help='Log to file'
+        help='Log to a file.'
     )
     parser.add_argument(
         '--valve', required=False, action='store_true',
-        help='Force Valve mode features (default: auto detect)'
+        help='Force Valve mode features (default: auto detect).'
     )
     parser.add_argument(
         '--check-port-timeout', required=False, action='store',
         default=4,
-        help='Timeout when checking open ports (default 4) - may need to be bumped up on very slow networks'
+        help='Timeout when checking open ports (default 4) - may need to be bumped up on very slow networks.'
+    )
+    parser.add_argument(
+        '--disable-popen-capture', required=False, action='store_true',
+        help='Disable capturing of launched external processes output to the status window.'
     )
 
     conf = parser.parse_args()
@@ -3337,6 +3341,10 @@ def main():
 
     shutdown_signal = signalslot.Signal()
 
+    if conf.disable_popen_capture:
+        devkit_client.g_captured_popen_factory.enabled = False
+    devkit_client.g_captured_popen_factory.set_shutdown_signal( shutdown_signal )
+
     settings = Settings()
     shutdown_signal.connect(settings.on_shutdown_signal)
     atexit.register(settings.shutdown) # saves settings on abnormal termination
-- 
GitLab