From db1eacdaacb9c8f6889f46bc1c6af155b81ad72a Mon Sep 17 00:00:00 2001 From: Zach White Date: Wed, 19 May 2021 15:24:46 -0700 Subject: [PATCH] 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 * fix the cpp invocation for older python * allow for unprompted installation * make sure qmk new-keyboard works on windows Co-authored-by: Ryan --- lib/python/qmk/cli/cformat.py | 10 ++++----- lib/python/qmk/cli/clean.py | 6 ++++-- lib/python/qmk/cli/compile.py | 5 +++-- lib/python/qmk/cli/doctor.py | 4 ++-- lib/python/qmk/cli/flash.py | 5 +++-- lib/python/qmk/cli/generate/docs.py | 14 +++++++------ lib/python/qmk/cli/multibuild.py | 5 +++-- lib/python/qmk/cli/new/keyboard.py | 2 +- lib/python/qmk/cli/pyformat.py | 8 +++---- lib/python/qmk/cli/pytest.py | 6 +++--- lib/python/qmk/commands.py | 23 ++------------------- lib/python/qmk/keymap.py | 9 ++++---- lib/python/qmk/os_helpers/__init__.py | 5 ++--- lib/python/qmk/os_helpers/linux/__init__.py | 3 +-- lib/python/qmk/submodules.py | 17 +++++++-------- lib/python/qmk/tests/test_cli_commands.py | 9 ++++---- util/install/debian.sh | 2 +- util/install/fedora.sh | 2 +- util/install/freebsd.sh | 2 +- util/install/msys2.sh | 2 +- util/install/void.sh | 2 +- util/qmk_install.sh | 7 +++++++ 22 files changed, 70 insertions(+), 78 deletions(-) diff --git a/lib/python/qmk/cli/cformat.py b/lib/python/qmk/cli/cformat.py index 15158d9c7d..efeb459676 100644 --- a/lib/python/qmk/cli/cformat.py +++ b/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) diff --git a/lib/python/qmk/cli/clean.py b/lib/python/qmk/cli/clean.py index 9096529fde..72b7ffe810 100644 --- a/lib/python/qmk/cli/clean.py +++ b/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 @@ from milc import cli 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) diff --git a/lib/python/qmk/cli/compile.py b/lib/python/qmk/cli/compile.py index 23ca4e00a6..7a45e77214 100755 --- a/lib/python/qmk/cli/compile.py +++ b/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 = {} diff --git a/lib/python/qmk/cli/doctor.py b/lib/python/qmk/cli/doctor.py index 4a2e2010f7..9e10570620 100755 --- a/lib/python/qmk/cli/doctor.py +++ b/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: diff --git a/lib/python/qmk/cli/flash.py b/lib/python/qmk/cli/flash.py index 1b67840616..1b2932a5b2 100644 --- a/lib/python/qmk/cli/flash.py +++ b/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: diff --git a/lib/python/qmk/cli/generate/docs.py b/lib/python/qmk/cli/generate/docs.py index a59a24db50..749336fea5 100644 --- a/lib/python/qmk/cli/generate/docs.py +++ b/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) diff --git a/lib/python/qmk/cli/multibuild.py b/lib/python/qmk/cli/multibuild.py index a4f0a0cc03..46594c0997 100755 --- a/lib/python/qmk/cli/multibuild.py +++ b/lib/python/qmk/cli/multibuild.py @@ -4,6 +4,7 @@ This will compile everything in parallel, for testing purposes. """ 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 @@ all: {keyboard_safe}_binary ) # 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) diff --git a/lib/python/qmk/cli/new/keyboard.py b/lib/python/qmk/cli/new/keyboard.py index cab0198fb7..ae4445ca48 100644 --- a/lib/python/qmk/cli/new/keyboard.py +++ b/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) diff --git a/lib/python/qmk/cli/pyformat.py b/lib/python/qmk/cli/pyformat.py index 02581f0d85..abe5f6de19 100755 --- a/lib/python/qmk/cli/pyformat.py +++ b/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: diff --git a/lib/python/qmk/cli/pytest.py b/lib/python/qmk/cli/pytest.py index 50a1d70a4f..bdb336b9a7 100644 --- a/lib/python/qmk/cli/pytest.py +++ b/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 @@ from milc import cli 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 diff --git a/lib/python/qmk/commands.py b/lib/python/qmk/commands.py index 8c3f95ea21..ee049e8af7 100644 --- a/lib/python/qmk/commands.py +++ b/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) diff --git a/lib/python/qmk/keymap.py b/lib/python/qmk/keymap.py index 4ad9ffb591..ac7951082e 100644 --- a/lib/python/qmk/keymap.py +++ b/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.token import Token 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 diff --git a/lib/python/qmk/os_helpers/__init__.py b/lib/python/qmk/os_helpers/__init__.py index 3f64a63a3a..3e98db3c32 100644 --- a/lib/python/qmk/os_helpers/__init__.py +++ b/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 diff --git a/lib/python/qmk/os_helpers/linux/__init__.py b/lib/python/qmk/os_helpers/linux/__init__.py index 9e73964e47..008654ab0f 100644 --- a/lib/python/qmk/os_helpers/linux/__init__.py +++ b/lib/python/qmk/os_helpers/linux/__init__.py @@ -5,7 +5,6 @@ import shutil 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: diff --git a/lib/python/qmk/submodules.py b/lib/python/qmk/submodules.py index be51a68043..6a272dae50 100644 --- a/lib/python/qmk/submodules.py +++ b/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) diff --git a/lib/python/qmk/tests/test_cli_commands.py b/lib/python/qmk/tests/test_cli_commands.py index 741551e5ec..d13e42d408 100644 --- a/lib/python/qmk/tests/test_cli_commands.py +++ b/lib/python/qmk/tests/test_cli_commands.py @@ -1,15 +1,14 @@ import platform +from subprocess import DEVNULL -from subprocess import STDOUT, PIPE - -from qmk.commands import run +from milc import cli is_windows = 'windows' in platform.platform().lower() def check_subcommand(command, *args): cmd = ['bin/qmk', command, *args] - result = run(cmd, stdout=PIPE, stderr=STDOUT, universal_newlines=True) + result = cli.run(cmd, stdin=DEVNULL, combined_output=True) return result @@ -18,7 +17,7 @@ def check_subcommand_stdin(file_to_read, command, *args): """ with open(file_to_read, encoding='utf-8') as my_file: cmd = ['bin/qmk', command, *args] - result = run(cmd, stdin=my_file, stdout=PIPE, stderr=STDOUT, universal_newlines=True) + result = cli.run(cmd, stdin=my_file, combined_output=True) return result diff --git a/util/install/debian.sh b/util/install/debian.sh index 0ae9764a33..885df723d9 100755 --- a/util/install/debian.sh +++ b/util/install/debian.sh @@ -5,7 +5,7 @@ DEBCONF_NONINTERACTIVE_SEEN=true export DEBIAN_FRONTEND DEBCONF_NONINTERACTIVE_SEEN _qmk_install_prepare() { - sudo apt-get update + sudo apt-get update $SKIP_PROMPT } _qmk_install() { diff --git a/util/install/fedora.sh b/util/install/fedora.sh index 44b71b98bf..d9452f68d5 100755 --- a/util/install/fedora.sh +++ b/util/install/fedora.sh @@ -4,7 +4,7 @@ _qmk_install() { echo "Installing dependencies" # TODO: Check whether devel/headers packages are really needed - sudo dnf -y install \ + sudo dnf $SKIP_PROMPT install \ clang diffutils git gcc glibc-headers kernel-devel kernel-headers make unzip wget zip \ python3 \ avr-binutils avr-gcc avr-libc \ diff --git a/util/install/freebsd.sh b/util/install/freebsd.sh index 353c52d647..d9399a5de9 100755 --- a/util/install/freebsd.sh +++ b/util/install/freebsd.sh @@ -1,7 +1,7 @@ #!/bin/bash _qmk_install_prepare() { - sudo pkg update + sudo pkg update $SKIP_PROMPT } _qmk_install() { diff --git a/util/install/msys2.sh b/util/install/msys2.sh index c8598a60fa..b1300a34d2 100755 --- a/util/install/msys2.sh +++ b/util/install/msys2.sh @@ -1,7 +1,7 @@ #!/bin/bash _qmk_install_prepare() { - pacman -Syu + pacman -Syu $MSYS2_CONFIRM } _qmk_install() { diff --git a/util/install/void.sh b/util/install/void.sh index 9ec7019e5c..c49c3ef63f 100755 --- a/util/install/void.sh +++ b/util/install/void.sh @@ -3,7 +3,7 @@ _qmk_install() { echo "Installing dependencies" - sudo xbps-install \ + sudo xbps-install $SKIP_PROMPT \ gcc git make wget unzip zip \ python3-pip \ avr-binutils avr-gcc avr-libc \ diff --git a/util/qmk_install.sh b/util/qmk_install.sh index 421a91a999..943069453a 100755 --- a/util/qmk_install.sh +++ b/util/qmk_install.sh @@ -2,6 +2,13 @@ QMK_FIRMWARE_DIR=$(cd -P -- "$(dirname -- "$0")/.." && pwd -P) QMK_FIRMWARE_UTIL_DIR=$QMK_FIRMWARE_DIR/util +if [ "$1" = "-y" ]; then + SKIP_PROMPT='-y' + MSYS2_CONFIRM='--noconfirm' +else + SKIP_PROMPT='' + MSYS2_CONFIRM='' +fi case $(uname -a) in *Darwin*)