scons_subproc_run: update docstrings, add tests

Documentation reworded (docstring/comment only, this is not a
public API).

Removed duplicated cleaning of execution env - previous iteration
both called the Util function and then did the same work manually.

Added some unit tests for the argument fiddling.

Signed-off-by: Mats Wichmann <mats@linux.com>

SCons/Action.py

@@ -108,7 +108,7 @@
 import sys
 from abc import ABC, abstractmethod
 from collections import OrderedDict
-from subprocess import DEVNULL
+from subprocess import DEVNULL, PIPE
 
 import SCons.Debug
 import SCons.Errors
@@ -468,7 +468,7 @@ def _do_create_action(act, kw):
         return CommandAction(act, **kw)
 
     if callable(act):
-        gen = kw.pop('generator', 0)
+        gen = kw.pop('generator', False)
         if gen:
             action_type = CommandGeneratorAction
         else:
@@ -819,43 +819,48 @@ def _resolve_shell_env(env, target, source):
 
 
 def scons_subproc_run(scons_env, *args, **kwargs) -> subprocess.CompletedProcess:
-    """Run a command with arguments using an SCons execution environment.
-
-    Does an underlyng call to :func:`subprocess.run` to run a command and
-    returns a :class:`subprocess.CompletedProcess` instance with the results.
-    Use when an external command needs to run in an "SCons context" -
-    that is, with a crafted execution environment, rather than the user's
-    existing environment, particularly when you need to collect output
-    back. Typical case: run a tool's "version" option to find out which
-    version you have installed.
-
-    If supplied, the ``env`` keyword argument passes an
-    execution environment to process into appropriate form before it is
-    supplied to :mod:`subprocess`; if omitted, *scons_env* is used to derive
-    a suitable default.  The other keyword arguments are passed through,
-    except that SCons legacy ``error`` keyword is remapped to
-    the subprocess ``check`` keyword.  The caller is responsible for
-    setting up the desired arguments  for :func:`subprocess.run`.
+    """Run an external command using an SCons execution environment.
+
+    SCons normally runs external build commands using :mod:`subprocess`,
+    but does not harvest any output from such commands. This function
+    is a thin wrapper around :func:`subprocess.run` allowing running
+    a command in an SCons context (i.e. uses an "execution environment"
+    rather than the user's existing environment), and provides the ability
+    to return any output in a :class:`subprocess.CompletedProcess`
+    instance (this must be selected by setting ``stdout`` and/or
+    ``stderr`` to ``PIPE``, or setting ``capture_output=True`` - see
+    Keyword Arguments). Typical use case is to run a tool's "version"
+    option to find out the installed version.
+
+    If supplied, the ``env`` keyword argument provides an execution
+    environment to process into appropriate form before it is supplied
+    to :mod:`subprocess`; if omitted, *scons_env* is used to derive a
+    suitable default.  The other keyword arguments are passed through,
+    except that the SCons legacy ``error`` keyword is remapped to the
+    subprocess ``check`` keyword; if both are omitted ``check=False``
+    will be passed. The caller is responsible for setting up the desired
+    arguments  for :func:`subprocess.run`.
 
     This function retains the legacy behavior of returning something
-    vaguely usable even in the face of complete failure: it synthesizes
-    a :class:`~subprocess.CompletedProcess` instance in this case.
+    vaguely usable even in the face of complete failure, unless
+    ``check=True`` (in which case an error is allowed to be raised):
+    it synthesizes a :class:`~subprocess.CompletedProcess` instance in
+    this case.
 
     A subset of interesting keyword arguments follows; see the Python
-    documentation of :mod:`subprocess` for a complete list.
+    documentation of :mod:`subprocess` for the complete list.
 
     Keyword Arguments:
        stdout: (and *stderr*, *stdin*) if set to :const:`subprocess.PIPE`.
           send input to or collect output from the relevant stream in
           the subprocess; the default ``None`` does no redirection
           (i.e. output or errors may go to the console or log file,
           but is not captured); if set to :const:`subprocess.DEVNULL`
-          they are explicitly thrown away. ``capture_output`` is a
+          they are explicitly thrown away. ``capture_output=True`` is a
           synonym for setting both ``stdout`` and ``stderr``
           to :const:`~subprocess.PIPE`.
        text: open *stdin*, *stdout*, *stderr* in text mode. Default
-          is binary mode. ``universal_newlines`` is a synonym -
-          note ``text`` is not understood before Python 3.7.
+          is binary mode. ``universal_newlines`` is a synonym.
        encoding: specifies an encoding. Changes to text mode.
        errors: specified error handling. Changes to text mode.
        input: a byte sequence to be passed to *stdin*, unless text
@@ -869,10 +874,10 @@ def scons_subproc_run(scons_env, *args, **kwargs) -> subprocess.CompletedProcess
     .. versionadded:: 4.6
     """
     # Figure out the execution environment to use
-    ENV = kwargs.get('env', None)
-    if ENV is None:
-        ENV = get_default_ENV(scons_env)
-    kwargs['env'] = SCons.Util.sanitize_shell_env(ENV)
+    env = kwargs.get('env', None)
+    if env is None:
+        env = get_default_ENV(scons_env)
+    kwargs['env'] = SCons.Util.sanitize_shell_env(env)
 
     # Backwards-compat with _subproc: accept 'error', map to 'check',
     # and remove, since subprocess.run does not recognize.
@@ -887,37 +892,30 @@ def scons_subproc_run(scons_env, *args, **kwargs) -> subprocess.CompletedProcess
         del kwargs['error']
     kwargs['check'] = check
 
-    # Ensure that the ENV values are all strings:
-    new_env = {}
-    for key, value in ENV.items():
-        if is_List(value):
-            # If the value is a list, then we assume it is a path list,
-            # because that's a pretty common list-like value to stick
-            # in an environment variable:
-            value = SCons.Util.flatten_sequence(value)
-            new_env[key] = os.pathsep.join(str(v) for v in value)
-        else:
-            # If it's not a list type, "convert" it to str. This is
-            # harmless if it's already a string, but gets us the right
-            # thing for Dir and File instances and will produce something
-            # reasonable for just about everything else:
-            new_env[key] = str(value)
-    kwargs['env'] = new_env
-
-    # Some tools/tests expect no failures for things like missing files
-    # unless raise/check is set. If set, let subprocess.run go ahead and
-    # kill things, else catch and construct a dummy CompletedProcess instance.
-    # Note pylint can't see we always include 'check', so quiet it.
+    # TODO: Python version-compat stuff: remap/remove too-new args if needed
+    if 'text' in kwargs and sys.version_info[:3] < (3, 7):
+        kwargs['universal_newlines'] = kwargs.pop('text')
+
+    if 'capture_output' in kwargs and sys.version_info[:3] < (3, 7):
+        capture_output = kwargs.pop('capture_output')
+        if capture_output:
+            kwargs['stdout'] = kwargs['stderr'] = PIPE
+
+    # Most SCons tools/tests expect not to fail on things like missing files.
+    # check=True (or error="raise") means we're okay to take an exception;
+    # else we catch the likely exception and construct a dummy
+    # CompletedProcess instance.
+    # Note pylint can't see we always include 'check' in kwargs: suppress.
     if check:
         cp = subprocess.run(*args, **kwargs)  # pylint: disable=subprocess-run-check
     else:
         try:
             cp = subprocess.run(*args, **kwargs)  # pylint: disable=subprocess-run-check
         except OSError as exc:
             argline = ' '.join(*args)
-            cp = subprocess.CompletedProcess(argline, 1)
-            cp.stdout = ""
-            cp.stderr = ""
+            cp = subprocess.CompletedProcess(
+                args=argline, returncode=1, stdout="", stderr=""
+            )
 
     return cp
 

SCons/ActionTests.py

@@ -41,11 +41,13 @@ def __call__(self) -> None:
 import sys
 import types
 import unittest
+from unittest import mock
 from subprocess import PIPE
 
 import SCons.Action
 import SCons.Environment
 import SCons.Errors
+from SCons.Action import scons_subproc_run
 
 import TestCmd
 
@@ -2329,7 +2331,7 @@ def test_code_contents(self) -> None:
     def test_uncaught_exception_bubbles(self):
         """Test that scons_subproc_run bubbles uncaught exceptions"""
         try:
-            cp = SCons.Action.scons_subproc_run(Environment(), None, stdout=PIPE)
+            cp = scons_subproc_run(Environment(), None, stdout=PIPE)
         except EnvironmentError:
             pass
         except Exception:
@@ -2338,6 +2340,53 @@ def test_uncaught_exception_bubbles(self):
 
         raise Exception("expected a non-EnvironmentError exception")
 
+
+    def mock_subprocess_run(*args, **kwargs):
+        """Replacement subprocess.run: return kwargs for checking."""
+        kwargs.pop("env")  # the value of env isn't interesting here
+        return kwargs
+
+    @mock.patch("subprocess.run", mock_subprocess_run)
+    def test_scons_subproc_run(self):
+        """Test the argument remapping options."""
+        env = Environment()
+        self.assertEqual(scons_subproc_run(env), {"check": False})
+        with self.subTest():
+            self.assertEqual(
+                scons_subproc_run(env, error="raise"),
+                {"check": True}
+            )
+        with self.subTest():
+            self.assertEqual(
+                scons_subproc_run(env, capture_output=True),
+                {"capture_output": True, "check": False},
+            )
+        with self.subTest():
+            self.assertEqual(
+                scons_subproc_run(env, text=True),
+                {"text": True, "check": False},
+            )
+
+        # 3.6:
+        save_info, sys.version_info = sys.version_info, (3, 6, 2)
+        with self.subTest():
+            self.assertEqual(
+                scons_subproc_run(env, capture_output=True),
+                {"check": False, "stdout": PIPE, "stderr": PIPE},
+            )
+        with self.subTest():
+            self.assertEqual(
+                scons_subproc_run(env, text=True),
+                {"check": False, "universal_newlines": True},
+            )
+        with self.subTest():
+            self.assertEqual(
+                scons_subproc_run(env, universal_newlines=True),
+                {"universal_newlines": True, "check": False},
+            )
+        sys.version_info = save_info
+
+
 if __name__ == "__main__":
     unittest.main()