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>
master
Zach White 2021-05-19 15:24:46 -07:00 committed by GitHub
parent a9aec546c8
commit db1eacdaac
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
22 changed files with 70 additions and 78 deletions

View File

@ -1,8 +1,8 @@
"""Format C code according to QMK's style. """Format C code according to QMK's style.
""" """
import subprocess
from os import path from os import path
from shutil import which from shutil import which
from subprocess import CalledProcessError, DEVNULL, Popen, PIPE
from argcomplete.completers import FilesCompleter from argcomplete.completers import FilesCompleter
from milc import cli from milc import cli
@ -34,7 +34,7 @@ def find_diffs(files):
for file in files: for file in files:
cli.log.debug('Checking for changes in %s', file) 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) 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: if diff.returncode != 0:
@ -51,11 +51,11 @@ def cformat_run(files):
clang_format = [find_clang_format(), '-i'] clang_format = [find_clang_format(), '-i']
try: 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.') cli.log.info('Successfully formatted the C code.')
return True return True
except subprocess.CalledProcessError as e: except CalledProcessError as e:
cli.log.error('Error formatting C code!') cli.log.error('Error formatting C code!')
cli.log.debug('%s exited with returncode %s', e.cmd, e.returncode) cli.log.debug('%s exited with returncode %s', e.cmd, e.returncode)
cli.log.debug('STDOUT:') cli.log.debug('STDOUT:')
@ -111,7 +111,7 @@ def cformat(cli):
else: else:
git_diff_cmd = ['git', 'diff', '--name-only', cli.args.base_branch, *core_dirs] 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: if git_diff.returncode != 0:
cli.log.error("Error running %s", git_diff_cmd) cli.log.error("Error running %s", git_diff_cmd)

View File

@ -1,6 +1,8 @@
"""Clean the QMK firmware folder of build artifacts. """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 from milc import cli
@ -9,4 +11,4 @@ from milc import cli
def clean(cli): def clean(cli):
"""Runs `make clean` (or `make distclean` if --all is passed) """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)

View File

@ -2,6 +2,8 @@
You can compile a keymap already in the repo or using a QMK Configurator export. 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 argcomplete.completers import FilesCompleter
from milc import cli 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: 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') 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, stdin=DEVNULL)
cli.run(command, capture_output=False, text=False)
# Build the environment vars # Build the environment vars
envs = {} envs = {}

View File

@ -3,12 +3,12 @@
Check out the user's QMK environment and make sure it's ready to compile. Check out the user's QMK environment and make sure it's ready to compile.
""" """
import platform import platform
from subprocess import DEVNULL
from milc import cli from milc import cli
from milc.questions import yesno from milc.questions import yesno
from qmk import submodules from qmk import submodules
from qmk.constants import QMK_FIRMWARE 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 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 not bin_ok:
if yesno('Would you like to install dependencies?', default=True): 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() bin_ok = check_binaries()
if bin_ok: if bin_ok:

View File

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

View File

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

View File

@ -4,6 +4,7 @@ This will compile everything in parallel, for testing purposes.
""" """
import re import re
from pathlib import Path from pathlib import Path
from subprocess import DEVNULL
from milc import cli from milc import cli
@ -35,7 +36,7 @@ def multibuild(cli):
make_cmd = _find_make() make_cmd = _find_make()
if cli.args.clean: 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' builddir = Path(QMK_FIRMWARE) / '.build'
makefile = builddir / 'parallel_kb_builds.mk' makefile = builddir / 'parallel_kb_builds.mk'
@ -75,4 +76,4 @@ all: {keyboard_safe}_binary
) )
# yapf: enable # 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)

View File

@ -8,4 +8,4 @@ def new_keyboard(cli):
"""Creates a new keyboard """Creates a new keyboard
""" """
# TODO: replace this bodge to the existing script # 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)

View File

@ -1,8 +1,8 @@
"""Format python code according to QMK's style. """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.") @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' edit = '--diff' if cli.args.dry_run else '--in-place'
yapf_cmd = ['yapf', '-vv', '--recursive', edit, 'bin/qmk', 'lib/python'] yapf_cmd = ['yapf', '-vv', '--recursive', edit, 'bin/qmk', 'lib/python']
try: 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.') cli.log.info('Python code in `bin/qmk` and `lib/python` is correctly formatted.')
return True return True
except subprocess.CalledProcessError: except CalledProcessError:
if cli.args.dry_run: if cli.args.dry_run:
cli.log.error('Python code in `bin/qmk` and `lib/python` incorrectly formatted!') cli.log.error('Python code in `bin/qmk` and `lib/python` incorrectly formatted!')
else: else:

View File

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

View File

@ -2,11 +2,9 @@
""" """
import json import json
import os import os
import platform
import subprocess
import shlex
import shutil import shutil
from pathlib import Path from pathlib import Path
from subprocess import DEVNULL
from time import strftime from time import strftime
from milc import cli 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'] git_describe_cmd = ['git', 'describe', '--abbrev=6', '--dirty', '--always', '--tags']
if Path(check_dir).exists(): 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: if git_describe.returncode == 0:
return git_describe.stdout.strip() 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']] user_keymap['layout'] = aliases[orig_keyboard]['layouts'][user_keymap['layout']]
return user_keymap 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)

