From 5b1ec638ce152a07464862c5f2d67ccc55e74672 Mon Sep 17 00:00:00 2001
From: ahochheiden <ahochheiden@mozilla.com>
Date: Thu, 20 Jan 2022 19:34:45 +0000
Subject: [PATCH] Bug 1748070 - rewrote `show_log` to resolve various bugs on
 Windows (While still maintaining current behavior on POSIX systems)
 r=firefox-build-system-reviewers,glandium,mhentges

- Changed byte strings being set in env to just strings
  as byte strings were only necessaery in Python2 and
  strings work fine in POSIX and Windows as of Python3.
  (Byte strings no longer work on Windows in Python3)
- Replaced all the file descriptor logic used to pipe
  to `less` with more Pythonic stream manipulation.
  (There was a change in Windows console IO in Python 3.6
  that broke the file descriptor logic we were using here)

  See: https://www.python.org/dev/peps/pep-0528/#add-io-windowsconsoleio

Differential Revision: https://phabricator.services.mozilla.com/D135969
---
 python/mozbuild/mozbuild/mach_commands.py | 67 ++++++++++++++---------
 1 file changed, 42 insertions(+), 25 deletions(-)

diff --git a/python/mozbuild/mozbuild/mach_commands.py b/python/mozbuild/mozbuild/mach_commands.py
index 56c90b3c809a..edebc82df7a4 100644
--- a/python/mozbuild/mozbuild/mach_commands.py
+++ b/python/mozbuild/mozbuild/mach_commands.py
@@ -343,7 +343,11 @@ def clobber(command_context, what, full=False):
     "mach command.",
 )
 def show_log(command_context, log_file=None):
-    """Show mach logs."""
+    """Show mach logs
+    If we're in a terminal context, the log is piped to 'less'
+    for more convenient viewing.
+    (https://man7.org/linux/man-pages/man1/less.1.html)
+    """
     if not log_file:
         path = command_context._get_state_filename("last_log.json")
         log_file = open(path, "rb")
@@ -351,22 +355,46 @@ def show_log(command_context, log_file=None):
     if os.isatty(sys.stdout.fileno()):
         env = dict(os.environ)
         if "LESS" not in env:
-            # Sensible default flags if none have been set in the user
-            # environment.
-            env[b"LESS"] = b"FRX"
-        less = subprocess.Popen(["less"], stdin=subprocess.PIPE, env=env)
-        # Various objects already have a reference to sys.stdout, so we
-        # can't just change it, we need to change the file descriptor under
-        # it to redirect to less's input.
-        # First keep a copy of the sys.stdout file descriptor.
-        output_fd = os.dup(sys.stdout.fileno())
-        os.dup2(less.stdin.fileno(), sys.stdout.fileno())
+            # Sensible default flags if none have been set in the user environment.
+            env["LESS"] = "FRX"
+        less = subprocess.Popen(
+            ["less"], stdin=subprocess.PIPE, env=env, encoding="UTF-8"
+        )
 
-    startTime = 0
+        # Create a new logger handler with the stream being the stdin of our 'less'
+        # process so that we can pipe the logger output into 'less'
+        less_handler = logging.StreamHandler(stream=less.stdin)
+        less_handler.setFormatter(
+            command_context.log_manager.terminal_handler.formatter
+        )
+        less_handler.setLevel(command_context.log_manager.terminal_handler.level)
+
+        # replace the existing terminal handler with the new one for 'less' while
+        # still keeping the original one to set back later
+        original_handler = command_context.log_manager.replace_terminal_handler(
+            less_handler
+        )
+
+        # Parses the log file line by line and streams (to less.stdin) the relevant records we want
+        handle_log_file(command_context, log_file)
+
+        # Set the handler (and contained stream) back to the original one
+        command_context.log_manager.replace_terminal_handler(original_handler)
+
+        # At this point we've piped the entire log file to 'less', so we can close the input stream
+        less.stdin.close()
+        # Wait for the user to manually terminate `less`
+        less.wait()
+    else:
+        handle_log_file(command_context, log_file)
+
+
+def handle_log_file(command_context, log_file):
+    start_time = 0
     for line in log_file:
         created, action, params = json.loads(line)
-        if not startTime:
-            startTime = created
+        if not start_time:
+            start_time = created
             command_context.log_manager.terminal_handler.formatter.start_time = created
         if "line" in params:
             record = logging.makeLogRecord(
@@ -381,17 +409,6 @@ def show_log(command_context, log_file=None):
             )
             command_context._logger.handle(record)
 
-    if command_context.log_manager.terminal:
-        # Close less's input so that it knows that we're done sending data.
-        less.stdin.close()
-        # Since the less's input file descriptor is now also the stdout
-        # file descriptor, we still actually have a non-closed system file
-        # descriptor for less's input. Replacing sys.stdout's file
-        # descriptor with what it was before we replaced it will properly
-        # close less's input.
-        os.dup2(output_fd, sys.stdout.fileno())
-        less.wait()
-
 
 # Provide commands for inspecting warnings.