From 5de48b4800c2cffcf054ebe5047461d6cd768493 Mon Sep 17 00:00:00 2001 From: Jeff Tratner Date: Thu, 29 Mar 2018 00:48:37 -0700 Subject: [PATCH] Better shell quoting 1. Only use split() when we actually come in from CLI (possibly never need to do that at all) 2. Add some additional test cases to cover quoting. 3. Better error message in case of missing executable pulled from a run script. E.g.: ``` [scripts] random = "myexecutable a b c" myexecutable = "echo 5" ``` ``` Error: the command myexecutable (from random) could not be found within PATH. ``` vs. previous, which had a slightly disingenous error message. ``` Error: the command myexecutable could not be found within PATH or Pipfile's [scripts]. ``` --- HISTORY.txt | 1 + pipenv/core.py | 66 +++++++++++++++++++++++++++++--------------- tests/test_pipenv.py | 35 ++++++++++++----------- 3 files changed, 62 insertions(+), 40 deletions(-) diff --git a/HISTORY.txt b/HISTORY.txt index b6eb43b8..f3044163 100644 --- a/HISTORY.txt +++ b/HISTORY.txt @@ -2,6 +2,7 @@ - Resolve editable packages on the local filesystem. - Ensure lock hash does not change based on injected env vars. - Fix bug in detecting .venv at project root when in subdirectories. + - Parse quoting in [scripts] section correctly + clearer run errors. 11.9.0: - Vastly improve markers capabilities. - Support for environment variables in Pipfiles. diff --git a/pipenv/core.py b/pipenv/core.py index b083ed9d..3e7a6b0d 100644 --- a/pipenv/core.py +++ b/pipenv/core.py @@ -2184,20 +2184,30 @@ def inline_activate_virtualenv(): ) +def _construct_run_command(project, command, args): + """Construct a command to run concatenating project and args. + + ``command`` and/or the script from the ``Pipfile`` will be inserted before args. + + Returns (executable, args for executable)""" + args = list(args or []) + # Script was found… + if command in project.scripts: + prefix_argv = project.scripts[command] + else: + # Separate out things that were passed in as a string. + prefix_argv = list(command.split()) + executable, prefix_argv = prefix_argv[0], prefix_argv[1:] + for __c in reversed(prefix_argv): + args.insert(0, __c) + return executable, args + + def do_run(command, args, three=None, python=False): # Ensure that virtualenv is available. ensure_project(three=three, python=python, validate=False) load_dot_env() - # Script was found… - if command in project.scripts: - command = ' '.join(project.scripts[command]) - # Separate out things that were passed in as a string. - _c = list(command.split()) - command = _c.pop(0) - if _c: - args = list(args) - for __c in reversed(_c): - args.insert(0, __c) + executable, args = _construct_run_command(project, command, args) # Activate virtualenv under the current interpreter's environment inline_activate_virtualenv() # Windows! @@ -2205,23 +2215,35 @@ def do_run(command, args, three=None, python=False): import subprocess p = subprocess.Popen( - [command] + list(args), shell=True, universal_newlines=True + [executable] + list(args), shell=True, universal_newlines=True ) p.communicate() sys.exit(p.returncode) else: - command_path = system_which(command) + command_path = system_which(executable) if not command_path: - click.echo( - '{0}: the command {1} could not be found within {2} or Pipfile\'s {3}.' - ''.format( - crayons.red('Error', bold=True), - crayons.red(command), - crayons.normal('PATH', bold=True), - crayons.normal('[scripts]', bold=True), - ), - err=True, - ) + if command in project.scripts: + click.echo( + '{0}: the command {1} (from {2}) could not be found within {3}.' + ''.format( + crayons.red('Error', bold=True), + crayons.red(executable), + crayons.normal(command, bold=True), + crayons.normal('PATH', bold=True), + ), + err=True, + ) + else: + click.echo( + '{0}: the command {1} could not be found within {2} or Pipfile\'s {3}.' + ''.format( + crayons.red('Error', bold=True), + crayons.red(command), + crayons.normal('PATH', bold=True), + crayons.normal('[scripts]', bold=True), + ), + err=True, + ) sys.exit(1) # Execute the command. os.execl(command_path, command_path, *args) diff --git a/tests/test_pipenv.py b/tests/test_pipenv.py index 37461fbc..a56242ea 100644 --- a/tests/test_pipenv.py +++ b/tests/test_pipenv.py @@ -4,7 +4,8 @@ import shutil import json import pytest import warnings -from pipenv.core import activate_virtualenv +from pipenv import core +from pipenv.core import activate_virtualenv, _construct_run_command from pipenv.utils import ( temp_environ, get_windows_path, mkdir_p, normalize_drive, TemporaryDirectory ) @@ -1170,6 +1171,9 @@ flask = "==0.12.2" f.write(r""" [scripts] printfoo = "python -c \"print('foo')\"" +notfoundscript = "randomthingtotally" +appendscript = "cmd arg1" +multicommand = "bash -c \"cd docs && make html\"" """) c = p.pipenv('install') assert c.return_code == 0 @@ -1179,20 +1183,15 @@ printfoo = "python -c \"print('foo')\"" assert c.out == 'foo\n' assert c.err == '' - @pytest.mark.run - @pytest.mark.skip(reason='This fails on Windows (not sure about POSIX).') - def test_scripts_quoted(self): - with PipenvInstance(chdir=True) as p: - with open(p.pipfile_path, 'w') as f: - f.write(""" -[scripts] -printfoo = "python -c print('foo')" - """) - - c = p.pipenv('install') - assert c.return_code == 0 - - c = p.pipenv('run printfoo') - assert c.return_code == 0 - assert c.out == 'foo\n' - assert c.err == '' + if os.name != 'nt': + c = p.pipenv('run notfoundscript') + assert c.return_code == 1 + assert c.out == '' + assert 'Error' in c.err + assert 'randomthingtotally (from notfoundscript)' in c.err + executable, argv = _construct_run_command(Project(), 'multicommand', []) + assert executable == 'bash' + assert argv == ['-c', 'cd docs && make html'] + executable, argv = _construct_run_command(Project(), 'appendscript', ['a', 'b']) + assert executable == 'cmd' + assert argv == ['arg1', 'a', 'b']