From 00a3a9a09bc4541701dc0164bcb3bb826337f120 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 21 Mar 2016 12:10:40 -0700 Subject: [PATCH 1/6] Add envcontext helper --- pre_commit/envcontext.py | 54 +++++++++++++++++++ pre_commit/five.py | 4 ++ tests/envcontext_test.py | 109 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+) create mode 100644 pre_commit/envcontext.py create mode 100644 tests/envcontext_test.py diff --git a/pre_commit/envcontext.py b/pre_commit/envcontext.py new file mode 100644 index 00000000..2013c723 --- /dev/null +++ b/pre_commit/envcontext.py @@ -0,0 +1,54 @@ +from __future__ import absolute_import +from __future__ import unicode_literals + +import collections +import contextlib +import os + +from pre_commit import five + + +UNSET = collections.namedtuple('UNSET', ())() + + +Var = collections.namedtuple('Var', ('name', 'default')) +setattr(Var.__new__, five.defaults_attr, ('',)) + + +def format_env(parts, env): + return ''.join( + env.get(part.name, part.default) + if isinstance(part, Var) + else part + for part in parts + ) + + +@contextlib.contextmanager +def envcontext(patch, _env=None): + """In this context, `os.environ` is modified according to `patch`. + + `patch` is an iterable of 2-tuples (key, value): + `key`: string + `value`: + - string: `environ[key] == value` inside the context. + - UNSET: `key not in environ` inside the context. + - template: A template is a tuple of strings and Var which will be + replaced with the previous environment + """ + env = os.environ if _env is None else _env + before = env.copy() + + for k, v in patch: + if v is UNSET: + env.pop(k, None) + elif isinstance(v, tuple): + env[k] = format_env(v, before) + else: + env[k] = v + + try: + yield + finally: + env.clear() + env.update(before) diff --git a/pre_commit/five.py b/pre_commit/five.py index 8b9a2b54..2ae91c59 100644 --- a/pre_commit/five.py +++ b/pre_commit/five.py @@ -12,6 +12,8 @@ if PY2: # pragma: no cover (PY2 only) return s else: return s.encode('UTF-8') + + defaults_attr = 'func_defaults' else: # pragma: no cover (PY3 only) text = str @@ -21,6 +23,8 @@ else: # pragma: no cover (PY3 only) else: return s.decode('UTF-8') + defaults_attr = '__defaults__' + def to_text(s): return s if isinstance(s, text) else s.decode('UTF-8') diff --git a/tests/envcontext_test.py b/tests/envcontext_test.py new file mode 100644 index 00000000..c03e9431 --- /dev/null +++ b/tests/envcontext_test.py @@ -0,0 +1,109 @@ +from __future__ import absolute_import +from __future__ import unicode_literals + +import os + +import mock +import pytest + +from pre_commit.envcontext import envcontext +from pre_commit.envcontext import UNSET +from pre_commit.envcontext import Var + + +def _test(**kwargs): + before = kwargs.pop('before') + patch = kwargs.pop('patch') + expected = kwargs.pop('expected') + assert not kwargs + + env = before.copy() + with envcontext(patch, _env=env): + assert env == expected + assert env == before + + +def test_trivial(): + _test(before={}, patch={}, expected={}) + + +def test_noop(): + _test(before={'foo': 'bar'}, patch=(), expected={'foo': 'bar'}) + + +def test_adds(): + _test(before={}, patch=[('foo', 'bar')], expected={'foo': 'bar'}) + + +def test_overrides(): + _test( + before={'foo': 'baz'}, + patch=[('foo', 'bar')], + expected={'foo': 'bar'}, + ) + + +def test_unset_but_nothing_to_unset(): + _test(before={}, patch=[('foo', UNSET)], expected={}) + + +def test_unset_things_to_remove(): + _test( + before={'PYTHONHOME': ''}, + patch=[('PYTHONHOME', UNSET)], + expected={}, + ) + + +def test_templated_environment_variable_missing(): + _test( + before={}, + patch=[('PATH', ('~/bin:', Var('PATH')))], + expected={'PATH': '~/bin:'}, + ) + + +def test_templated_environment_variable_defaults(): + _test( + before={}, + patch=[('PATH', ('~/bin:', Var('PATH', default='/bin')))], + expected={'PATH': '~/bin:/bin'}, + ) + + +def test_templated_environment_variable_there(): + _test( + before={'PATH': '/usr/local/bin:/usr/bin'}, + patch=[('PATH', ('~/bin:', Var('PATH')))], + expected={'PATH': '~/bin:/usr/local/bin:/usr/bin'}, + ) + + +def test_templated_environ_sources_from_previous(): + _test( + before={'foo': 'bar'}, + patch=( + ('foo', 'baz'), + ('herp', ('foo: ', Var('foo'))), + ), + expected={'foo': 'baz', 'herp': 'foo: bar'}, + ) + + +def test_exception_safety(): + class MyError(RuntimeError): + pass + + env = {} + with pytest.raises(MyError): + with envcontext([('foo', 'bar')], _env=env): + raise MyError() + assert env == {} + + +def test_integration_os_environ(): + with mock.patch.dict(os.environ, {'FOO': 'bar'}, clear=True): + assert os.environ == {'FOO': 'bar'} + with envcontext([('HERP', 'derp')]): + assert os.environ == {'FOO': 'bar', 'HERP': 'derp'} + assert os.environ == {'FOO': 'bar'} From a5b56bd9e3062582f8e133999a7ffae7258d8cae Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 21 Mar 2016 14:58:56 -0700 Subject: [PATCH 2/6] Factor out bash and activate files --- Makefile | 3 +- pre_commit/languages/all.py | 2 +- pre_commit/languages/helpers.py | 41 ++++----------- pre_commit/languages/node.py | 49 ++++++++++-------- pre_commit/languages/pcre.py | 25 +++++---- pre_commit/languages/python.py | 57 ++++++++++----------- pre_commit/languages/ruby.py | 74 +++++++++++++++++---------- pre_commit/languages/script.py | 12 ++--- pre_commit/languages/system.py | 11 ++-- pre_commit/prefixed_command_runner.py | 12 ++--- tests/languages/all_test.py | 2 +- tests/prefixed_command_runner_test.py | 29 ----------- tests/repository_test.py | 21 ++++---- 13 files changed, 149 insertions(+), 189 deletions(-) diff --git a/Makefile b/Makefile index 75415fc4..16868f1e 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,3 @@ - REBUILD_FLAG = .PHONY: all @@ -21,7 +20,7 @@ test: .venv.touch .PHONY: clean clean: - find . -iname '*.pyc' | xargs rm -f + find . -name '*.pyc' -delete rm -rf .tox rm -rf ./venv-* rm -f .venv.touch diff --git a/pre_commit/languages/all.py b/pre_commit/languages/all.py index 63c1d514..40c23131 100644 --- a/pre_commit/languages/all.py +++ b/pre_commit/languages/all.py @@ -15,7 +15,7 @@ from pre_commit.languages import system # def install_environment( # repo_cmd_runner, # version='default', -# additional_dependencies=None, +# additional_dependencies=(), # ): # """Installs a repository in the given repository. Note that the current # working directory will already be inside the repository. diff --git a/pre_commit/languages/helpers.py b/pre_commit/languages/helpers.py index b0add575..5887d3e2 100644 --- a/pre_commit/languages/helpers.py +++ b/pre_commit/languages/helpers.py @@ -1,6 +1,10 @@ from __future__ import unicode_literals -import pipes +from pre_commit.util import cmd_output + + +def run_setup_cmd(runner, cmd): + cmd_output(*cmd, cwd=runner.prefix_dir, encoding=None) def environment_dir(ENVIRONMENT_DIR, language_version): @@ -14,39 +18,12 @@ def file_args_to_stdin(file_args): return '\0'.join(list(file_args) + ['']) -def run_hook(env, hook, file_args): - quoted_args = [pipes.quote(arg) for arg in hook['args']] - return env.run( +def run_hook(cmd_args, file_args): + return cmd_output( # Use -s 4000 (slightly less than posix mandated minimum) # This is to prevent "xargs: ... Bad file number" on windows - ' '.join(['xargs', '-0', '-s4000', hook['entry']] + quoted_args), + 'xargs', '-0', '-s4000', *cmd_args, stdin=file_args_to_stdin(file_args), retcode=None, - encoding=None, + encoding=None ) - - -class Environment(object): - def __init__(self, repo_cmd_runner, language_version): - self.repo_cmd_runner = repo_cmd_runner - self.language_version = language_version - - @property - def env_prefix(self): - """env_prefix is a value that is prefixed to the command that is run. - - Usually this is to source a virtualenv, etc. - - Commands basically end up looking like: - - bash -c '{env_prefix} {cmd}' - - so you'll often want to end your prefix with && - """ - raise NotImplementedError - - def run(self, cmd, **kwargs): - """Returns (returncode, stdout, stderr).""" - return self.repo_cmd_runner.run( - ['bash', '-c', ' '.join([self.env_prefix, cmd])], **kwargs - ) diff --git a/pre_commit/languages/node.py b/pre_commit/languages/node.py index 99b46df7..1c1108ac 100644 --- a/pre_commit/languages/node.py +++ b/pre_commit/languages/node.py @@ -1,34 +1,44 @@ from __future__ import unicode_literals import contextlib +import os import sys +from pre_commit.envcontext import envcontext +from pre_commit.envcontext import Var from pre_commit.languages import helpers from pre_commit.util import clean_path_on_failure -from pre_commit.util import shell_escape ENVIRONMENT_DIR = 'node_env' -class NodeEnv(helpers.Environment): - @property - def env_prefix(self): - return ". '{{prefix}}{0}/bin/activate' &&".format( - helpers.environment_dir(ENVIRONMENT_DIR, self.language_version), - ) +def get_env_patch(venv): + return ( + ('NODE_VIRTUAL_ENV', venv), + ('NPM_CONFIG_PREFIX', venv), + ('npm_config_prefix', venv), + ('NODE_PATH', os.path.join(venv, 'lib', 'node_modules')), + ('PATH', (os.path.join(venv, 'bin'), os.pathsep, Var('PATH'))), + ) @contextlib.contextmanager def in_env(repo_cmd_runner, language_version): - yield NodeEnv(repo_cmd_runner, language_version) + envdir = os.path.join( + repo_cmd_runner.prefix_dir, + helpers.environment_dir(ENVIRONMENT_DIR, language_version), + ) + with envcontext(get_env_patch(envdir)): + yield def install_environment( repo_cmd_runner, version='default', - additional_dependencies=None, + additional_dependencies=(), ): + additional_dependencies = tuple(additional_dependencies) assert repo_cmd_runner.exists('package.json') directory = helpers.environment_dir(ENVIRONMENT_DIR, version) @@ -44,18 +54,15 @@ def install_environment( repo_cmd_runner.run(cmd) - with in_env(repo_cmd_runner, version) as node_env: - node_env.run("cd '{prefix}' && npm install -g", encoding=None) - if additional_dependencies: - node_env.run( - "cd '{prefix}' && npm install -g " + - ' '.join( - shell_escape(dep) for dep in additional_dependencies - ), - encoding=None, - ) + with in_env(repo_cmd_runner, version): + helpers.run_setup_cmd( + repo_cmd_runner, + ('npm', 'install', '-g', '.') + additional_dependencies, + ) def run_hook(repo_cmd_runner, hook, file_args): - with in_env(repo_cmd_runner, hook['language_version']) as env: - return helpers.run_hook(env, hook, file_args) + with in_env(repo_cmd_runner, hook['language_version']): + return helpers.run_hook( + (hook['entry'],) + tuple(hook['args']), file_args, + ) diff --git a/pre_commit/languages/pcre.py b/pre_commit/languages/pcre.py index 823e55cb..d8b7fd3e 100644 --- a/pre_commit/languages/pcre.py +++ b/pre_commit/languages/pcre.py @@ -2,7 +2,7 @@ from __future__ import unicode_literals from sys import platform -from pre_commit.languages.helpers import file_args_to_stdin +from pre_commit.languages import helpers from pre_commit.util import shell_escape @@ -12,29 +12,28 @@ ENVIRONMENT_DIR = None def install_environment( repo_cmd_runner, version='default', - additional_dependencies=None, + additional_dependencies=(), ): """Installation for pcre type is a noop.""" raise AssertionError('Cannot install pcre repo.') def run_hook(repo_cmd_runner, hook, file_args): - grep_command = 'grep -H -n -P' - if platform == 'darwin': # pragma: no cover (osx) - grep_command = 'ggrep -H -n -P' + grep_command = '{0} -H -n -P'.format( + 'ggrep' if platform == 'darwin' else 'grep', + ) # For PCRE the entry is the regular expression to match - return repo_cmd_runner.run( - [ - 'xargs', '-0', 'sh', '-c', + return helpers.run_hook( + ( + 'sh', '-c', # Grep usually returns 0 for matches, and nonzero for non-matches # so we flip it here. '! {0} {1} {2} $@'.format( grep_command, ' '.join(hook['args']), - shell_escape(hook['entry'])), + shell_escape(hook['entry']), + ), '--', - ], - stdin=file_args_to_stdin(file_args), - retcode=None, - encoding=None, + ), + file_args, ) diff --git a/pre_commit/languages/python.py b/pre_commit/languages/python.py index 4c463874..8bfc83b3 100644 --- a/pre_commit/languages/python.py +++ b/pre_commit/languages/python.py @@ -5,9 +5,11 @@ import distutils.spawn import os import sys +from pre_commit.envcontext import envcontext +from pre_commit.envcontext import UNSET +from pre_commit.envcontext import Var from pre_commit.languages import helpers from pre_commit.util import clean_path_on_failure -from pre_commit.util import shell_escape ENVIRONMENT_DIR = 'py_env' @@ -15,26 +17,26 @@ ENVIRONMENT_DIR = 'py_env' def bin_dir(venv): """On windows there's a different directory for the virtualenv""" - if os.name == 'nt': # pragma: no cover (windows) - return os.path.join(venv, 'Scripts') - else: - return os.path.join(venv, 'bin') + bin_part = 'Scripts' if os.name == 'nt' else 'bin' + return os.path.join(venv, bin_part) -class PythonEnv(helpers.Environment): - @property - def env_prefix(self): - return ". '{{prefix}}{0}{1}activate' &&".format( - bin_dir( - helpers.environment_dir(ENVIRONMENT_DIR, self.language_version) - ), - os.sep, - ) +def get_env_patch(venv): + return ( + ('PYTHONHOME', UNSET), + ('VIRTUAL_ENV', venv), + ('PATH', (bin_dir(venv), os.pathsep, Var('PATH'))), + ) @contextlib.contextmanager def in_env(repo_cmd_runner, language_version): - yield PythonEnv(repo_cmd_runner, language_version) + envdir = os.path.join( + repo_cmd_runner.prefix_dir, + helpers.environment_dir(ENVIRONMENT_DIR, language_version), + ) + with envcontext(get_env_patch(envdir)): + yield def norm_version(version): @@ -55,9 +57,9 @@ def norm_version(version): def install_environment( repo_cmd_runner, version='default', - additional_dependencies=None, + additional_dependencies=(), ): - assert repo_cmd_runner.exists('setup.py') + additional_dependencies = tuple(additional_dependencies) directory = helpers.environment_dir(ENVIRONMENT_DIR, version) # Install a virtualenv @@ -69,18 +71,15 @@ def install_environment( if version != 'default': venv_cmd.extend(['-p', norm_version(version)]) repo_cmd_runner.run(venv_cmd) - with in_env(repo_cmd_runner, version) as env: - env.run("cd '{prefix}' && pip install .", encoding=None) - if additional_dependencies: - env.run( - "cd '{prefix}' && pip install " + - ' '.join( - shell_escape(dep) for dep in additional_dependencies - ), - encoding=None, - ) + with in_env(repo_cmd_runner, version): + helpers.run_setup_cmd( + repo_cmd_runner, + ('pip', 'install', '.') + additional_dependencies, + ) def run_hook(repo_cmd_runner, hook, file_args): - with in_env(repo_cmd_runner, hook['language_version']) as env: - return helpers.run_hook(env, hook, file_args) + with in_env(repo_cmd_runner, hook['language_version']): + return helpers.run_hook( + (hook['entry'],) + tuple(hook['args']), file_args, + ) diff --git a/pre_commit/languages/ruby.py b/pre_commit/languages/ruby.py index a23df5d3..ed494c24 100644 --- a/pre_commit/languages/ruby.py +++ b/pre_commit/languages/ruby.py @@ -2,30 +2,42 @@ from __future__ import unicode_literals import contextlib import io +import os.path import shutil +from pre_commit.envcontext import envcontext +from pre_commit.envcontext import Var from pre_commit.languages import helpers from pre_commit.util import CalledProcessError from pre_commit.util import clean_path_on_failure from pre_commit.util import resource_filename -from pre_commit.util import shell_escape from pre_commit.util import tarfile_open ENVIRONMENT_DIR = 'rbenv' -class RubyEnv(helpers.Environment): - @property - def env_prefix(self): - return '. {{prefix}}{0}/bin/activate &&'.format( - helpers.environment_dir(ENVIRONMENT_DIR, self.language_version) - ) +def get_env_patch(venv, language_version): + return ( + ('GEM_HOME', os.path.join(venv, 'gems')), + ('RBENV_ROOT', venv), + ('RBENV_VERSION', language_version), + ('PATH', ( + os.path.join(venv, 'gems', 'bin'), os.pathsep, + os.path.join(venv, 'shims'), os.pathsep, + os.path.join(venv, 'bin'), os.pathsep, Var('PATH'), + )), + ) @contextlib.contextmanager def in_env(repo_cmd_runner, language_version): - yield RubyEnv(repo_cmd_runner, language_version) + envdir = os.path.join( + repo_cmd_runner.prefix_dir, + helpers.environment_dir(ENVIRONMENT_DIR, language_version), + ) + with envcontext(get_env_patch(envdir, language_version)): + yield def _install_rbenv(repo_cmd_runner, version='default'): @@ -71,42 +83,48 @@ def _install_rbenv(repo_cmd_runner, version='default'): activate_file.write('export RBENV_VERSION="{0}"\n'.format(version)) -def _install_ruby(environment, version): +def _install_ruby(runner, version): try: - environment.run('rbenv download {0}'.format(version)) + helpers.run_setup_cmd(runner, ('rbenv', 'download', version)) except CalledProcessError: # pragma: no cover (usually find with download) # Failed to download from mirror for some reason, build it instead - environment.run('rbenv install {0}'.format(version)) + helpers.run_setup_cmd(runner, ('rbenv', 'install', version)) def install_environment( repo_cmd_runner, version='default', - additional_dependencies=None, + additional_dependencies=(), ): + additional_dependencies = tuple(additional_dependencies) directory = helpers.environment_dir(ENVIRONMENT_DIR, version) with clean_path_on_failure(repo_cmd_runner.path(directory)): # TODO: this currently will fail if there's no version specified and # there's no system ruby installed. Is this ok? _install_rbenv(repo_cmd_runner, version=version) - with in_env(repo_cmd_runner, version) as ruby_env: + with in_env(repo_cmd_runner, version): + # Need to call this before installing so rbenv's directories are + # set up + helpers.run_setup_cmd(repo_cmd_runner, ('rbenv', 'init', '-')) if version != 'default': - _install_ruby(ruby_env, version) - ruby_env.run( - 'cd {prefix} && gem build *.gemspec && ' - 'gem install --no-ri --no-rdoc *.gem', - encoding=None, + _install_ruby(repo_cmd_runner, version) + # Need to call this after installing to set up the shims + helpers.run_setup_cmd(repo_cmd_runner, ('rbenv', 'rehash')) + helpers.run_setup_cmd( + repo_cmd_runner, + ('gem', 'build') + repo_cmd_runner.star('.gemspec'), + ) + helpers.run_setup_cmd( + repo_cmd_runner, + ( + ('gem', 'install', '--no-ri', '--no-rdoc') + + repo_cmd_runner.star('.gem') + additional_dependencies + ), ) - if additional_dependencies: - ruby_env.run( - 'cd {prefix} && gem install --no-ri --no-rdoc ' + - ' '.join( - shell_escape(dep) for dep in additional_dependencies - ), - encoding=None, - ) def run_hook(repo_cmd_runner, hook, file_args): - with in_env(repo_cmd_runner, hook['language_version']) as env: - return helpers.run_hook(env, hook, file_args) + with in_env(repo_cmd_runner, hook['language_version']): + return helpers.run_hook( + (hook['entry'],) + tuple(hook['args']), file_args, + ) diff --git a/pre_commit/languages/script.py b/pre_commit/languages/script.py index 5ba871ea..7c413c36 100644 --- a/pre_commit/languages/script.py +++ b/pre_commit/languages/script.py @@ -1,6 +1,6 @@ from __future__ import unicode_literals -from pre_commit.languages.helpers import file_args_to_stdin +from pre_commit.languages import helpers ENVIRONMENT_DIR = None @@ -9,16 +9,14 @@ ENVIRONMENT_DIR = None def install_environment( repo_cmd_runner, version='default', - additional_dependencies=None, + additional_dependencies=(), ): """Installation for script type is a noop.""" raise AssertionError('Cannot install script repo.') def run_hook(repo_cmd_runner, hook, file_args): - return repo_cmd_runner.run( - ['xargs', '-0', '{{prefix}}{0}'.format(hook['entry'])] + hook['args'], - stdin=file_args_to_stdin(file_args), - retcode=None, - encoding=None, + return helpers.run_hook( + (repo_cmd_runner.prefix_dir + hook['entry'],) + tuple(hook['args']), + file_args, ) diff --git a/pre_commit/languages/system.py b/pre_commit/languages/system.py index 3930422b..340f4feb 100644 --- a/pre_commit/languages/system.py +++ b/pre_commit/languages/system.py @@ -2,7 +2,7 @@ from __future__ import unicode_literals import shlex -from pre_commit.languages.helpers import file_args_to_stdin +from pre_commit.languages import helpers ENVIRONMENT_DIR = None @@ -11,16 +11,13 @@ ENVIRONMENT_DIR = None def install_environment( repo_cmd_runner, version='default', - additional_dependencies=None, + additional_dependencies=(), ): """Installation for system type is a noop.""" raise AssertionError('Cannot install system repo.') def run_hook(repo_cmd_runner, hook, file_args): - return repo_cmd_runner.run( - ['xargs', '-0'] + shlex.split(hook['entry']) + hook['args'], - stdin=file_args_to_stdin(file_args), - retcode=None, - encoding=None, + return helpers.run_hook( + tuple(shlex.split(hook['entry'])) + tuple(hook['args']), file_args, ) diff --git a/pre_commit/prefixed_command_runner.py b/pre_commit/prefixed_command_runner.py index 2b1212a2..fc4a3198 100644 --- a/pre_commit/prefixed_command_runner.py +++ b/pre_commit/prefixed_command_runner.py @@ -45,13 +45,7 @@ class PrefixedCommandRunner(object): def exists(self, *parts): return os.path.exists(self.path(*parts)) - @classmethod - def from_command_runner(cls, command_runner, path_end): - """Constructs a new command runner from an existing one by appending - `path_end` to the command runner's prefix directory. - """ - return cls( - command_runner.path(path_end), - popen=command_runner.__popen, - makedirs=command_runner.__makedirs, + def star(self, end): + return tuple( + path for path in os.listdir(self.prefix_dir) if path.endswith(end) ) diff --git a/tests/languages/all_test.py b/tests/languages/all_test.py index 2254e388..73b89cb5 100644 --- a/tests/languages/all_test.py +++ b/tests/languages/all_test.py @@ -14,7 +14,7 @@ def test_install_environment_argspec(language): args=['repo_cmd_runner', 'version', 'additional_dependencies'], varargs=None, keywords=None, - defaults=('default', None), + defaults=('default', ()), ) argspec = inspect.getargspec(languages[language].install_environment) assert argspec == expected_argspec diff --git a/tests/prefixed_command_runner_test.py b/tests/prefixed_command_runner_test.py index bafcae72..3f691b4b 100644 --- a/tests/prefixed_command_runner_test.py +++ b/tests/prefixed_command_runner_test.py @@ -110,35 +110,6 @@ def test_path_multiple_args(): assert ret == os.path.join('foo', 'bar', 'baz') -@pytest.mark.parametrize( - ('prefix', 'path_end', 'expected_output'), - tuple( - (prefix, path_end, expected_output + os.sep) - for prefix, path_end, expected_output in PATH_TESTS - ), -) -def test_from_command_runner(prefix, path_end, expected_output): - first = PrefixedCommandRunner(prefix) - second = PrefixedCommandRunner.from_command_runner(first, path_end) - assert second.prefix_dir == expected_output - - -def test_from_command_runner_preserves_popen(popen_mock, makedirs_mock): - first = PrefixedCommandRunner( - 'foo', popen=popen_mock, makedirs=makedirs_mock, - ) - second = PrefixedCommandRunner.from_command_runner(first, 'bar') - second.run(['foo/bar/baz'], retcode=None) - popen_mock.assert_called_once_with( - [five.n('foo/bar/baz')], - env=None, - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - ) - makedirs_mock.assert_called_once_with(os.path.join('foo', 'bar') + os.sep) - - def test_create_path_if_not_exists(in_tmpdir): instance = PrefixedCommandRunner('foo') assert not os.path.exists('foo') diff --git a/tests/repository_test.py b/tests/repository_test.py index ef870b53..9642b4c5 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -16,6 +16,7 @@ from pre_commit import five from pre_commit.clientlib.validate_config import CONFIG_JSON_SCHEMA from pre_commit.clientlib.validate_config import validate_config_extra from pre_commit.jsonschema_extensions import apply_defaults +from pre_commit.languages import helpers from pre_commit.languages import node from pre_commit.languages import python from pre_commit.languages import ruby @@ -355,8 +356,8 @@ def test_additional_python_dependencies_installed(tempdir_factory, store): config['hooks'][0]['additional_dependencies'] = ['mccabe'] repo = Repository.create(config, store) repo.run_hook(repo.hooks[0][1], []) - with python.in_env(repo.cmd_runner, 'default') as env: - output = env.run('pip freeze -l')[1] + with python.in_env(repo.cmd_runner, 'default'): + output = cmd_output('pip', 'freeze', '-l')[1] assert 'mccabe' in output @@ -372,8 +373,8 @@ def test_additional_dependencies_roll_forward(tempdir_factory, store): repo = Repository.create(config, store) repo.run_hook(repo.hooks[0][1], []) # We should see our additional dependency installed - with python.in_env(repo.cmd_runner, 'default') as env: - output = env.run('pip freeze -l')[1] + with python.in_env(repo.cmd_runner, 'default'): + output = cmd_output('pip', 'freeze', '-l')[1] assert 'mccabe' in output @@ -388,8 +389,8 @@ def test_additional_ruby_dependencies_installed( config['hooks'][0]['additional_dependencies'] = ['thread_safe'] repo = Repository.create(config, store) repo.run_hook(repo.hooks[0][1], []) - with ruby.in_env(repo.cmd_runner, 'default') as env: - output = env.run('gem list --local')[1] + with ruby.in_env(repo.cmd_runner, 'default'): + output = cmd_output('gem', 'list', '--local')[1] assert 'thread_safe' in output @@ -405,9 +406,9 @@ def test_additional_node_dependencies_installed( config['hooks'][0]['additional_dependencies'] = ['lodash'] repo = Repository.create(config, store) repo.run_hook(repo.hooks[0][1], []) - with node.in_env(repo.cmd_runner, 'default') as env: - env.run('npm config set global true') - output = env.run(('npm ls'))[1] + with node.in_env(repo.cmd_runner, 'default'): + cmd_output('npm', 'config', 'set', 'global', 'true') + output = cmd_output('npm', 'ls')[1] assert 'lodash' in output @@ -443,7 +444,7 @@ def test_control_c_control_c_on_install(tempdir_factory, store): # raise as well. with pytest.raises(MyKeyboardInterrupt): with mock.patch.object( - python.PythonEnv, 'run', side_effect=MyKeyboardInterrupt, + helpers, 'run_setup_cmd', side_effect=MyKeyboardInterrupt, ): with mock.patch.object( shutil, 'rmtree', side_effect=MyKeyboardInterrupt, From b7d395410beff6e678e0c1c8988bc01e033eb0fb Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 21 Mar 2016 19:30:47 -0700 Subject: [PATCH 3/6] Implement a simplified xargs in python --- pre_commit/commands/run.py | 2 +- pre_commit/languages/helpers.py | 15 --------- pre_commit/languages/node.py | 5 ++- pre_commit/languages/pcre.py | 4 +-- pre_commit/languages/python.py | 5 ++- pre_commit/languages/ruby.py | 5 ++- pre_commit/languages/script.py | 4 +-- pre_commit/languages/system.py | 4 +-- pre_commit/util.py | 6 +--- pre_commit/xargs.py | 59 +++++++++++++++++++++++++++++++++ tests/languages/helpers_test.py | 16 --------- tests/repository_test.py | 20 +++++------ tests/xargs_test.py | 47 ++++++++++++++++++++++++++ 13 files changed, 130 insertions(+), 62 deletions(-) create mode 100644 pre_commit/xargs.py delete mode 100644 tests/languages/helpers_test.py create mode 100644 tests/xargs_test.py diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py index f45e7089..9c219008 100644 --- a/pre_commit/commands/run.py +++ b/pre_commit/commands/run.py @@ -86,7 +86,7 @@ def _run_single_hook(hook, repo, args, write, skips=frozenset()): sys.stdout.flush() diff_before = cmd_output('git', 'diff', retcode=None, encoding=None) - retcode, stdout, stderr = repo.run_hook(hook, filenames) + retcode, stdout, stderr = repo.run_hook(hook, tuple(filenames)) diff_after = cmd_output('git', 'diff', retcode=None, encoding=None) file_modifications = diff_before != diff_after diff --git a/pre_commit/languages/helpers.py b/pre_commit/languages/helpers.py index 5887d3e2..322c55f1 100644 --- a/pre_commit/languages/helpers.py +++ b/pre_commit/languages/helpers.py @@ -12,18 +12,3 @@ def environment_dir(ENVIRONMENT_DIR, language_version): return None else: return '{0}-{1}'.format(ENVIRONMENT_DIR, language_version) - - -def file_args_to_stdin(file_args): - return '\0'.join(list(file_args) + ['']) - - -def run_hook(cmd_args, file_args): - return cmd_output( - # Use -s 4000 (slightly less than posix mandated minimum) - # This is to prevent "xargs: ... Bad file number" on windows - 'xargs', '-0', '-s4000', *cmd_args, - stdin=file_args_to_stdin(file_args), - retcode=None, - encoding=None - ) diff --git a/pre_commit/languages/node.py b/pre_commit/languages/node.py index 1c1108ac..2a23fe10 100644 --- a/pre_commit/languages/node.py +++ b/pre_commit/languages/node.py @@ -8,6 +8,7 @@ from pre_commit.envcontext import envcontext from pre_commit.envcontext import Var from pre_commit.languages import helpers from pre_commit.util import clean_path_on_failure +from pre_commit.xargs import xargs ENVIRONMENT_DIR = 'node_env' @@ -63,6 +64,4 @@ def install_environment( def run_hook(repo_cmd_runner, hook, file_args): with in_env(repo_cmd_runner, hook['language_version']): - return helpers.run_hook( - (hook['entry'],) + tuple(hook['args']), file_args, - ) + return xargs((hook['entry'],) + tuple(hook['args']), file_args) diff --git a/pre_commit/languages/pcre.py b/pre_commit/languages/pcre.py index d8b7fd3e..0b2cfd18 100644 --- a/pre_commit/languages/pcre.py +++ b/pre_commit/languages/pcre.py @@ -2,8 +2,8 @@ from __future__ import unicode_literals from sys import platform -from pre_commit.languages import helpers from pre_commit.util import shell_escape +from pre_commit.xargs import xargs ENVIRONMENT_DIR = None @@ -24,7 +24,7 @@ def run_hook(repo_cmd_runner, hook, file_args): ) # For PCRE the entry is the regular expression to match - return helpers.run_hook( + return xargs( ( 'sh', '-c', # Grep usually returns 0 for matches, and nonzero for non-matches diff --git a/pre_commit/languages/python.py b/pre_commit/languages/python.py index 8bfc83b3..5c8a60bf 100644 --- a/pre_commit/languages/python.py +++ b/pre_commit/languages/python.py @@ -10,6 +10,7 @@ from pre_commit.envcontext import UNSET from pre_commit.envcontext import Var from pre_commit.languages import helpers from pre_commit.util import clean_path_on_failure +from pre_commit.xargs import xargs ENVIRONMENT_DIR = 'py_env' @@ -80,6 +81,4 @@ def install_environment( def run_hook(repo_cmd_runner, hook, file_args): with in_env(repo_cmd_runner, hook['language_version']): - return helpers.run_hook( - (hook['entry'],) + tuple(hook['args']), file_args, - ) + return xargs((hook['entry'],) + tuple(hook['args']), file_args) diff --git a/pre_commit/languages/ruby.py b/pre_commit/languages/ruby.py index ed494c24..dc320b3f 100644 --- a/pre_commit/languages/ruby.py +++ b/pre_commit/languages/ruby.py @@ -12,6 +12,7 @@ from pre_commit.util import CalledProcessError from pre_commit.util import clean_path_on_failure from pre_commit.util import resource_filename from pre_commit.util import tarfile_open +from pre_commit.xargs import xargs ENVIRONMENT_DIR = 'rbenv' @@ -125,6 +126,4 @@ def install_environment( def run_hook(repo_cmd_runner, hook, file_args): with in_env(repo_cmd_runner, hook['language_version']): - return helpers.run_hook( - (hook['entry'],) + tuple(hook['args']), file_args, - ) + return xargs((hook['entry'],) + tuple(hook['args']), file_args) diff --git a/pre_commit/languages/script.py b/pre_commit/languages/script.py index 7c413c36..5c652846 100644 --- a/pre_commit/languages/script.py +++ b/pre_commit/languages/script.py @@ -1,6 +1,6 @@ from __future__ import unicode_literals -from pre_commit.languages import helpers +from pre_commit.xargs import xargs ENVIRONMENT_DIR = None @@ -16,7 +16,7 @@ def install_environment( def run_hook(repo_cmd_runner, hook, file_args): - return helpers.run_hook( + return xargs( (repo_cmd_runner.prefix_dir + hook['entry'],) + tuple(hook['args']), file_args, ) diff --git a/pre_commit/languages/system.py b/pre_commit/languages/system.py index 340f4feb..8be45855 100644 --- a/pre_commit/languages/system.py +++ b/pre_commit/languages/system.py @@ -2,7 +2,7 @@ from __future__ import unicode_literals import shlex -from pre_commit.languages import helpers +from pre_commit.xargs import xargs ENVIRONMENT_DIR = None @@ -18,6 +18,6 @@ def install_environment( def run_hook(repo_cmd_runner, hook, file_args): - return helpers.run_hook( + return xargs( tuple(shlex.split(hook['entry'])) + tuple(hook['args']), file_args, ) diff --git a/pre_commit/util.py b/pre_commit/util.py index 559ab703..853a95b1 100644 --- a/pre_commit/util.py +++ b/pre_commit/util.py @@ -160,7 +160,6 @@ class CalledProcessError(RuntimeError): def cmd_output(*cmd, **kwargs): retcode = kwargs.pop('retcode', 0) - stdin = kwargs.pop('stdin', None) encoding = kwargs.pop('encoding', 'UTF-8') __popen = kwargs.pop('__popen', subprocess.Popen) @@ -170,9 +169,6 @@ def cmd_output(*cmd, **kwargs): 'stderr': subprocess.PIPE, } - if stdin is not None: - stdin = stdin.encode('UTF-8') - # py2/py3 on windows are more strict about the types here cmd = [five.n(arg) for arg in cmd] kwargs['env'] = dict( @@ -182,7 +178,7 @@ def cmd_output(*cmd, **kwargs): popen_kwargs.update(kwargs) proc = __popen(cmd, **popen_kwargs) - stdout, stderr = proc.communicate(stdin) + stdout, stderr = proc.communicate() if encoding is not None and stdout is not None: stdout = stdout.decode(encoding) if encoding is not None and stderr is not None: diff --git a/pre_commit/xargs.py b/pre_commit/xargs.py new file mode 100644 index 00000000..2b8aff56 --- /dev/null +++ b/pre_commit/xargs.py @@ -0,0 +1,59 @@ +from __future__ import absolute_import +from __future__ import unicode_literals + +from pre_commit.util import cmd_output + + +# Limit used previously to avoid "xargs ... Bad file number" on windows +# This is slightly less than the posix mandated minimum +MAX_LENGTH = 4000 + + +class ArgumentTooLongError(RuntimeError): + pass + + +def partition(cmd, varargs, _max_length=MAX_LENGTH): + cmd = tuple(cmd) + ret = [] + + ret_cmd = [] + total_len = len(' '.join(cmd)) + # Reversed so arguments are in order + varargs = list(reversed(varargs)) + + while varargs: + arg = varargs.pop() + + if total_len + 1 + len(arg) <= _max_length: + ret_cmd.append(arg) + total_len += len(arg) + elif not ret_cmd: + raise ArgumentTooLongError(arg) + else: + # We've exceeded the length, yield a command + ret.append(cmd + tuple(ret_cmd)) + ret_cmd = [] + total_len = len(' '.join(cmd)) + varargs.append(arg) + + ret.append(cmd + tuple(ret_cmd)) + + return tuple(ret) + + +def xargs(cmd, varargs): + """A simplified implementation of xargs.""" + retcode = 0 + stdout = b'' + stderr = b'' + + for run_cmd in partition(cmd, varargs): + proc_retcode, proc_out, proc_err = cmd_output( + *run_cmd, encoding=None, retcode=None + ) + retcode |= proc_retcode + stdout += proc_out + stderr += proc_err + + return retcode, stdout, stderr diff --git a/tests/languages/helpers_test.py b/tests/languages/helpers_test.py deleted file mode 100644 index 8497ceb0..00000000 --- a/tests/languages/helpers_test.py +++ /dev/null @@ -1,16 +0,0 @@ -from __future__ import absolute_import -from __future__ import unicode_literals - -from pre_commit.languages.helpers import file_args_to_stdin - - -def test_file_args_to_stdin_empty(): - assert file_args_to_stdin([]) == '' - - -def test_file_args_to_stdin_some(): - assert file_args_to_stdin(['foo', 'bar']) == 'foo\0bar\0' - - -def test_file_args_to_stdin_tuple(): - assert file_args_to_stdin(('foo', 'bar')) == 'foo\0bar\0' diff --git a/tests/repository_test.py b/tests/repository_test.py index 9642b4c5..978b42ce 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -234,13 +234,13 @@ def test_pcre_hook_matching(tempdir_factory, store): _test_hook_repo( tempdir_factory, store, 'pcre_hooks_repo', 'regex-with-quotes', ['herp', 'derp'], b"herp:2:herpfoo'bard\n", - expected_return_code=123, + expected_return_code=1, ) _test_hook_repo( tempdir_factory, store, 'pcre_hooks_repo', 'other-regex', ['herp', 'derp'], b'derp:1:[INFO] information yo\n', - expected_return_code=123, + expected_return_code=1, ) @@ -255,7 +255,7 @@ def test_pcre_hook_case_insensitive_option(tempdir_factory, store): _test_hook_repo( tempdir_factory, store, 'pcre_hooks_repo', 'regex-with-grep-args', ['herp'], b'herp:1:FoOoOoObar\n', - expected_return_code=123, + expected_return_code=1, ) @@ -264,7 +264,7 @@ def test_pcre_hook_case_insensitive_option(tempdir_factory, store): def test_pcre_many_files(tempdir_factory, store): # This is intended to simulate lots of passing files and one failing file # to make sure it still fails. This is not the case when naively using - # a system hook with `grep -H -n '...'` and expected_return_code=123. + # a system hook with `grep -H -n '...'` and expected_return_code=1. path = git_dir(tempdir_factory) with cwd(path): with io.open('herp', 'w') as herp: @@ -275,7 +275,7 @@ def test_pcre_many_files(tempdir_factory, store): 'other-regex', ['/dev/null'] * 15000 + ['herp'], b'herp:1:[INFO] info\n', - expected_return_code=123, + expected_return_code=1, ) @@ -355,7 +355,7 @@ def test_additional_python_dependencies_installed(tempdir_factory, store): config = make_config_from_repo(path) config['hooks'][0]['additional_dependencies'] = ['mccabe'] repo = Repository.create(config, store) - repo.run_hook(repo.hooks[0][1], []) + repo.require_installed() with python.in_env(repo.cmd_runner, 'default'): output = cmd_output('pip', 'freeze', '-l')[1] assert 'mccabe' in output @@ -367,11 +367,11 @@ def test_additional_dependencies_roll_forward(tempdir_factory, store): config = make_config_from_repo(path) # Run the repo once without additional_dependencies repo = Repository.create(config, store) - repo.run_hook(repo.hooks[0][1], []) + repo.require_installed() # Now run it with additional_dependencies config['hooks'][0]['additional_dependencies'] = ['mccabe'] repo = Repository.create(config, store) - repo.run_hook(repo.hooks[0][1], []) + repo.require_installed() # We should see our additional dependency installed with python.in_env(repo.cmd_runner, 'default'): output = cmd_output('pip', 'freeze', '-l')[1] @@ -388,7 +388,7 @@ def test_additional_ruby_dependencies_installed( config = make_config_from_repo(path) config['hooks'][0]['additional_dependencies'] = ['thread_safe'] repo = Repository.create(config, store) - repo.run_hook(repo.hooks[0][1], []) + repo.require_installed() with ruby.in_env(repo.cmd_runner, 'default'): output = cmd_output('gem', 'list', '--local')[1] assert 'thread_safe' in output @@ -405,7 +405,7 @@ def test_additional_node_dependencies_installed( # Careful to choose a small package that's not depped by npm config['hooks'][0]['additional_dependencies'] = ['lodash'] repo = Repository.create(config, store) - repo.run_hook(repo.hooks[0][1], []) + repo.require_installed() with node.in_env(repo.cmd_runner, 'default'): cmd_output('npm', 'config', 'set', 'global', 'true') output = cmd_output('npm', 'ls')[1] diff --git a/tests/xargs_test.py b/tests/xargs_test.py new file mode 100644 index 00000000..239f1a2d --- /dev/null +++ b/tests/xargs_test.py @@ -0,0 +1,47 @@ +from __future__ import absolute_import +from __future__ import unicode_literals + +import pytest + +from pre_commit import xargs + + +def test_partition_trivial(): + assert xargs.partition(('cmd',), ()) == (('cmd',),) + + +def test_partition_simple(): + assert xargs.partition(('cmd',), ('foo',)) == (('cmd', 'foo'),) + + +def test_partition_limits(): + ret = xargs.partition( + ('ninechars',), ( + # Just match the end (with spaces) + '.' * 5, '.' * 4, + # Just match the end (single arg) + '.' * 10, + # Goes over the end + '.' * 5, + '.' * 6, + ), + _max_length=20, + ) + assert ret == ( + ('ninechars', '.' * 5, '.' * 4), + ('ninechars', '.' * 10), + ('ninechars', '.' * 5), + ('ninechars', '.' * 6), + ) + + +def test_argument_too_long(): + with pytest.raises(xargs.ArgumentTooLongError): + xargs.partition(('a' * 5,), ('a' * 5,), _max_length=10) + + +def test_xargs_smoke(): + ret, out, err = xargs.xargs(('echo',), ('hello', 'world')) + assert ret == 0 + assert out == b'hello world\n' + assert err == b'' From a932315a15fdc7903b2fadb2d68a5ef28e580728 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 21 Mar 2016 19:56:41 -0700 Subject: [PATCH 4/6] Implement 'negate' to simplify pcre --- pre_commit/languages/pcre.py | 26 ++++++++------------------ pre_commit/util.py | 4 ---- pre_commit/xargs.py | 22 ++++++++++++++++++---- tests/util_test.py | 13 ------------- tests/xargs_test.py | 25 +++++++++++++++++++++++++ 5 files changed, 51 insertions(+), 39 deletions(-) diff --git a/pre_commit/languages/pcre.py b/pre_commit/languages/pcre.py index 0b2cfd18..faba1da0 100644 --- a/pre_commit/languages/pcre.py +++ b/pre_commit/languages/pcre.py @@ -2,7 +2,6 @@ from __future__ import unicode_literals from sys import platform -from pre_commit.util import shell_escape from pre_commit.xargs import xargs @@ -19,21 +18,12 @@ def install_environment( def run_hook(repo_cmd_runner, hook, file_args): - grep_command = '{0} -H -n -P'.format( - 'ggrep' if platform == 'darwin' else 'grep', - ) - # For PCRE the entry is the regular expression to match - return xargs( - ( - 'sh', '-c', - # Grep usually returns 0 for matches, and nonzero for non-matches - # so we flip it here. - '! {0} {1} {2} $@'.format( - grep_command, ' '.join(hook['args']), - shell_escape(hook['entry']), - ), - '--', - ), - file_args, - ) + cmd = ( + 'ggrep' if platform == 'darwin' else 'grep', + '-H', '-n', '-P', + ) + tuple(hook['args']) + (hook['entry'],) + + # Grep usually returns 0 for matches, and nonzero for non-matches so we + # negate it here. + return xargs(cmd, file_args, negate=True) diff --git a/pre_commit/util.py b/pre_commit/util.py index 853a95b1..046cf96e 100644 --- a/pre_commit/util.py +++ b/pre_commit/util.py @@ -67,10 +67,6 @@ def noop_context(): yield -def shell_escape(arg): - return "'" + arg.replace("'", "'\"'\"'".strip()) + "'" - - def no_git_env(): # Too many bugs dealing with environment variables and GIT: # https://github.com/pre-commit/pre-commit/issues/300 diff --git a/pre_commit/xargs.py b/pre_commit/xargs.py index 2b8aff56..e0b87299 100644 --- a/pre_commit/xargs.py +++ b/pre_commit/xargs.py @@ -42,17 +42,31 @@ def partition(cmd, varargs, _max_length=MAX_LENGTH): return tuple(ret) -def xargs(cmd, varargs): - """A simplified implementation of xargs.""" +def xargs(cmd, varargs, **kwargs): + """A simplified implementation of xargs. + + negate: Make nonzero successful and zero a failure + """ + negate = kwargs.pop('negate', False) retcode = 0 stdout = b'' stderr = b'' - for run_cmd in partition(cmd, varargs): + for run_cmd in partition(cmd, varargs, **kwargs): proc_retcode, proc_out, proc_err = cmd_output( *run_cmd, encoding=None, retcode=None ) - retcode |= proc_retcode + # This is *slightly* too clever so I'll explain it. + # First the xor boolean table: + # T | F | + # +-------+ + # T | F | T | + # --+-------+ + # F | T | F | + # --+-------+ + # When negate is True, it has the effect of flipping the return code + # Otherwise, the retuncode is unchanged + retcode |= bool(proc_retcode) ^ negate stdout += proc_out stderr += proc_err diff --git a/tests/util_test.py b/tests/util_test.py index 1361d639..7fa25bcd 100644 --- a/tests/util_test.py +++ b/tests/util_test.py @@ -9,7 +9,6 @@ import pytest from pre_commit.util import clean_path_on_failure from pre_commit.util import cwd from pre_commit.util import memoize_by_cwd -from pre_commit.util import shell_escape from pre_commit.util import tmpdir @@ -79,18 +78,6 @@ def test_clean_path_on_failure_cleans_for_system_exit(in_tmpdir): assert not os.path.exists('foo') -@pytest.mark.parametrize( - ('input_str', 'expected'), - ( - ('', "''"), - ('foo"bar', "'foo\"bar'"), - ("foo'bar", "'foo'\"'\"'bar'") - ), -) -def test_shell_escape(input_str, expected): - assert shell_escape(input_str) == expected - - def test_tmpdir(): with tmpdir() as tempdir: assert os.path.exists(tempdir) diff --git a/tests/xargs_test.py b/tests/xargs_test.py index 239f1a2d..cb27f62b 100644 --- a/tests/xargs_test.py +++ b/tests/xargs_test.py @@ -45,3 +45,28 @@ def test_xargs_smoke(): assert ret == 0 assert out == b'hello world\n' assert err == b'' + + +exit_cmd = ('bash', '-c', 'exit $1', '--') +# Abuse max_length to control the exit code +max_length = len(' '.join(exit_cmd)) + 2 + + +def test_xargs_negate(): + ret, _, _ = xargs.xargs( + exit_cmd, ('1',), negate=True, _max_length=max_length, + ) + assert ret == 0 + + ret, _, _ = xargs.xargs( + exit_cmd, ('1', '0'), negate=True, _max_length=max_length, + ) + assert ret == 1 + + +def test_xargs_retcode_normal(): + ret, _, _ = xargs.xargs(exit_cmd, ('0',), _max_length=max_length) + assert ret == 0 + + ret, _, _ = xargs.xargs(exit_cmd, ('0', '1'), _max_length=max_length) + assert ret == 1 From 82369fd99f0a9a30ec4039f0703bdaf8c6cc5a3b Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Mon, 21 Mar 2016 21:08:44 -0700 Subject: [PATCH 5/6] Add utility for parsing shebangs and resolving PATH --- pre_commit/commands/install_uninstall.py | 10 +- pre_commit/parse_shebang.py | 95 ++++++++++++++ pre_commit/util.py | 13 +- tests/commands/install_uninstall_test.py | 4 +- tests/parse_shebang_test.py | 154 +++++++++++++++++++++++ tests/prefixed_command_runner_test.py | 4 +- 6 files changed, 267 insertions(+), 13 deletions(-) create mode 100644 pre_commit/parse_shebang.py create mode 100644 tests/parse_shebang_test.py diff --git a/pre_commit/commands/install_uninstall.py b/pre_commit/commands/install_uninstall.py index 9ab6fc57..a60f7273 100644 --- a/pre_commit/commands/install_uninstall.py +++ b/pre_commit/commands/install_uninstall.py @@ -5,10 +5,10 @@ import io import logging import os import os.path -import stat import sys from pre_commit.logging_handler import LoggingHandler +from pre_commit.util import make_executable from pre_commit.util import mkdirp from pre_commit.util import resource_filename @@ -42,14 +42,6 @@ def is_previous_pre_commit(filename): return any(hash in contents for hash in PREVIOUS_IDENTIFYING_HASHES) -def make_executable(filename): - original_mode = os.stat(filename).st_mode - os.chmod( - filename, - original_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH, - ) - - def install(runner, overwrite=False, hooks=False, hook_type='pre-commit'): """Install the pre-commit hooks.""" hook_path = runner.get_hook_path(hook_type) diff --git a/pre_commit/parse_shebang.py b/pre_commit/parse_shebang.py new file mode 100644 index 00000000..df10c6d3 --- /dev/null +++ b/pre_commit/parse_shebang.py @@ -0,0 +1,95 @@ +from __future__ import absolute_import +from __future__ import unicode_literals + +import io +import os.path +import shlex +import string + +from pre_commit import five + + +printable = frozenset(string.printable) + + +def parse_bytesio(bytesio): + """Parse the shebang from a file opened for reading binary.""" + if bytesio.read(2) != b'#!': + return () + first_line = bytesio.readline() + try: + first_line = first_line.decode('US-ASCII') + except UnicodeDecodeError: + return () + + # Require only printable ascii + for c in first_line: + if c not in printable: + return () + + # shlex.split is horribly broken in py26 on text strings + cmd = tuple(shlex.split(five.n(first_line))) + if cmd[0] == '/usr/bin/env': + cmd = cmd[1:] + return cmd + + +def parse_filename(filename): + """Parse the shebang given a filename.""" + if not os.path.exists(filename) or not os.access(filename, os.X_OK): + return () + + with io.open(filename, 'rb') as f: + return parse_bytesio(f) + + +def find_executable(exe, _environ=None): + exe = os.path.normpath(exe) + if os.sep in exe: + return exe + + environ = _environ if _environ is not None else os.environ + + if 'PATHEXT' in environ: + possible_exe_names = (exe,) + tuple( + exe + ext.lower() for ext in environ['PATHEXT'].split(os.pathsep) + ) + else: + possible_exe_names = (exe,) + + for path in environ.get('PATH', '').split(os.pathsep): + for possible_exe_name in possible_exe_names: + joined = os.path.join(path, possible_exe_name) + if os.path.isfile(joined) and os.access(joined, os.X_OK): + return joined + else: + return None + + +def normexe(orig_exe): + if os.sep not in orig_exe: + exe = find_executable(orig_exe) + if exe is None: + raise OSError('Executable {0} not found'.format(orig_exe)) + return exe + else: + return orig_exe + + +def normalize_cmd(cmd): + """Fixes for the following issues on windows + - http://bugs.python.org/issue8557 + - windows does not parse shebangs + + This function also makes deep-path shebangs work just fine + """ + # Use PATH to determine the executable + exe = normexe(cmd[0]) + + # Figure out the shebang from the resulting command + cmd = parse_filename(exe) + (exe,) + cmd[1:] + + # This could have given us back another bare executable + exe = normexe(cmd[0]) + + return (exe,) + cmd[1:] diff --git a/pre_commit/util.py b/pre_commit/util.py index 046cf96e..57303f56 100644 --- a/pre_commit/util.py +++ b/pre_commit/util.py @@ -14,6 +14,7 @@ import tempfile import pkg_resources from pre_commit import five +from pre_commit import parse_shebang @contextlib.contextmanager @@ -110,6 +111,14 @@ def resource_filename(filename): ) +def make_executable(filename): + original_mode = os.stat(filename).st_mode + os.chmod( + filename, + original_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH, + ) + + class CalledProcessError(RuntimeError): def __init__(self, returncode, cmd, expected_returncode, output=None): super(CalledProcessError, self).__init__( @@ -166,12 +175,14 @@ def cmd_output(*cmd, **kwargs): } # py2/py3 on windows are more strict about the types here - cmd = [five.n(arg) for arg in cmd] + cmd = tuple(five.n(arg) for arg in cmd) kwargs['env'] = dict( (five.n(key), five.n(value)) for key, value in kwargs.pop('env', {}).items() ) or None + cmd = parse_shebang.normalize_cmd(cmd) + popen_kwargs.update(kwargs) proc = __popen(cmd, **popen_kwargs) stdout, stderr = proc.communicate() diff --git a/tests/commands/install_uninstall_test.py b/tests/commands/install_uninstall_test.py index 7717a1f0..331d857f 100644 --- a/tests/commands/install_uninstall_test.py +++ b/tests/commands/install_uninstall_test.py @@ -15,12 +15,12 @@ from pre_commit.commands.install_uninstall import IDENTIFYING_HASH from pre_commit.commands.install_uninstall import install from pre_commit.commands.install_uninstall import is_our_pre_commit from pre_commit.commands.install_uninstall import is_previous_pre_commit -from pre_commit.commands.install_uninstall import make_executable from pre_commit.commands.install_uninstall import PREVIOUS_IDENTIFYING_HASHES from pre_commit.commands.install_uninstall import uninstall from pre_commit.runner import Runner from pre_commit.util import cmd_output from pre_commit.util import cwd +from pre_commit.util import make_executable from pre_commit.util import mkdirp from pre_commit.util import resource_filename from testing.fixtures import git_dir @@ -473,6 +473,8 @@ def test_installed_from_venv(tempdir_factory): 'TERM': os.environ.get('TERM', ''), # Windows needs this to import `random` 'SYSTEMROOT': os.environ.get('SYSTEMROOT', ''), + # Windows needs this to resolve executables + 'PATHEXT': os.environ.get('PATHEXT', ''), }, ) assert ret == 0 diff --git a/tests/parse_shebang_test.py b/tests/parse_shebang_test.py new file mode 100644 index 00000000..c26ff73f --- /dev/null +++ b/tests/parse_shebang_test.py @@ -0,0 +1,154 @@ +from __future__ import absolute_import +from __future__ import unicode_literals + +import contextlib +import distutils.spawn +import io +import os +import sys + +import pytest + +from pre_commit import parse_shebang +from pre_commit.envcontext import envcontext +from pre_commit.envcontext import Var +from pre_commit.util import make_executable + + +@pytest.mark.parametrize( + ('s', 'expected'), + ( + (b'', ()), + (b'#!/usr/bin/python', ('/usr/bin/python',)), + (b'#!/usr/bin/env python', ('python',)), + (b'#! /usr/bin/python', ('/usr/bin/python',)), + (b'#!/usr/bin/foo python', ('/usr/bin/foo', 'python')), + (b'\xf9\x93\x01\x42\xcd', ()), + (b'#!\xf9\x93\x01\x42\xcd', ()), + (b'#!\x00\x00\x00\x00', ()), + ), +) +def test_parse_bytesio(s, expected): + assert parse_shebang.parse_bytesio(io.BytesIO(s)) == expected + + +def test_file_doesnt_exist(): + assert parse_shebang.parse_filename('herp derp derp') == () + + +@pytest.mark.xfail( + sys.platform == 'win32', reason='Windows says everything is X_OK', +) +def test_file_not_executable(tmpdir): + x = tmpdir.join('f') + x.write_text('#!/usr/bin/env python', encoding='UTF-8') + assert parse_shebang.parse_filename(x.strpath) == () + + +def test_simple_case(tmpdir): + x = tmpdir.join('f') + x.write_text('#!/usr/bin/env python', encoding='UTF-8') + make_executable(x.strpath) + assert parse_shebang.parse_filename(x.strpath) == ('python',) + + +def test_find_executable_full_path(): + assert parse_shebang.find_executable(sys.executable) == sys.executable + + +def test_find_executable_on_path(): + expected = distutils.spawn.find_executable('echo') + assert parse_shebang.find_executable('echo') == expected + + +def test_find_executable_not_found_none(): + assert parse_shebang.find_executable('not-a-real-executable') is None + + +def write_executable(shebang, filename='run'): + os.mkdir('bin') + path = os.path.join('bin', filename) + with io.open(path, 'w') as f: + f.write('#!{0}'.format(shebang)) + make_executable(path) + return path + + +@contextlib.contextmanager +def bin_on_path(): + bindir = os.path.join(os.getcwd(), 'bin') + with envcontext((('PATH', (bindir, os.pathsep, Var('PATH'))),)): + yield + + +def test_find_executable_path_added(in_tmpdir): + path = os.path.abspath(write_executable('/usr/bin/env sh')) + assert parse_shebang.find_executable('run') is None + with bin_on_path(): + assert parse_shebang.find_executable('run') == path + + +def test_find_executable_path_ext(in_tmpdir): + """Windows exports PATHEXT as a list of extensions to automatically add + to executables when doing PATH searching. + """ + exe_path = os.path.abspath(write_executable( + '/usr/bin/env sh', filename='run.myext', + )) + env_path = {'PATH': os.path.dirname(exe_path)} + env_path_ext = dict(env_path, PATHEXT=os.pathsep.join(('.exe', '.myext'))) + assert parse_shebang.find_executable('run') is None + assert parse_shebang.find_executable('run', _environ=env_path) is None + ret = parse_shebang.find_executable('run.myext', _environ=env_path) + assert ret == exe_path + ret = parse_shebang.find_executable('run', _environ=env_path_ext) + assert ret == exe_path + + +def test_normexe_does_not_exist(): + with pytest.raises(OSError) as excinfo: + parse_shebang.normexe('i-dont-exist-lol') + assert excinfo.value.args == ('Executable i-dont-exist-lol not found',) + + +def test_normexe_already_full_path(): + assert parse_shebang.normexe(sys.executable) == sys.executable + + +def test_normexe_gives_full_path(): + expected = distutils.spawn.find_executable('echo') + assert parse_shebang.normexe('echo') == expected + assert os.sep in expected + + +def test_normalize_cmd_trivial(): + cmd = (distutils.spawn.find_executable('echo'), 'hi') + assert parse_shebang.normalize_cmd(cmd) == cmd + + +def test_normalize_cmd_PATH(): + cmd = ('python', '--version') + expected = (distutils.spawn.find_executable('python'), '--version') + assert parse_shebang.normalize_cmd(cmd) == expected + + +def test_normalize_cmd_shebang(in_tmpdir): + python = distutils.spawn.find_executable('python') + path = write_executable(python.replace(os.sep, '/')) + assert parse_shebang.normalize_cmd((path,)) == (python, path) + + +def test_normalize_cmd_PATH_shebang_full_path(in_tmpdir): + python = distutils.spawn.find_executable('python') + path = write_executable(python.replace(os.sep, '/')) + with bin_on_path(): + ret = parse_shebang.normalize_cmd(('run',)) + assert ret == (python, os.path.abspath(path)) + + +def test_normalize_cmd_PATH_shebang_PATH(in_tmpdir): + python = distutils.spawn.find_executable('python') + path = write_executable('/usr/bin/env python') + with bin_on_path(): + ret = parse_shebang.normalize_cmd(('run',)) + assert ret == (python, os.path.abspath(path)) diff --git a/tests/prefixed_command_runner_test.py b/tests/prefixed_command_runner_test.py index 3f691b4b..bb412101 100644 --- a/tests/prefixed_command_runner_test.py +++ b/tests/prefixed_command_runner_test.py @@ -78,7 +78,7 @@ def test_run_substitutes_prefix(popen_mock, makedirs_mock): ) ret = instance.run(['{prefix}bar', 'baz'], retcode=None) popen_mock.assert_called_once_with( - [five.n(os.path.join('prefix', 'bar')), five.n('baz')], + (five.n(os.path.join('prefix', 'bar')), five.n('baz')), env=None, stdin=subprocess.PIPE, stdout=subprocess.PIPE, @@ -132,4 +132,4 @@ def test_raises_on_error(popen_mock, makedirs_mock): instance = PrefixedCommandRunner( '.', popen=popen_mock, makedirs=makedirs_mock, ) - instance.run(['foo']) + instance.run(['echo']) From f8c82f99fdcf65060f2b5e549b971f62412d9401 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Thu, 7 Apr 2016 21:33:10 -0700 Subject: [PATCH 6/6] Make the pcre check for a more compliant implementation --- testing/util.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testing/util.py b/testing/util.py index 4275a28a..a234adf6 100644 --- a/testing/util.py +++ b/testing/util.py @@ -75,8 +75,8 @@ xfailif_windows_no_node = pytest.mark.xfail( def platform_supports_pcre(): - output = cmd_output('grep', '-P', 'setup', 'setup.py', retcode=None) - return output[0] == 0 and 'from setuptools import setup' in output[1] + output = cmd_output('grep', '-P', "name='pre", 'setup.py', retcode=None) + return output[0] == 0 and "name='pre_commit'," in output[1] xfailif_no_pcre_support = pytest.mark.xfail(