Align our subprocess usage with current best practices. (#12940)

* Align our subprocess usage with current best practices.

* remove unused import

* Apply suggestions from code review

Co-authored-by: Ryan <fauxpark@gmail.com>

* fix the cpp invocation for older python

* allow for unprompted installation

* make sure qmk new-keyboard works on windows

Co-authored-by: Ryan <fauxpark@gmail.com>

lib/python/qmk/cli/cformat.py

@@ -1,8 +1,8 @@
 """Format C code according to QMK's style.
 """
-import subprocess
 from os import path
 from shutil import which
+from subprocess import CalledProcessError, DEVNULL, Popen, PIPE
 
 from argcomplete.completers import FilesCompleter
 from milc import cli
@@ -34,7 +34,7 @@ def find_diffs(files):
 
     for file in files:
         cli.log.debug('Checking for changes in %s', file)
-        clang_format = subprocess.Popen([find_clang_format(), file], stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
+        clang_format = Popen([find_clang_format(), file], stdout=PIPE, stderr=PIPE, universal_newlines=True)
         diff = cli.run(['diff', '-u', f'--label=a/{file}', f'--label=b/{file}', str(file), '-'], stdin=clang_format.stdout, capture_output=True)
 
         if diff.returncode != 0:
@@ -51,11 +51,11 @@ def cformat_run(files):
     clang_format = [find_clang_format(), '-i']
 
     try:
-        cli.run(clang_format + list(map(str, files)), check=True, capture_output=False)
+        cli.run([*clang_format, *map(str, files)], check=True, capture_output=False, stdin=DEVNULL)
         cli.log.info('Successfully formatted the C code.')
         return True
 
-    except subprocess.CalledProcessError as e:
+    except CalledProcessError as e:
         cli.log.error('Error formatting C code!')
         cli.log.debug('%s exited with returncode %s', e.cmd, e.returncode)
         cli.log.debug('STDOUT:')
@@ -111,7 +111,7 @@ def cformat(cli):
 
     else:
         git_diff_cmd = ['git', 'diff', '--name-only', cli.args.base_branch, *core_dirs]
-        git_diff = cli.run(git_diff_cmd)
+        git_diff = cli.run(git_diff_cmd, stdin=DEVNULL)
 
         if git_diff.returncode != 0:
             cli.log.error("Error running %s", git_diff_cmd)

lib/python/qmk/cli/clean.py

@@ -1,6 +1,8 @@
 """Clean the QMK firmware folder of build artifacts.
 """
-from qmk.commands import run, create_make_target
+from subprocess import DEVNULL
+
+from qmk.commands import create_make_target
 from milc import cli
 
 
@@ -9,4 +11,4 @@
 def clean(cli):
     """Runs `make clean` (or `make distclean` if --all is passed)
     """
-    run(create_make_target('distclean' if cli.args.all else 'clean'))
+    cli.run(create_make_target('distclean' if cli.args.all else 'clean'), capture_output=False, stdin=DEVNULL)

lib/python/qmk/cli/compile.py

@@ -2,6 +2,8 @@
 
 You can compile a keymap already in the repo or using a QMK Configurator export.
 """
+from subprocess import DEVNULL
+
 from argcomplete.completers import FilesCompleter
 from milc import cli
 
@@ -31,8 +33,7 @@ def compile(cli):
     """
     if cli.args.clean and not cli.args.filename and not cli.args.dry_run:
         command = create_make_command(cli.config.compile.keyboard, cli.config.compile.keymap, 'clean')
-        # FIXME(skullydazed/anyone): Remove text=False once milc 1.0.11 has had enough time to be installed everywhere.
-        cli.run(command, capture_output=False, text=False)
+        cli.run(command, capture_output=False, stdin=DEVNULL)
 
     # Build the environment vars
     envs = {}

lib/python/qmk/cli/doctor.py

@@ -3,12 +3,12 @@
 Check out the user's QMK environment and make sure it's ready to compile.
 """
 import platform
+from subprocess import DEVNULL
 
 from milc import cli
 from milc.questions import yesno
 from qmk import submodules
 from qmk.constants import QMK_FIRMWARE
-from qmk.commands import run
 from qmk.os_helpers import CheckStatus, check_binaries, check_binary_versions, check_submodules, check_git_repo
 
 
@@ -93,7 +93,7 @@ def doctor(cli):
 
     if not bin_ok:
         if yesno('Would you like to install dependencies?', default=True):
-            run(['util/qmk_install.sh'])
+            cli.run(['util/qmk_install.sh', '-y'], stdin=DEVNULL, capture_output=False)
             bin_ok = check_binaries()
 
     if bin_ok:

lib/python/qmk/cli/flash.py

@@ -3,6 +3,7 @@
 You can compile a keymap already in the repo or using a QMK Configurator export.
 A bootloader must be specified.
 """
+from subprocess import DEVNULL
 
 from argcomplete.completers import FilesCompleter
 from milc import cli
@@ -55,7 +56,7 @@ def flash(cli):
     """
     if cli.args.clean and not cli.args.filename and not cli.args.dry_run:
         command = create_make_command(cli.config.flash.keyboard, cli.config.flash.keymap, 'clean')
-        cli.run(command, capture_output=False)
+        cli.run(command, capture_output=False, stdin=DEVNULL)
 
     # Build the environment vars
     envs = {}
@@ -98,7 +99,7 @@ def flash(cli):
         cli.log.info('Compiling keymap with {fg_cyan}%s', ' '.join(command))
         if not cli.args.dry_run:
             cli.echo('\n')
-            compile = cli.run(command, capture_output=False, text=True)
+            compile = cli.run(command, capture_output=False, stdin=DEVNULL)
             return compile.returncode
 
     else:

lib/python/qmk/cli/generate/docs.py

@@ -1,8 +1,8 @@
 """Build QMK documentation locally
 """
 import shutil
-import subprocess
 from pathlib import Path
+from subprocess import DEVNULL
 
 from milc import cli
 
@@ -24,14 +24,16 @@ def generate_docs(cli):
     shutil.copytree(DOCS_PATH, BUILD_PATH)
 
     # When not verbose we want to hide all output
-    args = {'check': True}
-    if not cli.args.verbose:
-        args.update({'stdout': subprocess.DEVNULL, 'stderr': subprocess.STDOUT})
+    args = {
+        'capture_output': False if cli.config.general.verbose else True,
+        'check': True,
+        'stdin': DEVNULL,
+    }
 
     cli.log.info('Generating internal docs...')
 
     # Generate internal docs
-    subprocess.run(['doxygen', 'Doxyfile'], **args)
-    subprocess.run(['moxygen', '-q', '-a', '-g', '-o', BUILD_PATH / 'internals_%s.md', 'doxygen/xml'], **args)
+    cli.run(['doxygen', 'Doxyfile'], **args)
+    cli.run(['moxygen', '-q', '-a', '-g', '-o', BUILD_PATH / 'internals_%s.md', 'doxygen/xml'], **args)
 
     cli.log.info('Successfully generated internal docs to %s.', BUILD_PATH)

lib/python/qmk/cli/multibuild.py

@@ -4,6 +4,7 @@
 """
 import re
 from pathlib import Path
+from subprocess import DEVNULL
 
 from milc import cli
 
@@ -35,7 +36,7 @@ def multibuild(cli):
 
     make_cmd = _find_make()
     if cli.args.clean:
-        cli.run([make_cmd, 'clean'], capture_output=False, text=False)
+        cli.run([make_cmd, 'clean'], capture_output=False, stdin=DEVNULL)
 
     builddir = Path(QMK_FIRMWARE) / '.build'
     makefile = builddir / 'parallel_kb_builds.mk'
@@ -75,4 +76,4 @@ def multibuild(cli):
             )
             # yapf: enable
 
-    cli.run([make_cmd, '-j', str(cli.args.parallel), '-f', makefile, 'all'], capture_output=False, text=False)
+    cli.run([make_cmd, '-j', str(cli.args.parallel), '-f', makefile, 'all'], capture_output=False, stdin=DEVNULL)

lib/python/qmk/cli/new/keyboard.py

@@ -8,4 +8,4 @@ def new_keyboard(cli):
     """Creates a new keyboard
     """
     # TODO: replace this bodge to the existing script
-    cli.run(['util/new_keyboard.sh'], capture_output=False)
+    cli.run(['util/new_keyboard.sh'], stdin=None, capture_output=False)

lib/python/qmk/cli/pyformat.py

@@ -1,8 +1,8 @@
 """Format python code according to QMK's style.
 """
-from milc import cli
+from subprocess import CalledProcessError, DEVNULL
 
-import subprocess
+from milc import cli
 
 
 @cli.argument('-n', '--dry-run', arg_only=True, action='store_true', help="Flag only, don't automatically format.")
@@ -13,11 +13,11 @@ def pyformat(cli):
     edit = '--diff' if cli.args.dry_run else '--in-place'
     yapf_cmd = ['yapf', '-vv', '--recursive', edit, 'bin/qmk', 'lib/python']
     try:
-        cli.run(yapf_cmd, check=True, capture_output=False)
+        cli.run(yapf_cmd, check=True, capture_output=False, stdin=DEVNULL)
         cli.log.info('Python code in `bin/qmk` and `lib/python` is correctly formatted.')
         return True
 
-    except subprocess.CalledProcessError:
+    except CalledProcessError:
         if cli.args.dry_run:
             cli.log.error('Python code in `bin/qmk` and `lib/python` incorrectly formatted!')
         else:

lib/python/qmk/cli/pytest.py

@@ -2,7 +2,7 @@
 
 QMK script to run unit and integration tests against our python code.
 """
-import subprocess
+from subprocess import DEVNULL
 
 from milc import cli
 
@@ -11,7 +11,7 @@
 def pytest(cli):
     """Run several linting/testing commands.
     """
-    nose2 = subprocess.run(['nose2', '-v'])
-    flake8 = subprocess.run(['flake8', 'lib/python', 'bin/qmk'])
+    nose2 = cli.run(['nose2', '-v'], capture_output=False, stdin=DEVNULL)
+    flake8 = cli.run(['flake8', 'lib/python', 'bin/qmk'], capture_output=False, stdin=DEVNULL)
 
     return flake8.returncode | nose2.returncode

lib/python/qmk/commands.py

@@ -2,11 +2,9 @@
 """
 import json
 import os
-import platform
-import subprocess
-import shlex
 import shutil
 from pathlib import Path
+from subprocess import DEVNULL
 from time import strftime
 
 from milc import cli
@@ -94,7 +92,7 @@ def get_git_version(repo_dir='.', check_dir='.'):
     git_describe_cmd = ['git', 'describe', '--abbrev=6', '--dirty', '--always', '--tags']
 
     if Path(check_dir).exists():
-        git_describe = cli.run(git_describe_cmd, cwd=repo_dir)
+        git_describe = cli.run(git_describe_cmd, stdin=DEVNULL, cwd=repo_dir)
 
         if git_describe.returncode == 0:
             return git_describe.stdout.strip()
@@ -224,20 +222,3 @@ def parse_configurator_json(configurator_file):
             user_keymap['layout'] = aliases[orig_keyboard]['layouts'][user_keymap['layout']]
 
     return user_keymap
-
-
-def run(command, *args, **kwargs):
-    """Run a command with subprocess.run
-    """
-    platform_id = platform.platform().lower()
-
-    if isinstance(command, str):
-        raise TypeError('`command` must be a non-text sequence such as list or tuple.')
-
-    if 'windows' in platform_id:
-        safecmd = map(str, command)
-        safecmd = map(shlex.quote, safecmd)
-        safecmd = ' '.join(safecmd)
-        command = [os.environ.get('SHELL', '/usr/bin/bash'), '-c', safecmd]
-
-    return subprocess.run(command, *args, **kwargs)

lib/python/qmk/keymap.py

@@ -1,9 +1,9 @@
 """Functions that help you work with QMK keymaps.
 """
 import json
-import subprocess
 import sys
 from pathlib import Path
+from subprocess import DEVNULL
 
 import argcomplete
 from milc import cli
@@ -12,7 +12,6 @@
 from pygments import lex
 
 import qmk.path
-import qmk.commands
 from qmk.keyboard import find_keyboard_from_dir, rules_mk
 
 # The `keymap.c` template to use when a keyboard doesn't have its own
@@ -361,7 +360,7 @@ def list_keymaps(keyboard, c=True, json=True, additional_files=None, fullpath=Fa
     return sorted(names)
 
 
-def _c_preprocess(path, stdin=None):
+def _c_preprocess(path, stdin=DEVNULL):
     """ Run a file through the C pre-processor
 
     Args:
@@ -371,7 +370,9 @@ def _c_preprocess(path, stdin=None):
     Returns:
         the stdout of the pre-processor
     """
-    pre_processed_keymap = qmk.commands.run(['cpp', path] if path else ['cpp'], stdin=stdin, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
+    cmd = ['cpp', str(path)] if path else ['cpp']
+    pre_processed_keymap = cli.run(cmd, stdin=stdin)
+
     return pre_processed_keymap.stdout
 
 

lib/python/qmk/os_helpers/__init__.py

@@ -3,10 +3,9 @@
 from enum import Enum
 import re
 import shutil
-import subprocess
+from subprocess import DEVNULL
 
 from milc import cli
-from qmk.commands import run
 from qmk import submodules
 from qmk.constants import QMK_FIRMWARE
 
@@ -142,7 +141,7 @@ def is_executable(command):
 
     # Make sure the command can be executed
     version_arg = ESSENTIAL_BINARIES[command].get('version_arg', '--version')
-    check = run([command, version_arg], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, timeout=5, universal_newlines=True)
+    check = cli.run([command, version_arg], combined_output=True, stdin=DEVNULL, timeout=5)
 
     ESSENTIAL_BINARIES[command]['output'] = check.stdout
 

lib/python/qmk/os_helpers/linux/__init__.py

@@ -5,7 +5,6 @@
 
 from milc import cli
 from qmk.constants import QMK_FIRMWARE
-from qmk.commands import run
 from qmk.os_helpers import CheckStatus
 
 
@@ -132,7 +131,7 @@ def check_modem_manager():
 
     """
     if check_systemd():
-        mm_check = run(["systemctl", "--quiet", "is-active", "ModemManager.service"], timeout=10)
+        mm_check = cli.run(["systemctl", "--quiet", "is-active", "ModemManager.service"], timeout=10)
         if mm_check.returncode == 0:
             return True
     else:

lib/python/qmk/submodules.py

@@ -1,7 +1,6 @@
 """Functions for working with QMK's submodules.
 """
-
-import subprocess
+from milc import cli
 
 
 def status():
@@ -18,7 +17,7 @@ def status():
     status is None when the submodule doesn't exist, False when it's out of date, and True when it's current
     """
     submodules = {}
-    git_cmd = subprocess.run(['git', 'submodule', 'status'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, timeout=30, universal_newlines=True)
+    git_cmd = cli.run(['git', 'submodule', 'status'], timeout=30)
 
     for line in git_cmd.stdout.split('\n'):
         if not line:
@@ -53,19 +52,19 @@ def update(submodules=None):
         # Update everything
         git_sync_cmd.append('--recursive')
         git_update_cmd.append('--recursive')
-        subprocess.run(git_sync_cmd, check=True)
-        subprocess.run(git_update_cmd, check=True)
+        cli.run(git_sync_cmd, check=True)
+        cli.run(git_update_cmd, check=True)
 
     else:
         if isinstance(submodules, str):
             # Update only a single submodule
             git_sync_cmd.append(submodules)
             git_update_cmd.append(submodules)
-            subprocess.run(git_sync_cmd, check=True)
-            subprocess.run(git_update_cmd, check=True)
+            cli.run(git_sync_cmd, check=True)
+            cli.run(git_update_cmd, check=True)
 
         else:
             # Update submodules in a list
             for submodule in submodules:
-                subprocess.run(git_sync_cmd + [submodule], check=True)
-                subprocess.run(git_update_cmd + [submodule], check=True)
+                cli.run([*git_sync_cmd, submodule], check=True)
+                cli.run([*git_update_cmd, submodule], check=True)