View File

@ -1,9 +1,9 @@
"""Functions that help you work with QMK keymaps. """Functions that help you work with QMK keymaps.
""" """
import json import json
import subprocess
import sys import sys
from pathlib import Path from pathlib import Path
from subprocess import DEVNULL
import argcomplete import argcomplete
from milc import cli from milc import cli
@ -12,7 +12,6 @@ from pygments.token import Token
from pygments import lex from pygments import lex
import qmk.path import qmk.path
import qmk.commands
from qmk.keyboard import find_keyboard_from_dir, rules_mk from qmk.keyboard import find_keyboard_from_dir, rules_mk
# The `keymap.c` template to use when a keyboard doesn't have its own # 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) return sorted(names)
def _c_preprocess(path, stdin=None): def _c_preprocess(path, stdin=DEVNULL):
""" Run a file through the C pre-processor """ Run a file through the C pre-processor
Args: Args:
@ -371,7 +370,9 @@ def _c_preprocess(path, stdin=None):
Returns: Returns:
the stdout of the pre-processor 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 return pre_processed_keymap.stdout

View File

@ -3,10 +3,9 @@
from enum import Enum from enum import Enum
import re import re
import shutil import shutil
import subprocess from subprocess import DEVNULL
from milc import cli from milc import cli
from qmk.commands import run
from qmk import submodules from qmk import submodules
from qmk.constants import QMK_FIRMWARE from qmk.constants import QMK_FIRMWARE
@ -142,7 +141,7 @@ def is_executable(command):
# Make sure the command can be executed # Make sure the command can be executed
version_arg = ESSENTIAL_BINARIES[command].get('version_arg', '--version') 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 ESSENTIAL_BINARIES[command]['output'] = check.stdout

View File

@ -5,7 +5,6 @@ import shutil
from milc import cli from milc import cli
from qmk.constants import QMK_FIRMWARE from qmk.constants import QMK_FIRMWARE
from qmk.commands import run
from qmk.os_helpers import CheckStatus from qmk.os_helpers import CheckStatus
@ -132,7 +131,7 @@ def check_modem_manager():
""" """
if check_systemd(): 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: if mm_check.returncode == 0:
return True return True
else: else:

View File

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

View File

@ -1,15 +1,14 @@
import platform import platform
from subprocess import DEVNULL
from subprocess import STDOUT, PIPE from milc import cli
from qmk.commands import run
is_windows = 'windows' in platform.platform().lower() is_windows = 'windows' in platform.platform().lower()
def check_subcommand(command, *args): def check_subcommand(command, *args):
cmd = ['bin/qmk', 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 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: with open(file_to_read, encoding='utf-8') as my_file:
cmd = ['bin/qmk', command, *args] 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 return result

View File

@ -5,7 +5,7 @@ DEBCONF_NONINTERACTIVE_SEEN=true
export DEBIAN_FRONTEND DEBCONF_NONINTERACTIVE_SEEN export DEBIAN_FRONTEND DEBCONF_NONINTERACTIVE_SEEN
_qmk_install_prepare() { _qmk_install_prepare() {
sudo apt-get update sudo apt-get update $SKIP_PROMPT
} }
_qmk_install() { _qmk_install() {

View File

@ -4,7 +4,7 @@ _qmk_install() {
echo "Installing dependencies" echo "Installing dependencies"
# TODO: Check whether devel/headers packages are really needed # 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 \ clang diffutils git gcc glibc-headers kernel-devel kernel-headers make unzip wget zip \
python3 \ python3 \
avr-binutils avr-gcc avr-libc \ avr-binutils avr-gcc avr-libc \

View File

@ -1,7 +1,7 @@
#!/bin/bash #!/bin/bash
_qmk_install_prepare() { _qmk_install_prepare() {
sudo pkg update sudo pkg update $SKIP_PROMPT
} }
_qmk_install() { _qmk_install() {

View File

@ -1,7 +1,7 @@
#!/bin/bash #!/bin/bash
_qmk_install_prepare() { _qmk_install_prepare() {
pacman -Syu pacman -Syu $MSYS2_CONFIRM
} }
_qmk_install() { _qmk_install() {

View File

@ -3,7 +3,7 @@
_qmk_install() { _qmk_install() {
echo "Installing dependencies" echo "Installing dependencies"
sudo xbps-install \ sudo xbps-install $SKIP_PROMPT \
gcc git make wget unzip zip \ gcc git make wget unzip zip \
python3-pip \ python3-pip \
avr-binutils avr-gcc avr-libc \ avr-binutils avr-gcc avr-libc \

View File

@ -2,6 +2,13 @@
QMK_FIRMWARE_DIR=$(cd -P -- "$(dirname -- "$0")/.." && pwd -P) QMK_FIRMWARE_DIR=$(cd -P -- "$(dirname -- "$0")/.." && pwd -P)
QMK_FIRMWARE_UTIL_DIR=$QMK_FIRMWARE_DIR/util 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 case $(uname -a) in
*Darwin*) *Darwin*)