From 62d03c8e752ee35057031a91d7028e0a2e5d43e4 Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Mon, 25 Apr 2022 12:39:09 -0700 Subject: [PATCH] ansible-test - Fix subprocess management. (#77638) * Run code-smell sanity tests in UTF-8 Mode. * Update subprocess use in sanity test programs. * Use raw_command instead of run_command with always=True set. * Add more capture=True usage. * Don't expose stdin to subprocesses. * Capture more output. Warn on retry. * Add more captures. * Capture coverage cli output. * Capture windows and network host checks. * Be explicit about interactive usage. * Use a shell for non-captured, non-interactive subprocesses. * Add integration test to assert no TTY. * Add unit test to assert no TTY. * Require blocking stdin/stdout/stderr. * Use subprocess.run in ansible-core sanity tests. * Remove unused arg. * Be explicit with subprocess.run check=False. * Add changelog. --- .../ansible-test-subprocess-isolation.yml | 10 +++++++++ .../targets/ansible-test-no-tty/aliases | 2 ++ .../targets/ansible-test-no-tty/runme.py | 7 ++++++ .../targets/ansible-test-no-tty/runme.sh | 5 +++++ .../ansible_test/_internal/ansible_util.py | 6 ++--- .../_internal/commands/coverage/__init__.py | 11 +++++++++- .../_internal/commands/coverage/combine.py | 4 ++-- .../commands/integration/cloud/cs.py | 2 +- .../_internal/commands/sanity/__init__.py | 1 + .../_internal/commands/sanity/pylint.py | 2 +- .../commands/sanity/validate_modules.py | 2 +- .../_internal/commands/shell/__init__.py | 6 ++++- test/lib/ansible_test/_internal/config.py | 1 + .../lib/ansible_test/_internal/connections.py | 16 +++++++++----- test/lib/ansible_test/_internal/core_ci.py | 2 +- test/lib/ansible_test/_internal/delegation.py | 13 ++++++----- .../lib/ansible_test/_internal/docker_util.py | 8 ++++--- .../ansible_test/_internal/host_profiles.py | 10 +++++---- test/lib/ansible_test/_internal/util.py | 20 +++++++++++++++-- .../lib/ansible_test/_internal/util_common.py | 4 +++- test/lib/ansible_test/_internal/venv.py | 13 ++++++----- .../controller/sanity/code-smell/changelog.py | 6 ++++- .../validate-modules/validate_modules/main.py | 16 +++++++------- .../validate_modules/module_args.py | 8 +++---- .../_util/target/cli/ansible_test_cli_stub.py | 3 +++ test/sanity/code-smell/docs-build.py | 7 +++--- test/sanity/code-smell/package-data.py | 22 ++++++++++--------- test/units/test_no_tty.py | 7 ++++++ 28 files changed, 148 insertions(+), 66 deletions(-) create mode 100644 changelogs/fragments/ansible-test-subprocess-isolation.yml create mode 100644 test/integration/targets/ansible-test-no-tty/aliases create mode 100755 test/integration/targets/ansible-test-no-tty/runme.py create mode 100755 test/integration/targets/ansible-test-no-tty/runme.sh create mode 100644 test/units/test_no_tty.py diff --git a/changelogs/fragments/ansible-test-subprocess-isolation.yml b/changelogs/fragments/ansible-test-subprocess-isolation.yml new file mode 100644 index 00000000000..3be259d6089 --- /dev/null +++ b/changelogs/fragments/ansible-test-subprocess-isolation.yml @@ -0,0 +1,10 @@ +bugfixes: + - ansible-test - Subprocesses are now isolated from the stdin, stdout and stderr of ansible-test. + This avoids issues with subprocesses tampering with the file descriptors, such as SSH making them non-blocking. + As a result of this change, subprocess output from unit and integration tests on stderr now go to stdout. + - ansible-test - Subprocesses no longer have access to the TTY ansible-test is connected to, if any. + This maintains consistent behavior between local testing and CI systems, which typically do not provide a TTY. + Tests which require a TTY should use pexpect or another mechanism to create a PTY. +minor_changes: + - ansible-test - Blocking mode is now enforced for stdin, stdout and stderr. + If any of these are non-blocking then ansible-test will exit during startup with an error. diff --git a/test/integration/targets/ansible-test-no-tty/aliases b/test/integration/targets/ansible-test-no-tty/aliases new file mode 100644 index 00000000000..0ac86c9200c --- /dev/null +++ b/test/integration/targets/ansible-test-no-tty/aliases @@ -0,0 +1,2 @@ +context/controller +shippable/posix/group1 diff --git a/test/integration/targets/ansible-test-no-tty/runme.py b/test/integration/targets/ansible-test-no-tty/runme.py new file mode 100755 index 00000000000..c8c5cfccce6 --- /dev/null +++ b/test/integration/targets/ansible-test-no-tty/runme.py @@ -0,0 +1,7 @@ +#!/usr/bin/env python + +import sys + +assert not sys.stdin.isatty() +assert not sys.stdout.isatty() +assert not sys.stderr.isatty() diff --git a/test/integration/targets/ansible-test-no-tty/runme.sh b/test/integration/targets/ansible-test-no-tty/runme.sh new file mode 100755 index 00000000000..bae5220e2c2 --- /dev/null +++ b/test/integration/targets/ansible-test-no-tty/runme.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash + +set -eux + +./runme.py diff --git a/test/lib/ansible_test/_internal/ansible_util.py b/test/lib/ansible_test/_internal/ansible_util.py index a3582dc89f4..c6f231bc7dc 100644 --- a/test/lib/ansible_test/_internal/ansible_util.py +++ b/test/lib/ansible_test/_internal/ansible_util.py @@ -22,11 +22,11 @@ from .util import ( ANSIBLE_SOURCE_ROOT, ANSIBLE_TEST_TOOLS_ROOT, get_ansible_version, + raw_command, ) from .util_common import ( create_temp_dir, - run_command, ResultType, intercept_python, get_injector_path, @@ -258,12 +258,12 @@ class CollectionDetailError(ApplicationError): self.reason = reason -def get_collection_detail(args, python): # type: (EnvironmentConfig, PythonConfig) -> CollectionDetail +def get_collection_detail(python): # type: (PythonConfig) -> CollectionDetail """Return collection detail.""" collection = data_context().content.collection directory = os.path.join(collection.root, collection.directory) - stdout = run_command(args, [python.path, os.path.join(ANSIBLE_TEST_TOOLS_ROOT, 'collection_detail.py'), directory], capture=True, always=True)[0] + stdout = raw_command([python.path, os.path.join(ANSIBLE_TEST_TOOLS_ROOT, 'collection_detail.py'), directory], capture=True)[0] result = json.loads(stdout) error = result.get('error') diff --git a/test/lib/ansible_test/_internal/commands/coverage/__init__.py b/test/lib/ansible_test/_internal/commands/coverage/__init__.py index 8cae31298c1..3d27b64bb6e 100644 --- a/test/lib/ansible_test/_internal/commands/coverage/__init__.py +++ b/test/lib/ansible_test/_internal/commands/coverage/__init__.py @@ -100,7 +100,16 @@ def run_coverage(args, host_state, output_file, command, cmd): # type: (Coverag cmd = ['python', '-m', 'coverage.__main__', command, '--rcfile', COVERAGE_CONFIG_PATH] + cmd - intercept_python(args, host_state.controller_profile.python, cmd, env) + stdout, stderr = intercept_python(args, host_state.controller_profile.python, cmd, env, capture=True) + + stdout = (stdout or '').strip() + stderr = (stderr or '').strip() + + if stdout: + display.info(stdout) + + if stderr: + display.warning(stderr) def get_all_coverage_files(): # type: () -> t.List[str] diff --git a/test/lib/ansible_test/_internal/commands/coverage/combine.py b/test/lib/ansible_test/_internal/commands/coverage/combine.py index 3356cc857c9..ea1851a1c9d 100644 --- a/test/lib/ansible_test/_internal/commands/coverage/combine.py +++ b/test/lib/ansible_test/_internal/commands/coverage/combine.py @@ -18,11 +18,11 @@ from ...util import ( ANSIBLE_TEST_TOOLS_ROOT, display, ApplicationError, + raw_command, ) from ...util_common import ( ResultType, - run_command, write_json_file, write_json_test_results, ) @@ -196,7 +196,7 @@ def _command_coverage_combine_powershell(args): # type: (CoverageCombineConfig) cmd = ['pwsh', os.path.join(ANSIBLE_TEST_TOOLS_ROOT, 'coverage_stub.ps1')] cmd.extend(source_paths) - stubs = json.loads(run_command(args, cmd, capture=True, always=True)[0]) + stubs = json.loads(raw_command(cmd, capture=True)[0]) return dict((d['Path'], dict((line, 0) for line in d['Lines'])) for d in stubs) diff --git a/test/lib/ansible_test/_internal/commands/integration/cloud/cs.py b/test/lib/ansible_test/_internal/commands/integration/cloud/cs.py index f20a7d887ef..8ffcabfb32e 100644 --- a/test/lib/ansible_test/_internal/commands/integration/cloud/cs.py +++ b/test/lib/ansible_test/_internal/commands/integration/cloud/cs.py @@ -106,7 +106,7 @@ class CsCloudProvider(CloudProvider): # apply work-around for OverlayFS issue # https://github.com/docker/for-linux/issues/72#issuecomment-319904698 - docker_exec(self.args, self.DOCKER_SIMULATOR_NAME, ['find', '/var/lib/mysql', '-type', 'f', '-exec', 'touch', '{}', ';']) + docker_exec(self.args, self.DOCKER_SIMULATOR_NAME, ['find', '/var/lib/mysql', '-type', 'f', '-exec', 'touch', '{}', ';'], capture=True) if self.args.explain: values = dict( diff --git a/test/lib/ansible_test/_internal/commands/sanity/__init__.py b/test/lib/ansible_test/_internal/commands/sanity/__init__.py index d819c37e33f..7b7e6196fee 100644 --- a/test/lib/ansible_test/_internal/commands/sanity/__init__.py +++ b/test/lib/ansible_test/_internal/commands/sanity/__init__.py @@ -952,6 +952,7 @@ class SanityCodeSmellTest(SanitySingleVersion): cmd = [python.path, self.path] env = ansible_environment(args, color=False) + env.update(PYTHONUTF8='1') # force all code-smell sanity tests to run with Python UTF-8 Mode enabled pattern = None data = None diff --git a/test/lib/ansible_test/_internal/commands/sanity/pylint.py b/test/lib/ansible_test/_internal/commands/sanity/pylint.py index 0e6ace8ea99..370e8998438 100644 --- a/test/lib/ansible_test/_internal/commands/sanity/pylint.py +++ b/test/lib/ansible_test/_internal/commands/sanity/pylint.py @@ -141,7 +141,7 @@ class PylintTest(SanitySingleVersion): if data_context().content.collection: try: - collection_detail = get_collection_detail(args, python) + collection_detail = get_collection_detail(python) if not collection_detail.version: display.warning('Skipping pylint collection version checks since no collection version was found.') diff --git a/test/lib/ansible_test/_internal/commands/sanity/validate_modules.py b/test/lib/ansible_test/_internal/commands/sanity/validate_modules.py index e0fbac6439e..f1d448804c1 100644 --- a/test/lib/ansible_test/_internal/commands/sanity/validate_modules.py +++ b/test/lib/ansible_test/_internal/commands/sanity/validate_modules.py @@ -121,7 +121,7 @@ class ValidateModulesTest(SanitySingleVersion): cmd.extend(['--collection', data_context().content.collection.directory]) try: - collection_detail = get_collection_detail(args, python) + collection_detail = get_collection_detail(python) if collection_detail.version: cmd.extend(['--collection-version', collection_detail.version]) diff --git a/test/lib/ansible_test/_internal/commands/shell/__init__.py b/test/lib/ansible_test/_internal/commands/shell/__init__.py index 4b205171a38..c518af94c9d 100644 --- a/test/lib/ansible_test/_internal/commands/shell/__init__.py +++ b/test/lib/ansible_test/_internal/commands/shell/__init__.py @@ -2,6 +2,7 @@ from __future__ import annotations import os +import sys import typing as t from ...util import ( @@ -44,6 +45,9 @@ def command_shell(args): # type: (ShellConfig) -> None if args.raw and isinstance(args.targets[0], ControllerConfig): raise ApplicationError('The --raw option has no effect on the controller.') + if not sys.stdin.isatty(): + raise ApplicationError('Standard input must be a TTY to launch a shell.') + host_state = prepare_profiles(args, skip_setup=args.raw) # shell if args.delegate: @@ -87,4 +91,4 @@ def command_shell(args): # type: (ShellConfig) -> None else: cmd = [] - con.run(cmd) + con.run(cmd, interactive=True) diff --git a/test/lib/ansible_test/_internal/config.py b/test/lib/ansible_test/_internal/config.py index 0a14a806ca8..5da5fc00e4d 100644 --- a/test/lib/ansible_test/_internal/config.py +++ b/test/lib/ansible_test/_internal/config.py @@ -238,6 +238,7 @@ class ShellConfig(EnvironmentConfig): super().__init__(args, 'shell') self.raw = args.raw # type: bool + self.interactive = True class SanityConfig(TestConfig): diff --git a/test/lib/ansible_test/_internal/connections.py b/test/lib/ansible_test/_internal/connections.py index 14234b2d936..b0c7d948ad1 100644 --- a/test/lib/ansible_test/_internal/connections.py +++ b/test/lib/ansible_test/_internal/connections.py @@ -3,7 +3,6 @@ from __future__ import annotations import abc import shlex -import sys import tempfile import typing as t @@ -47,6 +46,7 @@ class Connection(metaclass=abc.ABCMeta): def run(self, command, # type: t.List[str] capture=False, # type: bool + interactive=False, # type: bool data=None, # type: t.Optional[str] stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] @@ -60,7 +60,7 @@ class Connection(metaclass=abc.ABCMeta): """Extract the given archive file stream in the specified directory.""" tar_cmd = ['tar', 'oxzf', '-', '-C', chdir] - retry(lambda: self.run(tar_cmd, stdin=src)) + retry(lambda: self.run(tar_cmd, stdin=src, capture=True)) def create_archive(self, chdir, # type: str @@ -82,7 +82,7 @@ class Connection(metaclass=abc.ABCMeta): sh_cmd = ['sh', '-c', ' | '.join(' '.join(shlex.quote(cmd) for cmd in command) for command in commands)] - retry(lambda: self.run(sh_cmd, stdout=dst)) + retry(lambda: self.run(sh_cmd, stdout=dst, capture=True)) class LocalConnection(Connection): @@ -93,6 +93,7 @@ class LocalConnection(Connection): def run(self, command, # type: t.List[str] capture=False, # type: bool + interactive=False, # type: bool data=None, # type: t.Optional[str] stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] @@ -105,6 +106,7 @@ class LocalConnection(Connection): data=data, stdin=stdin, stdout=stdout, + interactive=interactive, ) @@ -131,6 +133,7 @@ class SshConnection(Connection): def run(self, command, # type: t.List[str] capture=False, # type: bool + interactive=False, # type: bool data=None, # type: t.Optional[str] stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] @@ -143,7 +146,7 @@ class SshConnection(Connection): options.append('-q') - if not data and not stdin and not stdout and sys.stdin.isatty(): + if interactive: options.append('-tt') with tempfile.NamedTemporaryFile(prefix='ansible-test-ssh-debug-', suffix='.log') as ssh_logfile: @@ -166,6 +169,7 @@ class SshConnection(Connection): data=data, stdin=stdin, stdout=stdout, + interactive=interactive, error_callback=error_callback, ) @@ -209,6 +213,7 @@ class DockerConnection(Connection): def run(self, command, # type: t.List[str] capture=False, # type: bool + interactive=False, # type: bool data=None, # type: t.Optional[str] stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] @@ -219,7 +224,7 @@ class DockerConnection(Connection): if self.user: options.extend(['--user', self.user]) - if not data and not stdin and not stdout and sys.stdin.isatty(): + if interactive: options.append('-it') return docker_exec( @@ -231,6 +236,7 @@ class DockerConnection(Connection): data=data, stdin=stdin, stdout=stdout, + interactive=interactive, ) def inspect(self): # type: () -> DockerInspect diff --git a/test/lib/ansible_test/_internal/core_ci.py b/test/lib/ansible_test/_internal/core_ci.py index dbb428aeebf..37e4ac061f0 100644 --- a/test/lib/ansible_test/_internal/core_ci.py +++ b/test/lib/ansible_test/_internal/core_ci.py @@ -469,7 +469,7 @@ class SshKey: make_dirs(os.path.dirname(key)) if not os.path.isfile(key) or not os.path.isfile(pub): - run_command(args, ['ssh-keygen', '-m', 'PEM', '-q', '-t', self.KEY_TYPE, '-N', '', '-f', key]) + run_command(args, ['ssh-keygen', '-m', 'PEM', '-q', '-t', self.KEY_TYPE, '-N', '', '-f', key], capture=True) if args.explain: return key, pub diff --git a/test/lib/ansible_test/_internal/delegation.py b/test/lib/ansible_test/_internal/delegation.py index 6298bf24058..7ee5bd971db 100644 --- a/test/lib/ansible_test/_internal/delegation.py +++ b/test/lib/ansible_test/_internal/delegation.py @@ -160,11 +160,12 @@ def delegate_command(args, host_state, exclude, require): # type: (EnvironmentC os.path.join(content_root, ResultType.COVERAGE.relative_path), ] - con.run(['mkdir', '-p'] + writable_dirs) - con.run(['chmod', '777'] + writable_dirs) - con.run(['chmod', '755', working_directory]) - con.run(['chmod', '644', os.path.join(content_root, args.metadata_path)]) - con.run(['useradd', pytest_user, '--create-home']) + con.run(['mkdir', '-p'] + writable_dirs, capture=True) + con.run(['chmod', '777'] + writable_dirs, capture=True) + con.run(['chmod', '755', working_directory], capture=True) + con.run(['chmod', '644', os.path.join(content_root, args.metadata_path)], capture=True) + con.run(['useradd', pytest_user, '--create-home'], capture=True) + con.run(insert_options(command, options + ['--requirements-mode', 'only'])) container = con.inspect() @@ -191,7 +192,7 @@ def delegate_command(args, host_state, exclude, require): # type: (EnvironmentC success = False try: - con.run(insert_options(command, options)) + con.run(insert_options(command, options), interactive=args.interactive) success = True finally: if host_delegation: diff --git a/test/lib/ansible_test/_internal/docker_util.py b/test/lib/ansible_test/_internal/docker_util.py index 12509076776..b8bc838c4be 100644 --- a/test/lib/ansible_test/_internal/docker_util.py +++ b/test/lib/ansible_test/_internal/docker_util.py @@ -279,7 +279,7 @@ def docker_pull(args, image): # type: (EnvironmentConfig, str) -> None def docker_cp_to(args, container_id, src, dst): # type: (EnvironmentConfig, str, str, str) -> None """Copy a file to the specified container.""" - docker_command(args, ['cp', src, '%s:%s' % (container_id, dst)]) + docker_command(args, ['cp', src, '%s:%s' % (container_id, dst)], capture=True) def docker_run( @@ -514,6 +514,7 @@ def docker_exec( capture=False, # type: bool stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] + interactive=False, # type: bool data=None, # type: t.Optional[str] ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Execute the given command in the specified container.""" @@ -523,7 +524,7 @@ def docker_exec( if data or stdin or stdout: options.append('-i') - return docker_command(args, ['exec'] + options + [container_id] + cmd, capture=capture, stdin=stdin, stdout=stdout, data=data) + return docker_command(args, ['exec'] + options + [container_id] + cmd, capture=capture, stdin=stdin, stdout=stdout, interactive=interactive, data=data) def docker_info(args): # type: (CommonConfig) -> t.Dict[str, t.Any] @@ -544,6 +545,7 @@ def docker_command( capture=False, # type: bool stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] + interactive=False, # type: bool always=False, # type: bool data=None, # type: t.Optional[str] ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] @@ -552,7 +554,7 @@ def docker_command( command = [require_docker().command] if command[0] == 'podman' and _get_podman_remote(): command.append('--remote') - return run_command(args, command + cmd, env=env, capture=capture, stdin=stdin, stdout=stdout, always=always, data=data) + return run_command(args, command + cmd, env=env, capture=capture, stdin=stdin, stdout=stdout, interactive=interactive, always=always, data=data) def docker_environment(): # type: () -> t.Dict[str, str] diff --git a/test/lib/ansible_test/_internal/host_profiles.py b/test/lib/ansible_test/_internal/host_profiles.py index 9079c7e9244..e5a3dbd8256 100644 --- a/test/lib/ansible_test/_internal/host_profiles.py +++ b/test/lib/ansible_test/_internal/host_profiles.py @@ -484,8 +484,9 @@ class NetworkRemoteProfile(RemoteProfile[NetworkRemoteConfig]): for dummy in range(1, 90): try: - intercept_python(self.args, self.args.controller_python, cmd, env) - except SubprocessError: + intercept_python(self.args, self.args.controller_python, cmd, env, capture=True) + except SubprocessError as ex: + display.warning(str(ex)) time.sleep(10) else: return @@ -717,8 +718,9 @@ class WindowsRemoteProfile(RemoteProfile[WindowsRemoteConfig]): for dummy in range(1, 120): try: - intercept_python(self.args, self.args.controller_python, cmd, env) - except SubprocessError: + intercept_python(self.args, self.args.controller_python, cmd, env, capture=True) + except SubprocessError as ex: + display.warning(str(ex)) time.sleep(10) else: return diff --git a/test/lib/ansible_test/_internal/util.py b/test/lib/ansible_test/_internal/util.py index 9f1b7a4fa77..a0fd6fa70d9 100644 --- a/test/lib/ansible_test/_internal/util.py +++ b/test/lib/ansible_test/_internal/util.py @@ -261,6 +261,7 @@ def raw_command( explain=False, # type: bool stdin=None, # type: t.Optional[t.Union[t.IO[bytes], int]] stdout=None, # type: t.Optional[t.Union[t.IO[bytes], int]] + interactive=False, # type: bool cmd_verbosity=1, # type: int str_errors='strict', # type: str error_callback=None, # type: t.Optional[t.Callable[[SubprocessError], None]] @@ -274,6 +275,14 @@ def raw_command( cmd = list(cmd) + if not capture and not interactive: + # When not capturing stdout/stderr and not running interactively, send subprocess stdout/stderr through a shell to the current stdout. + # Additionally send output through a pipe to `tee` to block access to the current TTY, if any. + # This prevents subprocesses from sharing stdin/stdout/stderr with the current process or each other. + # Doing so allows subprocesses to safely make changes to their file handles, such as making them non-blocking (ssh does this). + # This also maintains consistency between local testing and CI systems, which typically do not provide a TTY. + cmd = ['/bin/sh', '-c', f'{shlex.join(cmd)} 2>&1 | tee'] + escaped_cmd = ' '.join(shlex.quote(c) for c in cmd) display.info('Run command: %s' % escaped_cmd, verbosity=cmd_verbosity, truncate=True) @@ -298,6 +307,10 @@ def raw_command( elif data is not None: stdin = subprocess.PIPE communicate = True + elif interactive: + pass # allow the subprocess access to our stdin + else: + stdin = subprocess.DEVNULL if stdout: communicate = True @@ -654,12 +667,15 @@ class MissingEnvironmentVariable(ApplicationError): self.name = name -def retry(func, ex_type=SubprocessError, sleep=10, attempts=10): +def retry(func, ex_type=SubprocessError, sleep=10, attempts=10, warn=True): """Retry the specified function on failure.""" for dummy in range(1, attempts): try: return func() - except ex_type: + except ex_type as ex: + if warn: + display.warning(str(ex)) + time.sleep(sleep) return func() diff --git a/test/lib/ansible_test/_internal/util_common.py b/test/lib/ansible_test/_internal/util_common.py index 99d22c2bcae..2f68a6dfe5e 100644 --- a/test/lib/ansible_test/_internal/util_common.py +++ b/test/lib/ansible_test/_internal/util_common.py @@ -126,6 +126,7 @@ class CommonConfig: """Configuration common to all commands.""" def __init__(self, args, command): # type: (t.Any, str) -> None self.command = command + self.interactive = False self.success = None # type: t.Optional[bool] self.color = args.color # type: bool @@ -406,13 +407,14 @@ def run_command( always=False, # type: bool stdin=None, # type: t.Optional[t.IO[bytes]] stdout=None, # type: t.Optional[t.IO[bytes]] + interactive=False, # type: bool cmd_verbosity=1, # type: int str_errors='strict', # type: str error_callback=None, # type: t.Optional[t.Callable[[SubprocessError], None]] ): # type: (...) -> t.Tuple[t.Optional[str], t.Optional[str]] """Run the specified command and return stdout and stderr as a tuple.""" explain = args.explain and not always - return raw_command(cmd, capture=capture, env=env, data=data, cwd=cwd, explain=explain, stdin=stdin, stdout=stdout, + return raw_command(cmd, capture=capture, env=env, data=data, cwd=cwd, explain=explain, stdin=stdin, stdout=stdout, interactive=interactive, cmd_verbosity=cmd_verbosity, str_errors=str_errors, error_callback=error_callback) diff --git a/test/lib/ansible_test/_internal/venv.py b/test/lib/ansible_test/_internal/venv.py index 64d8d04ce11..21dded627cb 100644 --- a/test/lib/ansible_test/_internal/venv.py +++ b/test/lib/ansible_test/_internal/venv.py @@ -20,6 +20,7 @@ from .util import ( remove_tree, ApplicationError, str_to_version, + raw_command, ) from .util_common import ( @@ -92,7 +93,7 @@ def create_virtual_environment(args, # type: EnvironmentConfig # creating a virtual environment using 'venv' when running in a virtual environment created by 'virtualenv' results # in a copy of the original virtual environment instead of creation of a new one # avoid this issue by only using "real" python interpreters to invoke 'venv' - for real_python in iterate_real_pythons(args, python.version): + for real_python in iterate_real_pythons(python.version): if run_venv(args, real_python, system_site_packages, pip, path): display.info('Created Python %s virtual environment using "venv": %s' % (python.version, path), verbosity=1) return True @@ -128,7 +129,7 @@ def create_virtual_environment(args, # type: EnvironmentConfig return False -def iterate_real_pythons(args, version): # type: (EnvironmentConfig, str) -> t.Iterable[str] +def iterate_real_pythons(version): # type: (str) -> t.Iterable[str] """ Iterate through available real python interpreters of the requested version. The current interpreter will be checked and then the path will be searched. @@ -138,7 +139,7 @@ def iterate_real_pythons(args, version): # type: (EnvironmentConfig, str) -> t. if version_info == sys.version_info[:len(version_info)]: current_python = sys.executable - real_prefix = get_python_real_prefix(args, current_python) + real_prefix = get_python_real_prefix(current_python) if real_prefix: current_python = find_python(version, os.path.join(real_prefix, 'bin')) @@ -159,7 +160,7 @@ def iterate_real_pythons(args, version): # type: (EnvironmentConfig, str) -> t. if found_python == current_python: return - real_prefix = get_python_real_prefix(args, found_python) + real_prefix = get_python_real_prefix(found_python) if real_prefix: found_python = find_python(version, os.path.join(real_prefix, 'bin')) @@ -168,12 +169,12 @@ def iterate_real_pythons(args, version): # type: (EnvironmentConfig, str) -> t. yield found_python -def get_python_real_prefix(args, python_path): # type: (EnvironmentConfig, str) -> t.Optional[str] +def get_python_real_prefix(python_path): # type: (str) -> t.Optional[str] """ Return the real prefix of the specified interpreter or None if the interpreter is not a virtual environment created by 'virtualenv'. """ cmd = [python_path, os.path.join(os.path.join(ANSIBLE_TEST_TARGET_TOOLS_ROOT, 'virtualenvcheck.py'))] - check_result = json.loads(run_command(args, cmd, capture=True, always=True)[0]) + check_result = json.loads(raw_command(cmd, capture=True)[0]) real_prefix = check_result['real_prefix'] return real_prefix diff --git a/test/lib/ansible_test/_util/controller/sanity/code-smell/changelog.py b/test/lib/ansible_test/_util/controller/sanity/code-smell/changelog.py index fe2ba5e3727..983eaeb4266 100644 --- a/test/lib/ansible_test/_util/controller/sanity/code-smell/changelog.py +++ b/test/lib/ansible_test/_util/controller/sanity/code-smell/changelog.py @@ -47,7 +47,11 @@ def main(): env = os.environ.copy() env.update(PYTHONPATH='%s:%s' % (os.path.join(os.path.dirname(__file__), 'changelog'), env['PYTHONPATH'])) - subprocess.call(cmd, env=env) # ignore the return code, rely on the output instead + # ignore the return code, rely on the output instead + process = subprocess.run(cmd, stdin=subprocess.DEVNULL, capture_output=True, text=True, env=env, check=False) + + sys.stdout.write(process.stdout) + sys.stderr.write(process.stderr) if __name__ == '__main__': diff --git a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py index 0bdd9dee21c..f18477d529b 100644 --- a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py +++ b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py @@ -436,14 +436,13 @@ class ModuleValidator(Validator): base_path = self._get_base_branch_module_path() command = ['git', 'show', '%s:%s' % (self.base_branch, base_path or self.path)] - p = subprocess.Popen(command, stdout=subprocess.PIPE, - stderr=subprocess.PIPE) - stdout, stderr = p.communicate() + p = subprocess.run(command, stdin=subprocess.DEVNULL, capture_output=True, check=False) + if int(p.returncode) != 0: return None t = tempfile.NamedTemporaryFile(delete=False) - t.write(stdout) + t.write(p.stdout) t.close() return t.name @@ -2456,11 +2455,12 @@ class GitCache: @staticmethod def _git(args): cmd = ['git'] + args - p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) - stdout, stderr = p.communicate() + p = subprocess.run(cmd, stdin=subprocess.DEVNULL, capture_output=True, text=True, check=False) + if p.returncode != 0: - raise GitError(stderr, p.returncode) - return stdout.decode('utf-8').splitlines() + raise GitError(p.stderr, p.returncode) + + return p.stdout.splitlines() class GitError(Exception): diff --git a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/module_args.py b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/module_args.py index ee938142ff8..03a14019274 100644 --- a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/module_args.py +++ b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/module_args.py @@ -122,14 +122,12 @@ def get_ps_argument_spec(filename, collection): }) script_path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'ps_argspec.ps1') - proc = subprocess.Popen(['pwsh', script_path, util_manifest], stdout=subprocess.PIPE, stderr=subprocess.PIPE, - shell=False) - stdout, stderr = proc.communicate() + proc = subprocess.run(['pwsh', script_path, util_manifest], stdin=subprocess.DEVNULL, capture_output=True, text=True, check=False) if proc.returncode != 0: - raise AnsibleModuleImportError("STDOUT:\n%s\nSTDERR:\n%s" % (stdout.decode('utf-8'), stderr.decode('utf-8'))) + raise AnsibleModuleImportError("STDOUT:\n%s\nSTDERR:\n%s" % (proc.stdout, proc.stderr)) - kwargs = json.loads(stdout) + kwargs = json.loads(proc.stdout) # the validate-modules code expects the options spec to be under the argument_spec key not options as set in PS kwargs['argument_spec'] = kwargs.pop('options', {}) diff --git a/test/lib/ansible_test/_util/target/cli/ansible_test_cli_stub.py b/test/lib/ansible_test/_util/target/cli/ansible_test_cli_stub.py index 95209493089..930654fc1e7 100755 --- a/test/lib/ansible_test/_util/target/cli/ansible_test_cli_stub.py +++ b/test/lib/ansible_test/_util/target/cli/ansible_test_cli_stub.py @@ -27,6 +27,9 @@ def main(args=None): raise SystemExit('This version of ansible-test cannot be executed with Python version %s. Supported Python versions are: %s' % ( version_to_str(sys.version_info[:3]), ', '.join(CONTROLLER_PYTHON_VERSIONS))) + if any(not os.get_blocking(handle.fileno()) for handle in (sys.stdin, sys.stdout, sys.stderr)): + raise SystemExit('Standard input, output and error file handles must be blocking to run ansible-test.') + # noinspection PyProtectedMember from ansible_test._internal import main as cli_main diff --git a/test/sanity/code-smell/docs-build.py b/test/sanity/code-smell/docs-build.py index 9461620a9a3..aaa69378c7f 100644 --- a/test/sanity/code-smell/docs-build.py +++ b/test/sanity/code-smell/docs-build.py @@ -29,13 +29,12 @@ def main(): try: cmd = ['make', 'core_singlehtmldocs'] - sphinx = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=docs_dir) - stdout, stderr = sphinx.communicate() + sphinx = subprocess.run(cmd, stdin=subprocess.DEVNULL, capture_output=True, cwd=docs_dir, check=False, text=True) finally: shutil.move(tmp, requirements_txt) - stdout = stdout.decode('utf-8') - stderr = stderr.decode('utf-8') + stdout = sphinx.stdout + stderr = sphinx.stderr if sphinx.returncode != 0: sys.stderr.write("Command '%s' failed with status code: %d\n" % (' '.join(cmd), sphinx.returncode)) diff --git a/test/sanity/code-smell/package-data.py b/test/sanity/code-smell/package-data.py index 110b1635628..e707b0eb4f5 100644 --- a/test/sanity/code-smell/package-data.py +++ b/test/sanity/code-smell/package-data.py @@ -172,14 +172,15 @@ def clean_repository(file_list): def create_sdist(tmp_dir): """Create an sdist in the repository""" - create = subprocess.Popen( + create = subprocess.run( ['make', 'snapshot', 'SDIST_DIR=%s' % tmp_dir], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - universal_newlines=True, + stdin=subprocess.DEVNULL, + capture_output=True, + text=True, + check=False, ) - stderr = create.communicate()[1] + stderr = create.stderr if create.returncode != 0: raise Exception('make snapshot failed:\n%s' % stderr) @@ -220,15 +221,16 @@ def extract_sdist(sdist_path, tmp_dir): def install_sdist(tmp_dir, sdist_dir): """Install the extracted sdist into the temporary directory""" - install = subprocess.Popen( + install = subprocess.run( ['python', 'setup.py', 'install', '--root=%s' % tmp_dir], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - universal_newlines=True, + stdin=subprocess.DEVNULL, + capture_output=True, + text=True, cwd=os.path.join(tmp_dir, sdist_dir), + check=False, ) - stdout, stderr = install.communicate() + stdout, stderr = install.stdout, install.stderr if install.returncode != 0: raise Exception('sdist install failed:\n%s' % stderr) diff --git a/test/units/test_no_tty.py b/test/units/test_no_tty.py new file mode 100644 index 00000000000..290c0b922ab --- /dev/null +++ b/test/units/test_no_tty.py @@ -0,0 +1,7 @@ +import sys + + +def test_no_tty(): + assert not sys.stdin.isatty() + assert not sys.stdout.isatty() + assert not sys.stderr.isatty()