diff --git a/news/6290.feature b/news/6290.feature new file mode 100644 index 00000000000..86ba68e0207 --- /dev/null +++ b/news/6290.feature @@ -0,0 +1,2 @@ +Command arguments in ``subprocess`` log messages are now quoted using +``shlex.quote()``. diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py index 79e4bad777c..3140c09c5b1 100644 --- a/src/pip/_internal/utils/misc.py +++ b/src/pip/_internal/utils/misc.py @@ -23,7 +23,7 @@ # why we ignore the type on this import. from pip._vendor.retrying import retry # type: ignore from pip._vendor.six import PY2 -from pip._vendor.six.moves import input +from pip._vendor.six.moves import input, shlex_quote from pip._vendor.six.moves.urllib import parse as urllib_parse from pip._vendor.six.moves.urllib.parse import unquote as urllib_unquote @@ -67,6 +67,8 @@ logger = std_logging.getLogger(__name__) +LOG_DIVIDER = '----------------------------------------' + WHEEL_EXTENSION = '.whl' BZ2_EXTENSIONS = ('.tar.bz2', '.tbz') XZ_EXTENSIONS = ('.tar.xz', '.txz', '.tlz', '.tar.lz', '.tar.lzma') @@ -648,6 +650,14 @@ def unpack_file( ) +def format_command_args(args): + # type: (List[str]) -> str + """ + Format command arguments for display. + """ + return ' '.join(shlex_quote(arg) for arg in args) + + def call_subprocess( cmd, # type: List[str] show_stdout=False, # type: bool @@ -697,12 +707,8 @@ def call_subprocess( else: stdout = subprocess.PIPE if command_desc is None: - cmd_parts = [] - for part in cmd: - if ' ' in part or '\n' in part or '"' in part or "'" in part: - part = '"%s"' % part.replace('"', '\\"') - cmd_parts.append(part) - command_desc = ' '.join(cmd_parts) + command_desc = format_command_args(cmd) + logger.debug("Running command %s", command_desc) env = os.environ.copy() if extra_environ: @@ -752,10 +758,8 @@ def call_subprocess( logger.info( 'Complete output from command %s:', command_desc, ) - logger.info( - ''.join(all_output) + - '\n----------------------------------------' - ) + # The all_output value already ends in a newline. + logger.info(''.join(all_output) + LOG_DIVIDER) raise InstallationError( 'Command "%s" failed with error code %s in %s' % (command_desc, proc.returncode, cwd)) diff --git a/src/pip/_internal/wheel.py b/src/pip/_internal/wheel.py index 1227412cc44..208d76da75c 100644 --- a/src/pip/_internal/wheel.py +++ b/src/pip/_internal/wheel.py @@ -33,7 +33,8 @@ from pip._internal.models.link import Link from pip._internal.utils.logging import indent_log from pip._internal.utils.misc import ( - call_subprocess, captured_stdout, ensure_dir, read_chunks, + LOG_DIVIDER, call_subprocess, captured_stdout, ensure_dir, + format_command_args, read_chunks, ) from pip._internal.utils.setuptools_build import SETUPTOOLS_SHIM from pip._internal.utils.temp_dir import TempDirectory @@ -786,7 +787,7 @@ def should_use_ephemeral_cache( return True -def format_command( +def format_command_result( command_args, # type: List[str] command_output, # type: str ): @@ -794,7 +795,8 @@ def format_command( """ Format command information for logging. """ - text = 'Command arguments: {}\n'.format(command_args) + command_desc = format_command_args(command_args) + text = 'Command arguments: {}\n'.format(command_desc) if not command_output: text += 'Command output: None' @@ -803,10 +805,7 @@ def format_command( else: if not command_output.endswith('\n'): command_output += '\n' - text += ( - 'Command output:\n{}' - '-----------------------------------------' - ).format(command_output) + text += 'Command output:\n{}{}'.format(command_output, LOG_DIVIDER) return text @@ -828,7 +827,7 @@ def get_legacy_build_wheel_path( msg = ( 'Legacy build of wheel for {!r} created no files.\n' ).format(req.name) - msg += format_command(command_args, command_output) + msg += format_command_result(command_args, command_output) logger.warning(msg) return None @@ -837,7 +836,7 @@ def get_legacy_build_wheel_path( 'Legacy build of wheel for {!r} created more than one file.\n' 'Filenames (choosing first): {}\n' ).format(req.name, names) - msg += format_command(command_args, command_output) + msg += format_command_result(command_args, command_output) logger.warning(msg) return os.path.join(temp_dir, names[0]) diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 2ceebd07991..9348dd771e8 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -24,10 +24,11 @@ from pip._internal.utils.glibc import check_glibc_version from pip._internal.utils.hashes import Hashes, MissingHashes from pip._internal.utils.misc import ( - call_subprocess, egg_link_path, ensure_dir, get_installed_distributions, - get_prog, make_vcs_requirement_url, normalize_path, redact_netloc, - redact_password_from_url, remove_auth_from_url, rmtree, - split_auth_from_netloc, untar_file, unzip_file, + call_subprocess, egg_link_path, ensure_dir, format_command_args, + get_installed_distributions, get_prog, make_vcs_requirement_url, + normalize_path, redact_netloc, redact_password_from_url, + remove_auth_from_url, rmtree, split_auth_from_netloc, untar_file, + unzip_file, ) from pip._internal.utils.packaging import check_dist_requires_python from pip._internal.utils.temp_dir import AdjacentTempDirectory, TempDirectory @@ -724,6 +725,16 @@ def test_get_prog(self, monkeypatch, argv, executable, expected): assert get_prog() == expected +@pytest.mark.parametrize('args, expected', [ + (['pip', 'list'], 'pip list'), + (['foo', 'space space', 'new\nline', 'double"quote', "single'quote"], + """foo 'space space' 'new\nline' 'double"quote' 'single'"'"'quote'"""), +]) +def test_format_command_args(args, expected): + actual = format_command_args(args) + assert actual == expected + + def test_call_subprocess_works__no_keyword_arguments(): result = call_subprocess( [sys.executable, '-c', 'print("Hello")'], diff --git a/tests/unit/test_wheel.py b/tests/unit/test_wheel.py index 4f210e2e125..19a3259ede4 100644 --- a/tests/unit/test_wheel.py +++ b/tests/unit/test_wheel.py @@ -99,15 +99,15 @@ def test_should_use_ephemeral_cache__issue_6197( assert ephem_cache is expected -def test_format_command__INFO(caplog): - +def test_format_command_result__INFO(caplog): caplog.set_level(logging.INFO) - actual = wheel.format_command( - command_args=['arg1', 'arg2'], + actual = wheel.format_command_result( + # Include an argument with a space to test argument quoting. + command_args=['arg1', 'second arg'], command_output='output line 1\noutput line 2\n', ) assert actual.splitlines() == [ - "Command arguments: ['arg1', 'arg2']", + "Command arguments: arg1 'second arg'", 'Command output: [use --verbose to show]', ] @@ -118,30 +118,30 @@ def test_format_command__INFO(caplog): # Test no trailing newline. 'output line 1\noutput line 2', ]) -def test_format_command__DEBUG(caplog, command_output): +def test_format_command_result__DEBUG(caplog, command_output): caplog.set_level(logging.DEBUG) - actual = wheel.format_command( + actual = wheel.format_command_result( command_args=['arg1', 'arg2'], command_output=command_output, ) assert actual.splitlines() == [ - "Command arguments: ['arg1', 'arg2']", + "Command arguments: arg1 arg2", 'Command output:', 'output line 1', 'output line 2', - '-----------------------------------------', + '----------------------------------------', ] @pytest.mark.parametrize('log_level', ['DEBUG', 'INFO']) -def test_format_command__empty_output(caplog, log_level): +def test_format_command_result__empty_output(caplog, log_level): caplog.set_level(log_level) - actual = wheel.format_command( + actual = wheel.format_command_result( command_args=['arg1', 'arg2'], command_output='', ) assert actual.splitlines() == [ - "Command arguments: ['arg1', 'arg2']", + "Command arguments: arg1 arg2", 'Command output: None', ] @@ -172,7 +172,7 @@ def test_get_legacy_build_wheel_path__no_names(caplog): assert record.levelname == 'WARNING' assert record.message.splitlines() == [ "Legacy build of wheel for 'pendulum' created no files.", - "Command arguments: ['arg1', 'arg2']", + "Command arguments: arg1 arg2", 'Command output: [use --verbose to show]', ] @@ -189,7 +189,7 @@ def test_get_legacy_build_wheel_path__multiple_names(caplog): assert record.message.splitlines() == [ "Legacy build of wheel for 'pendulum' created more than one file.", "Filenames (choosing first): ['name1', 'name2']", - "Command arguments: ['arg1', 'arg2']", + "Command arguments: arg1 arg2", 'Command output: [use --verbose to show]', ]