From d2772c22931fb051ef330436e59719ce3a05fe2c Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Sun, 14 Feb 2021 12:56:33 -0500 Subject: [PATCH] refactor: subprocess.run (#592) * change check_call to run * refactor: change check_output to run * Apply suggestions from code review Co-authored-by: Joe Rickerby Co-authored-by: Joe Rickerby --- bin/run_tests.py | 4 ++-- bin/update_dependencies.py | 10 +++++----- cibuildwheel/bashlex_eval.py | 2 +- cibuildwheel/linux.py | 3 ++- cibuildwheel/macos.py | 20 +++++++++++--------- cibuildwheel/windows.py | 8 ++++---- test/test_macos_archs.py | 6 ++++-- test/test_projects/__main__.py | 6 +++--- test/utils.py | 9 ++++++--- 9 files changed, 38 insertions(+), 30 deletions(-) diff --git a/bin/run_tests.py b/bin/run_tests.py index 9e8e9075..94bc00c9 100755 --- a/bin/run_tests.py +++ b/bin/run_tests.py @@ -14,7 +14,7 @@ if __name__ == '__main__': # run the docker unit tests only on Linux if sys.platform.startswith('linux'): unit_test_args += ['--run-docker'] - subprocess.check_call(unit_test_args) + subprocess.run(unit_test_args, check=True) # run the integration tests - subprocess.check_call([sys.executable, '-m', 'pytest', '-x', '--durations', '0', '--timeout=2400', 'test']) + subprocess.run([sys.executable, '-m', 'pytest', '-x', '--durations', '0', '--timeout=2400', 'test'], check=True) diff --git a/bin/update_dependencies.py b/bin/update_dependencies.py index bd113eb5..937a0572 100755 --- a/bin/update_dependencies.py +++ b/bin/update_dependencies.py @@ -20,20 +20,20 @@ PYTHON_VERSIONS = ['27', '35', '36', '37', '38', '39'] if '--no-docker' in sys.argv: for python_version in PYTHON_VERSIONS: - subprocess.check_call([ + subprocess.run([ f'./env{python_version}/bin/pip-compile', '--allow-unsafe', '--upgrade', 'cibuildwheel/resources/constraints.in', '--output-file', f'cibuildwheel/resources/constraints-python{python_version}.txt' - ]) + ], check=True) else: image_runner = 'quay.io/pypa/manylinux2010_x86_64:latest' - subprocess.check_call(['docker', 'pull', image_runner]) + subprocess.run(['docker', 'pull', image_runner], check=True) for python_version in PYTHON_VERSIONS: abi_flags = '' if int(python_version) >= 38 else 'm' python_path = f'/opt/python/cp{python_version}-cp{python_version}{abi_flags}/bin/' - subprocess.check_call([ + subprocess.run([ 'docker', 'run', '--rm', '-e', 'CUSTOM_COMPILE_COMMAND', '-v', f'{os.getcwd()}:/volume', @@ -43,7 +43,7 @@ else: f'{python_path}pip-compile --allow-unsafe --upgrade ' 'cibuildwheel/resources/constraints.in ' f'--output-file cibuildwheel/resources/constraints-python{python_version}.txt' - ]) + ], check=True) # default constraints.txt shutil.copyfile(f'cibuildwheel/resources/constraints-python{PYTHON_VERSIONS[-1]}.txt', 'cibuildwheel/resources/constraints.txt') diff --git a/cibuildwheel/bashlex_eval.py b/cibuildwheel/bashlex_eval.py index 26fe1f5b..d82b3269 100644 --- a/cibuildwheel/bashlex_eval.py +++ b/cibuildwheel/bashlex_eval.py @@ -8,7 +8,7 @@ EnvironmentExecutor = Callable[[List[str], Dict[str, str]], str] def local_environment_executor(command: List[str], env: Dict[str, str]) -> str: - return subprocess.check_output(command, env=env, universal_newlines=True) + return subprocess.run(command, env=env, universal_newlines=True, stdout=subprocess.PIPE, check=True).stdout class NodeExecutionContext(NamedTuple): diff --git a/cibuildwheel/linux.py b/cibuildwheel/linux.py index 5d19dbd2..1e4e6f73 100644 --- a/cibuildwheel/linux.py +++ b/cibuildwheel/linux.py @@ -49,7 +49,8 @@ def get_python_configurations( def build(options: BuildOptions) -> None: try: - subprocess.check_output(['docker', '--version']) + # check docker is installed + subprocess.run(['docker', '--version'], check=True, stdout=subprocess.DEVNULL) except Exception: print('cibuildwheel: Docker not found. Docker is required to run Linux builds. ' 'If you\'re building on Travis CI, add `services: [docker]` to your .travis.yml.' diff --git a/cibuildwheel/macos.py b/cibuildwheel/macos.py index 3f23a330..a51d0b5e 100644 --- a/cibuildwheel/macos.py +++ b/cibuildwheel/macos.py @@ -28,14 +28,14 @@ from .util import ( ) -def call(args: Sequence[PathOrStr], env: Optional[Dict[str, str]] = None, cwd: Optional[str] = None, shell: bool = False) -> int: +def call(args: Sequence[PathOrStr], env: Optional[Dict[str, str]] = None, cwd: Optional[str] = None, shell: bool = False) -> None: # print the command executing for the logs if shell: print(f'+ {args}') else: print('+ ' + ' '.join(shlex.quote(str(a)) for a in args)) - return subprocess.check_call(args, env=env, cwd=cwd, shell=shell) + subprocess.run(args, env=env, cwd=cwd, shell=shell, check=True) def get_macos_version() -> Tuple[int, int]: @@ -53,10 +53,12 @@ def get_macos_version() -> Tuple[int, int]: def get_macos_sdks() -> List[str]: - output = subprocess.check_output( + output = subprocess.run( ['xcodebuild', '-showsdks'], universal_newlines=True, - ) + check=True, + stdout=subprocess.PIPE, + ).stdout return [m.group(1) for m in re.finditer(r'-sdk (macosx\S+)', output)] @@ -125,7 +127,7 @@ def make_symlinks(installation_bin_path: Path, python_executable: str, pip_execu def install_cpython(version: str, url: str) -> Path: - installed_system_packages = subprocess.check_output(['pkgutil', '--pkgs'], universal_newlines=True).splitlines() + installed_system_packages = subprocess.run(['pkgutil', '--pkgs'], universal_newlines=True, check=True, stdout=subprocess.PIPE).stdout.splitlines() # if this version of python isn't installed, get it from python.org and install python_package_identifier = f'org.python.Python.PythonFramework-{version}' @@ -211,7 +213,7 @@ def setup_python(python_configuration: PythonConfiguration, # check what version we're on call(['which', 'python'], env=env) call(['python', '--version'], env=env) - which_python = subprocess.check_output(['which', 'python'], env=env, universal_newlines=True).strip() + which_python = subprocess.run(['which', 'python'], env=env, universal_newlines=True, check=True, stdout=subprocess.PIPE).stdout.strip() if which_python != '/tmp/cibw_bin/python': print("cibuildwheel: python available on PATH doesn't match our installed instance. If you have modified PATH, ensure that you don't overwrite cibuildwheel's entry or insert python above it.", file=sys.stderr) sys.exit(1) @@ -221,7 +223,7 @@ def setup_python(python_configuration: PythonConfiguration, assert (installation_bin_path / 'pip').exists() call(['which', 'pip'], env=env) call(['pip', '--version'], env=env) - which_pip = subprocess.check_output(['which', 'pip'], env=env, universal_newlines=True).strip() + which_pip = subprocess.run(['which', 'pip'], env=env, universal_newlines=True, check=True, stdout=subprocess.PIPE).stdout.strip() if which_pip != '/tmp/cibw_bin/pip': print("cibuildwheel: pip available on PATH doesn't match our installed instance. If you have modified PATH, ensure that you don't overwrite cibuildwheel's entry or insert pip above it.", file=sys.stderr) sys.exit(1) @@ -413,12 +415,12 @@ def build(options: BuildOptions) -> None: raise RuntimeError("don't know how to emulate {testing_arch} on {machine_arch}") # define a custom 'call' function that adds the arch prefix each time - def call_with_arch(args: Sequence[PathOrStr], **kwargs: Any) -> int: + def call_with_arch(args: Sequence[PathOrStr], **kwargs: Any) -> None: if isinstance(args, str): args = ' '.join(arch_prefix) + ' ' + args else: args = [*arch_prefix, *args] - return call(args, **kwargs) + call(args, **kwargs) # Use --no-download to ensure determinism by using seed libraries # built into virtualenv diff --git a/cibuildwheel/windows.py b/cibuildwheel/windows.py index 45f0141a..fce0838b 100644 --- a/cibuildwheel/windows.py +++ b/cibuildwheel/windows.py @@ -33,12 +33,12 @@ def call(args: Sequence[PathOrStr], env: Optional[Dict[str, str]] = None, print('+ ' + ' '.join(str(a) for a in args)) # we use shell=True here, even though we don't need a shell due to a bug # https://bugs.python.org/issue8557 - subprocess.check_call([str(a) for a in args], env=env, cwd=cwd, shell=True) + subprocess.run([str(a) for a in args], env=env, cwd=cwd, shell=True, check=True) def shell(command: str, env: Optional[Dict[str, str]] = None, cwd: Optional[str] = None) -> None: print(f'+ {command}') - subprocess.check_call(command, env=env, cwd=cwd, shell=True) + subprocess.run(command, env=env, cwd=cwd, shell=True, check=True) def get_nuget_args(version: str, arch: str) -> List[str]: @@ -152,7 +152,7 @@ def setup_python(python_configuration: PythonConfiguration, dependency_constrain call(['where', 'python'], env=env) call(['python', '--version'], env=env) call(['python', '-c', '"import struct; print(struct.calcsize(\'P\') * 8)"'], env=env) - where_python = subprocess.check_output(['where', 'python'], env=env, universal_newlines=True).splitlines()[0].strip() + where_python = subprocess.run(['where', 'python'], env=env, universal_newlines=True, check=True, stdout=subprocess.PIPE).stdout.splitlines()[0].strip() if where_python != str(installation_path / 'python.exe'): print("cibuildwheel: python available on PATH doesn't match our installed instance. If you have modified PATH, ensure that you don't overwrite cibuildwheel's entry or insert python above it.", file=sys.stderr) sys.exit(1) @@ -161,7 +161,7 @@ def setup_python(python_configuration: PythonConfiguration, dependency_constrain if not (installation_path / 'Scripts' / 'pip.exe').exists(): call(['python', get_pip_script, *dependency_constraint_flags], env=env, cwd="C:\\cibw") assert (installation_path / 'Scripts' / 'pip.exe').exists() - where_pip = subprocess.check_output(['where', 'pip'], env=env, universal_newlines=True).splitlines()[0].strip() + where_pip = subprocess.run(['where', 'pip'], env=env, universal_newlines=True, check=True, stdout=subprocess.PIPE).stdout.splitlines()[0].strip() if where_pip.strip() != str(installation_path / 'Scripts' / 'pip.exe'): print("cibuildwheel: pip available on PATH doesn't match our installed instance. If you have modified PATH, ensure that you don't overwrite cibuildwheel's entry or insert pip above it.", file=sys.stderr) sys.exit(1) diff --git a/test/test_macos_archs.py b/test/test_macos_archs.py index de2ada1c..51256adc 100644 --- a/test/test_macos_archs.py +++ b/test/test_macos_archs.py @@ -15,10 +15,12 @@ ALL_MACOS_WHEELS = ( def get_xcode_version() -> Tuple[int, int]: - output = subprocess.check_output( + output = subprocess.run( ['xcodebuild', '-version'], universal_newlines=True, - ) + check=True, + stdout=subprocess.PIPE, + ).stdout lines = output.splitlines() _, version_str = lines[0].split() diff --git a/test/test_projects/__main__.py b/test/test_projects/__main__.py index 070cc2a2..efd7d7d3 100644 --- a/test/test_projects/__main__.py +++ b/test/test_projects/__main__.py @@ -34,11 +34,11 @@ def main(): if options.open: if sys.platform == 'darwin': - subprocess.check_call(['open', '--', project_dir]) + subprocess.run(['open', '--', project_dir], check=True) elif sys.platform == 'linux2': - subprocess.check_call(['xdg-open', '--', project_dir]) + subprocess.run(['xdg-open', '--', project_dir], check=True) elif sys.platform == 'win32': - subprocess.check_call(['explorer', project_dir]) + subprocess.run(['explorer', project_dir], check=True) if __name__ == '__main__': diff --git a/test/utils.py b/test/utils.py index 8dcb2f60..dc87f638 100644 --- a/test/utils.py +++ b/test/utils.py @@ -31,11 +31,13 @@ def cibuildwheel_get_build_identifiers(project_path, env=None): Returns the list of build identifiers that cibuildwheel will try to build for the current platform. ''' - cmd_output = subprocess.check_output( + cmd_output = subprocess.run( [sys.executable, '-m', 'cibuildwheel', '--print-build-identifiers', str(project_path)], universal_newlines=True, env=env, - ) + check=True, + stdout=subprocess.PIPE, + ).stdout return cmd_output.strip().split('\n') @@ -64,10 +66,11 @@ def cibuildwheel_run(project_path, package_dir='.', env=None, add_env=None, outp env.update(add_env) with TemporaryDirectoryIfNone(output_dir) as _output_dir: - subprocess.check_call( + subprocess.run( [sys.executable, '-m', 'cibuildwheel', '--output-dir', str(_output_dir), str(package_dir)], env=env, cwd=project_path, + check=True, ) wheels = os.listdir(_output_dir) return wheels