diff --git a/news/7711.feature b/news/7711.feature new file mode 100644 index 00000000000..51168b5a4c0 --- /dev/null +++ b/news/7711.feature @@ -0,0 +1 @@ +Refactor ``call_subprocess`` by renaming it into ``call_subprocess_for_install`` and making it a wrapper of the former ``call_subprocess`` function. diff --git a/src/pip/_internal/build_env.py b/src/pip/_internal/build_env.py index b8f005f5ca9..0e794f53e83 100644 --- a/src/pip/_internal/build_env.py +++ b/src/pip/_internal/build_env.py @@ -17,7 +17,7 @@ from pip import __file__ as pip_location from pip._internal.cli.spinners import open_spinner -from pip._internal.utils.subprocess import call_subprocess +from pip._internal.utils.subprocess import call_subprocess_for_install from pip._internal.utils.temp_dir import TempDirectory, tempdir_kinds from pip._internal.utils.typing import MYPY_CHECK_RUNNING @@ -196,7 +196,7 @@ def install_requirements( args.append('--') args.extend(requirements) with open_spinner(message) as spinner: - call_subprocess(args, spinner=spinner) + call_subprocess_for_install(args, spinner=spinner) class NoOpBuildEnvironment(BuildEnvironment): diff --git a/src/pip/_internal/operations/build/metadata_legacy.py b/src/pip/_internal/operations/build/metadata_legacy.py index b6813f89ba7..b2f83841ada 100644 --- a/src/pip/_internal/operations/build/metadata_legacy.py +++ b/src/pip/_internal/operations/build/metadata_legacy.py @@ -7,7 +7,7 @@ from pip._internal.exceptions import InstallationError from pip._internal.utils.misc import ensure_dir from pip._internal.utils.setuptools_build import make_setuptools_egg_info_args -from pip._internal.utils.subprocess import call_subprocess +from pip._internal.utils.subprocess import call_subprocess_for_install from pip._internal.utils.typing import MYPY_CHECK_RUNNING from pip._internal.vcs import vcs @@ -112,7 +112,7 @@ def generate_metadata( ) with build_env: - call_subprocess( + call_subprocess_for_install( args, cwd=source_dir, command_desc='python setup.py egg_info', diff --git a/src/pip/_internal/operations/build/wheel_legacy.py b/src/pip/_internal/operations/build/wheel_legacy.py index 37dc876acbd..a84646a9315 100644 --- a/src/pip/_internal/operations/build/wheel_legacy.py +++ b/src/pip/_internal/operations/build/wheel_legacy.py @@ -7,7 +7,7 @@ ) from pip._internal.utils.subprocess import ( LOG_DIVIDER, - call_subprocess, + call_subprocess_for_install, format_command_args, ) from pip._internal.utils.typing import MYPY_CHECK_RUNNING @@ -94,7 +94,7 @@ def build_wheel_legacy( logger.debug('Destination directory: %s', tempd) try: - output = call_subprocess( + output = call_subprocess_for_install( wheel_args, cwd=source_dir, spinner=spinner, diff --git a/src/pip/_internal/operations/install/editable_legacy.py b/src/pip/_internal/operations/install/editable_legacy.py index a668a61dc60..8f4cda8ede0 100644 --- a/src/pip/_internal/operations/install/editable_legacy.py +++ b/src/pip/_internal/operations/install/editable_legacy.py @@ -4,7 +4,7 @@ from pip._internal.utils.logging import indent_log from pip._internal.utils.setuptools_build import make_setuptools_develop_args -from pip._internal.utils.subprocess import call_subprocess +from pip._internal.utils.subprocess import call_subprocess_for_install from pip._internal.utils.typing import MYPY_CHECK_RUNNING if MYPY_CHECK_RUNNING: @@ -46,7 +46,7 @@ def install_editable( with indent_log(): with build_env: - call_subprocess( + call_subprocess_for_install( args, cwd=unpacked_source_directory, ) diff --git a/src/pip/_internal/utils/subprocess.py b/src/pip/_internal/utils/subprocess.py index 55c82daea7c..722197a51c3 100644 --- a/src/pip/_internal/utils/subprocess.py +++ b/src/pip/_internal/utils/subprocess.py @@ -27,6 +27,18 @@ LOG_DIVIDER = '----------------------------------------' +class SubProcessResult(object): + def __init__(self, cmd, returncode, had_error, output): + # type: (Union[List[str], CommandArgs], int, bool, List[Text]) -> None + """ + Initialize a SubProcessResult + """ + self.cmd = cmd + self.returncode = returncode + self.had_error = had_error + self.output = output if output else [] + + def make_command(*args): # type: (Union[str, HiddenText, CommandArgs]) -> CommandArgs """ @@ -113,17 +125,17 @@ def make_subprocess_output_error( return msg -def call_subprocess( - cmd, # type: Union[List[str], CommandArgs] - show_stdout=False, # type: bool - cwd=None, # type: Optional[str] - on_returncode='raise', # type: str +def call_subprocess_for_install( + cmd, # type: Union[List[str], CommandArgs] + show_stdout=False, # type: bool + cwd=None, # type: Optional[str] + on_returncode='raise', # type: str extra_ok_returncodes=None, # type: Optional[Iterable[int]] - command_desc=None, # type: Optional[str] - extra_environ=None, # type: Optional[Mapping[str, Any]] - unset_environ=None, # type: Optional[Iterable[str]] - spinner=None, # type: Optional[SpinnerInterface] - log_failed_cmd=True # type: Optional[bool] + command_desc=None, # type: Optional[str] + extra_environ=None, # type: Optional[Mapping[str, Any]] + unset_environ=None, # type: Optional[Iterable[str]] + spinner=None, # type: Optional[SpinnerInterface] + log_failed_cmd=True # type: Optional[bool] ): # type: (...) -> Text """ @@ -136,10 +148,6 @@ def call_subprocess( prior to calling subprocess.Popen(). log_failed_cmd: if false, failed commands are not logged, only raised. """ - if extra_ok_returncodes is None: - extra_ok_returncodes = [] - if unset_environ is None: - unset_environ = [] # Most places in pip use show_stdout=False. What this means is-- # # - We connect the child's output (combined stderr and stdout) to a @@ -155,35 +163,84 @@ def call_subprocess( # # If show_stdout=True, then the above is still done, but with DEBUG # replaced by INFO. - if show_stdout: - # Then log the subprocess output at INFO level. - log_subprocess = subprocess_logger.info - used_level = logging.INFO - else: - # Then log the subprocess output using DEBUG. This also ensures - # it will be logged to the log file (aka user_log), if enabled. - log_subprocess = subprocess_logger.debug - used_level = logging.DEBUG - + used_level = logging.INFO if show_stdout else logging.DEBUG # Whether the subprocess will be visible in the console. showing_subprocess = subprocess_logger.getEffectiveLevel() <= used_level - - # Only use the spinner if we're not showing the subprocess output - # and we have a spinner. - use_spinner = not showing_subprocess and spinner is not None - if command_desc is None: command_desc = format_command_args(cmd) + proc = _call_subprocess( + cmd=cmd, cwd=cwd, spinner=spinner, command_desc=command_desc, + extra_environ=extra_environ, unset_environ=unset_environ, + log_failed_cmd=log_failed_cmd, log_level=used_level, + extra_ok_returncodes=extra_ok_returncodes) + if proc.had_error: + if on_returncode == "raise": + if not showing_subprocess and log_failed_cmd: + # Then the subprocess streams haven't been logged to the + # console yet. + msg = make_subprocess_output_error( + cmd_args=cmd, + cwd=cwd, + lines=proc.output, + exit_status=proc.returncode, + ) + subprocess_logger.error(msg) + exc_msg = ( + "Command errored out with exit status {}: {} " + "Check the logs for full command output." + ).format(proc.returncode, command_desc) + raise InstallationError(exc_msg) + elif on_returncode == "warn": + subprocess_logger.warning( + "Command '{}' had error code {} in {}".format( + command_desc, proc.returncode, cwd)) + elif on_returncode == "ignore": + pass + else: + raise ValueError( + "Invalid value: on_returncode={!r}".format(on_returncode)) + return "".join(proc.output) - log_subprocess("Running command %s", command_desc) + +def _call_subprocess( + cmd, # type: Union[List[str], CommandArgs] + spinner=None, # type: Optional[SpinnerInterface] + cwd=None, # type: Optional[str] + log_level=logging.DEBUG, # type: int + command_desc=None, # type: Optional[str] + extra_environ=None, # type: Optional[Mapping[str, Any]] + unset_environ=None, # type: Optional[Iterable[str]] + extra_ok_returncodes=None, # type: Optional[Iterable[int]] + log_failed_cmd=True # type: Optional[bool] +): + # type: (...) -> SubProcessResult + """ + This function calls the subprocess and returns a SubProcessResult + object representing the stdout lines, returncode, and command that + was executed. If subprocess logging is enabled, then display it + on stdout with the INFO logging level + """ + if unset_environ is None: + unset_environ = [] + if extra_ok_returncodes is None: + extra_ok_returncodes = [] + if log_level == logging.INFO: + log_subprocess = subprocess_logger.info + else: + log_subprocess = subprocess_logger.debug + showing_subprocess = subprocess_logger.getEffectiveLevel() <= log_level + use_spinner = not showing_subprocess and spinner is not None + log_subprocess("Running command {}".format(command_desc)) env = os.environ.copy() if extra_environ: env.update(extra_environ) for name in unset_environ: env.pop(name, None) + + all_output = [] try: + # Convert HiddenText objects to the underlying str. proc = subprocess.Popen( - # Convert HiddenText objects to the underlying str. reveal_command_args(cmd), stderr=subprocess.STDOUT, stdin=subprocess.PIPE, stdout=subprocess.PIPE, cwd=cwd, env=env, @@ -192,18 +249,16 @@ def call_subprocess( except Exception as exc: if log_failed_cmd: subprocess_logger.critical( - "Error %s while executing command %s", exc, command_desc, + "Error %s while executing command %s", exc, command_desc ) raise - all_output = [] while True: # The "line" value is a unicode string in Python 2. line = console_to_str(proc.stdout.readline()) if not line: break line = line.rstrip() - all_output.append(line + '\n') - + all_output.append(line + "\n") # Show the line immediately. log_subprocess(line) # Update the spinner. @@ -215,41 +270,17 @@ def call_subprocess( if proc.stdout: proc.stdout.close() proc_had_error = ( - proc.returncode and proc.returncode not in extra_ok_returncodes + proc.returncode != 0 and proc.returncode not in extra_ok_returncodes ) if use_spinner: if proc_had_error: spinner.finish("error") else: spinner.finish("done") - if proc_had_error: - if on_returncode == 'raise': - if not showing_subprocess and log_failed_cmd: - # Then the subprocess streams haven't been logged to the - # console yet. - msg = make_subprocess_output_error( - cmd_args=cmd, - cwd=cwd, - lines=all_output, - exit_status=proc.returncode, - ) - subprocess_logger.error(msg) - exc_msg = ( - 'Command errored out with exit status {}: {} ' - 'Check the logs for full command output.' - ).format(proc.returncode, command_desc) - raise InstallationError(exc_msg) - elif on_returncode == 'warn': - subprocess_logger.warning( - 'Command "{}" had error code {} in {}'.format( - command_desc, proc.returncode, cwd) - ) - elif on_returncode == 'ignore': - pass - else: - raise ValueError('Invalid value: on_returncode={!r}'.format( - on_returncode)) - return ''.join(all_output) + return SubProcessResult( + cmd=cmd, returncode=proc.returncode, + had_error=proc_had_error, output=all_output + ) def runner_with_spinner_message(message): @@ -267,7 +298,7 @@ def runner( ): # type: (...) -> None with open_spinner(message) as spinner: - call_subprocess( + call_subprocess_for_install( cmd, cwd=cwd, extra_environ=extra_environ, diff --git a/src/pip/_internal/vcs/subversion.py b/src/pip/_internal/vcs/subversion.py index 6c76d1ad435..466cde1c655 100644 --- a/src/pip/_internal/vcs/subversion.py +++ b/src/pip/_internal/vcs/subversion.py @@ -270,8 +270,8 @@ def get_remote_call_options(self): svn_version = self.get_vcs_version() # By default, Subversion >= 1.8 runs in non-interactive mode if # stdin is not a TTY. Since that is how pip invokes SVN, in - # call_subprocess(), pip must pass --force-interactive to ensure - # the user can be prompted for a password, if required. + # call_subprocess_for_install(), pip must pass --force-interactive + # to ensure the user can be prompted for a password, if required. # SVN added the --force-interactive option in SVN 1.8. Since # e.g. RHEL/CentOS 7, which is supported until 2024, ships with # SVN 1.7, pip should continue to support SVN 1.7. Therefore, pip diff --git a/src/pip/_internal/vcs/versioncontrol.py b/src/pip/_internal/vcs/versioncontrol.py index 7b6171451ac..df4d8696258 100644 --- a/src/pip/_internal/vcs/versioncontrol.py +++ b/src/pip/_internal/vcs/versioncontrol.py @@ -21,7 +21,10 @@ hide_value, rmtree, ) -from pip._internal.utils.subprocess import call_subprocess, make_command +from pip._internal.utils.subprocess import ( + call_subprocess_for_install, + make_command, +) from pip._internal.utils.typing import MYPY_CHECK_RUNNING from pip._internal.utils.urls import get_url_scheme @@ -279,7 +282,8 @@ class VersionControl(object): repo_name = '' # List of supported schemes for this Version Control schemes = () # type: Tuple[str, ...] - # Iterable of environment variable names to pass to call_subprocess(). + # Iterable of environment variable names to pass to + # call_subprocess_for_install(). unset_environ = () # type: Tuple[str, ...] default_arg_rev = None # type: Optional[str] @@ -665,19 +669,22 @@ def run_command( # type: (...) -> Text """ Run a VCS subcommand - This is simply a wrapper around call_subprocess that adds the VCS - command name, and checks that the VCS is available + This is simply a wrapper around call_subprocess_for_install + that adds the VCS command name, and checks that the VCS + is available """ cmd = make_command(cls.name, *cmd) try: - return call_subprocess(cmd, show_stdout, cwd, - on_returncode=on_returncode, - extra_ok_returncodes=extra_ok_returncodes, - command_desc=command_desc, - extra_environ=extra_environ, - unset_environ=cls.unset_environ, - spinner=spinner, - log_failed_cmd=log_failed_cmd) + return call_subprocess_for_install( + cmd, show_stdout, cwd, + on_returncode=on_returncode, + extra_ok_returncodes=extra_ok_returncodes, + command_desc=command_desc, + extra_environ=extra_environ, + unset_environ=cls.unset_environ, + spinner=spinner, + log_failed_cmd=log_failed_cmd + ) except OSError as e: # errno.ENOENT = no such file or directory # In other words, the VCS executable isn't available diff --git a/src/pip/_internal/wheel_builder.py b/src/pip/_internal/wheel_builder.py index 7c7820d4f26..5be8eee54a3 100644 --- a/src/pip/_internal/wheel_builder.py +++ b/src/pip/_internal/wheel_builder.py @@ -15,7 +15,7 @@ from pip._internal.utils.logging import indent_log from pip._internal.utils.misc import ensure_dir, hash_file, is_wheel_installed from pip._internal.utils.setuptools_build import make_setuptools_clean_args -from pip._internal.utils.subprocess import call_subprocess +from pip._internal.utils.subprocess import call_subprocess_for_install from pip._internal.utils.temp_dir import TempDirectory from pip._internal.utils.typing import MYPY_CHECK_RUNNING from pip._internal.utils.urls import path_to_url @@ -246,7 +246,7 @@ def _clean_one_legacy(req, global_options): logger.info('Running setup.py clean for %s', req.name) try: - call_subprocess(clean_args, cwd=req.source_dir) + call_subprocess_for_install(clean_args, cwd=req.source_dir) return True except Exception: logger.error('Failed cleaning build dir for %s', req.name) diff --git a/tests/unit/test_utils_subprocess.py b/tests/unit/test_utils_subprocess.py index b0de2bf578d..83081f2e593 100644 --- a/tests/unit/test_utils_subprocess.py +++ b/tests/unit/test_utils_subprocess.py @@ -10,7 +10,7 @@ from pip._internal.exceptions import InstallationError from pip._internal.utils.misc import hide_value from pip._internal.utils.subprocess import ( - call_subprocess, + call_subprocess_for_install, format_command_args, make_command, make_subprocess_output_error, @@ -170,7 +170,7 @@ def finish(self, final_status): class TestCallSubprocess(object): """ - Test call_subprocess(). + Test call_subprocess_for_install(). """ def check_result( @@ -178,18 +178,19 @@ def check_result( expected_spinner, ): """ - Check the result of calling call_subprocess(). + Check the result of calling call_subprocess_for_install(). :param log_level: the logging level that caplog was set to. - :param spinner: the FakeSpinner object passed to call_subprocess() - to be checked. - :param result: the call_subprocess() return value to be checked. + :param spinner: the FakeSpinner object passed to + call_subprocess_for_install() to be checked. + :param result: the call_subprocess_for_install() + return value to be checked. :param expected: a pair (expected_proc, expected_records), where 1) `expected_proc` is the expected return value of - call_subprocess() as a list of lines, or None if the return - value is expected to be None; + call_subprocess_for_install() as a list of lines, or None + if the return value is expected to be None; 2) `expected_records` is the expected value of - caplog.record_tuples. + caplog.record_tuples. :param expected_spinner: a 2-tuple of the spinner's expected (spin_count, final_status). """ @@ -237,7 +238,7 @@ def test_debug_logging(self, capfd, caplog): """ log_level = DEBUG args, spinner = self.prepare_call(caplog, log_level) - result = call_subprocess(args, spinner=spinner) + result = call_subprocess_for_install(args, spinner=spinner) expected = (['Hello', 'world'], [ ('pip.subprocessor', DEBUG, 'Running command '), @@ -257,7 +258,7 @@ def test_info_logging(self, capfd, caplog): """ log_level = INFO args, spinner = self.prepare_call(caplog, log_level) - result = call_subprocess(args, spinner=spinner) + result = call_subprocess_for_install(args, spinner=spinner) expected = (['Hello', 'world'], []) # The spinner should spin twice in this case since the subprocess @@ -277,7 +278,7 @@ def test_info_logging__subprocess_error(self, capfd, caplog): args, spinner = self.prepare_call(caplog, log_level, command=command) with pytest.raises(InstallationError) as exc: - call_subprocess(args, spinner=spinner) + call_subprocess_for_install(args, spinner=spinner) result = None exc_message = str(exc.value) assert exc_message.startswith( @@ -328,7 +329,9 @@ def test_info_logging_with_show_stdout_true(self, capfd, caplog): """ log_level = INFO args, spinner = self.prepare_call(caplog, log_level) - result = call_subprocess(args, spinner=spinner, show_stdout=True) + result = call_subprocess_for_install( + args, spinner=spinner, show_stdout=True + ) expected = (['Hello', 'world'], [ ('pip.subprocessor', INFO, 'Running command '), @@ -380,7 +383,7 @@ def test_spinner_finish( ) args, spinner = self.prepare_call(caplog, log_level, command=command) try: - call_subprocess( + call_subprocess_for_install( args, show_stdout=show_stdout, extra_ok_returncodes=extra_ok_returncodes, @@ -397,7 +400,7 @@ def test_spinner_finish( def test_closes_stdin(self): with pytest.raises(InstallationError): - call_subprocess( + call_subprocess_for_install( [sys.executable, '-c', 'input()'], show_stdout=True, ) diff --git a/tests/unit/test_vcs.py b/tests/unit/test_vcs.py index 42fc43d6855..5b7533302a6 100644 --- a/tests/unit/test_vcs.py +++ b/tests/unit/test_vcs.py @@ -512,9 +512,11 @@ def test_subversion__get_remote_call_options( class TestSubversionArgs(TestCase): def setUp(self): - patcher = patch('pip._internal.vcs.versioncontrol.call_subprocess') + patcher = patch( + 'pip._internal.vcs.versioncontrol.call_subprocess_for_install' + ) self.addCleanup(patcher.stop) - self.call_subprocess_mock = patcher.start() + self.call_subprocess_for_install_mock = patcher.start() # Test Data. self.url = 'svn+http://username:password@svn.example.com/' @@ -525,7 +527,7 @@ def setUp(self): self.dest = '/tmp/test' def assert_call_args(self, args): - assert self.call_subprocess_mock.call_args[0][0] == args + assert self.call_subprocess_for_install_mock.call_args[0][0] == args def test_obtain(self): self.svn.obtain(self.dest, hide_url(self.url))