From aa9c77abe891f3f85bdeab34521d9c7ec0621fee Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sat, 29 Mar 2014 13:00:24 -0700 Subject: [PATCH 1/8] Adds failing test for cwd problem. --- .../resources/prints_cwd_repo/manifest.yaml | 4 ++ .../prints_cwd_repo/prints_cwd/__init__.py | 0 .../prints_cwd_repo/prints_cwd/main.py | 5 ++ testing/resources/prints_cwd_repo/setup.py | 11 ++++ tests/conftest.py | 55 +++++++++++-------- tests/repository_test.py | 22 ++++++-- 6 files changed, 69 insertions(+), 28 deletions(-) create mode 100644 testing/resources/prints_cwd_repo/manifest.yaml create mode 100644 testing/resources/prints_cwd_repo/prints_cwd/__init__.py create mode 100644 testing/resources/prints_cwd_repo/prints_cwd/main.py create mode 100644 testing/resources/prints_cwd_repo/setup.py diff --git a/testing/resources/prints_cwd_repo/manifest.yaml b/testing/resources/prints_cwd_repo/manifest.yaml new file mode 100644 index 00000000..4a716c21 --- /dev/null +++ b/testing/resources/prints_cwd_repo/manifest.yaml @@ -0,0 +1,4 @@ +- id: prints_cwd + name: Prints Cwd + entry: prints_cwd + language: python diff --git a/testing/resources/prints_cwd_repo/prints_cwd/__init__.py b/testing/resources/prints_cwd_repo/prints_cwd/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/testing/resources/prints_cwd_repo/prints_cwd/main.py b/testing/resources/prints_cwd_repo/prints_cwd/main.py new file mode 100644 index 00000000..5e854ab1 --- /dev/null +++ b/testing/resources/prints_cwd_repo/prints_cwd/main.py @@ -0,0 +1,5 @@ +import os + +def func(): + print os.getcwd() + return 0 diff --git a/testing/resources/prints_cwd_repo/setup.py b/testing/resources/prints_cwd_repo/setup.py new file mode 100644 index 00000000..be840725 --- /dev/null +++ b/testing/resources/prints_cwd_repo/setup.py @@ -0,0 +1,11 @@ +from setuptools import find_packages +from setuptools import setup + +setup( + name='prints_cwd', + version='0.0.0', + packages=find_packages('.'), + entry_points={ + 'console_scripts': ['prints_cwd = prints_cwd.main:func'], + }, +) diff --git a/tests/conftest.py b/tests/conftest.py index f2ac3d6c..3b989779 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -32,37 +32,33 @@ def dummy_git_repo(empty_git_dir): yield empty_git_dir +def _make_repo(repo_path, repo_source): + copy_tree_to_path(get_resource_path(repo_source), repo_path) + add_and_commit() + return repo_path + + @pytest.yield_fixture def python_hooks_repo(dummy_git_repo): - copy_tree_to_path( - get_resource_path('python_hooks_repo'), - dummy_git_repo, - ) - add_and_commit() - yield dummy_git_repo + yield _make_repo(dummy_git_repo, 'python_hooks_repo') @pytest.yield_fixture def node_hooks_repo(dummy_git_repo): - copy_tree_to_path( - get_resource_path('node_hooks_repo'), - dummy_git_repo, - ) - add_and_commit() - yield dummy_git_repo + yield _make_repo(dummy_git_repo, 'node_hooks_repo') @pytest.yield_fixture def consumer_repo(dummy_git_repo): - copy_tree_to_path( - get_resource_path('consumer_repo'), - dummy_git_repo, - ) - add_and_commit() - yield dummy_git_repo + yield _make_repo(dummy_git_repo, 'consumer_repo') -@pytest.fixture +@pytest.yield_fixture +def prints_cwd_repo(dummy_git_repo): + yield _make_repo(dummy_git_repo, 'prints_cwd_repo') + + +@pytest.yield_fixture def config_for_node_hooks_repo(node_hooks_repo): config = { 'repo': node_hooks_repo, @@ -74,11 +70,10 @@ def config_for_node_hooks_repo(node_hooks_repo): } jsonschema.validate([config], CONFIG_JSON_SCHEMA) validate_config_extra([config]) - - return config + yield config -@pytest.fixture +@pytest.yield_fixture def config_for_python_hooks_repo(python_hooks_repo): config = { 'repo': python_hooks_repo, @@ -90,5 +85,19 @@ def config_for_python_hooks_repo(python_hooks_repo): } jsonschema.validate([config], CONFIG_JSON_SCHEMA) validate_config_extra([config]) + yield config - return config + +@pytest.yield_fixture +def config_for_prints_cwd_repo(prints_cwd_repo): + config = { + 'repo': prints_cwd_repo, + 'sha': git.get_head_sha(prints_cwd_repo), + 'hooks': [{ + 'id': 'prints_cwd', + 'files': '\.py$', + }], + } + jsonschema.validate([config], CONFIG_JSON_SCHEMA) + validate_config_extra([config]) + yield config diff --git a/tests/repository_test.py b/tests/repository_test.py index 0f3ccf35..35550b3b 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -5,6 +5,7 @@ import pytest import pre_commit.constants as C from pre_commit import git from pre_commit.clientlib.validate_config import CONFIG_JSON_SCHEMA +from pre_commit.clientlib.validate_config import validate_config_extra from pre_commit.repository import Repository @@ -28,13 +29,13 @@ def test_create_repo_in_env(dummy_repo_config, dummy_git_repo): ) @pytest.mark.integration -def test_install_python_repo_in_env(python_hooks_repo, config_for_python_hooks_repo): +def test_install_python_repo_in_env(config_for_python_hooks_repo): repo = Repository(config_for_python_hooks_repo) repo.install() assert os.path.exists( os.path.join( - python_hooks_repo, + repo.repo_url, C.HOOKS_WORKSPACE, repo.sha, 'py_env', @@ -61,6 +62,17 @@ def test_run_a_hook_lots_of_files(config_for_python_hooks_repo): assert ret[0] == 0 +@pytest.mark.xfail +@pytest.mark.integration +def test_cwd_of_hook(config_for_prints_cwd_repo): + repo = Repository(config_for_prints_cwd_repo) + repo.install() + ret = repo.run_hook('prints_cwd', []) + + assert ret[0] == 0 + assert ret[1] == '{0}\n'.format(repo.repo_url) + + @pytest.mark.skipif( os.environ.get('slowtests', None) == 'false', reason="TODO: make this test not super slow", @@ -74,6 +86,7 @@ def test_run_a_node_hook(config_for_node_hooks_repo): assert ret[0] == 0 assert ret[1] == 'Hello World\n' + @pytest.fixture def mock_repo_config(): config = { @@ -81,12 +94,11 @@ def mock_repo_config(): 'sha': '5e713f8878b7d100c0e059f8cc34be4fc2e8f897', 'hooks': [{ 'id': 'pyflakes', - 'files': '*.py', + 'files': '\.py$', }], } - jsonschema.validate([config], CONFIG_JSON_SCHEMA) - + validate_config_extra([config]) return config From bd6e62e28d0398a6f91c808e10cd0d8b8e11208d Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sat, 29 Mar 2014 15:17:01 -0700 Subject: [PATCH 2/8] Add PrefixedCommandRunner --- pre_commit/prefixed_command_runner.py | 42 +++++++++++++++ tests/prefixed_command_runner_test.py | 78 +++++++++++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100644 pre_commit/prefixed_command_runner.py create mode 100644 tests/prefixed_command_runner_test.py diff --git a/pre_commit/prefixed_command_runner.py b/pre_commit/prefixed_command_runner.py new file mode 100644 index 00000000..d079d7ea --- /dev/null +++ b/pre_commit/prefixed_command_runner.py @@ -0,0 +1,42 @@ + +import os +import os.path +import subprocess + + +def _replace_cmd(cmd, **kwargs): + return [part.format(**kwargs) for part in cmd] + + +class PrefixedCommandRunner(object): + """A PrefixedCommandRunner allows you to run subprocess commands with + comand substitution. + + For instance: + PrefixedCommandRunner('/tmp/foo').run(['{prefix}foo.sh', 'bar', 'baz']) + + will run ['/tmpl/foo/foo.sh', 'bar', 'baz'] + """ + def __init__(self, prefix_dir, popen=subprocess.Popen): + self.prefix_dir = prefix_dir.rstrip(os.sep) + os.sep + self.__popen = popen + + def run(self, cmd, stdin=None): + replaced_cmd = _replace_cmd(cmd, prefix=self.prefix_dir) + proc = self.__popen( + replaced_cmd, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + stdout, stderr = proc.communicate(stdin) + + return proc.returncode, stdout, stderr + + @classmethod + def from_command_runner(cls, command_runner, prefix_postfix): + """Constructs a new command runner from an existing one by appending + `prefix_postfix` to the command runner's prefix directory. + """ + new_prefix = os.path.join(command_runner.prefix_dir, prefix_postfix) + return cls(new_prefix, popen=command_runner.__popen) diff --git a/tests/prefixed_command_runner_test.py b/tests/prefixed_command_runner_test.py new file mode 100644 index 00000000..33100c69 --- /dev/null +++ b/tests/prefixed_command_runner_test.py @@ -0,0 +1,78 @@ + +import mock +import pytest +import subprocess + +from pre_commit.prefixed_command_runner import _replace_cmd +from pre_commit.prefixed_command_runner import PrefixedCommandRunner + + +@pytest.fixture +def popen_mock(): + popen = mock.Mock() + popen.return_value.communicate.return_value = (mock.Mock(), mock.Mock()) + return popen + + +@pytest.mark.parametrize(('input', 'kwargs', 'expected_output'), ( + ([], {}, []), + (['foo'], {}, ['foo']), + ([], {'foo': 'bar'}, []), + (['{foo}/baz'], {'foo': 'bar'}, ['bar/baz']), + (['foo'], {'foo': 'bar'}, ['foo']), + (['foo', '{bar}'], {'bar': 'baz'}, ['foo', 'baz']), +)) +def test_replace_cmd(input, kwargs, expected_output): + ret = _replace_cmd(input, **kwargs) + assert ret == expected_output + + +@pytest.mark.parametrize(('input', 'expected_prefix'), ( + ('.', './'), + ('foo', 'foo/'), + ('bar/', 'bar/'), + ('foo/bar', 'foo/bar/'), + ('foo/bar/', 'foo/bar/'), +)) +def test_init_normalizes_path_endings(input, expected_prefix): + instance = PrefixedCommandRunner(input) + assert instance.prefix_dir == expected_prefix + + +def test_run_substitutes_prefix(popen_mock): + instance = PrefixedCommandRunner('prefix', popen=popen_mock) + ret = instance.run(['{prefix}bar', 'baz']) + popen_mock.assert_called_once_with( + ['prefix/bar', 'baz'], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + assert ret == ( + popen_mock.return_value.returncode, + popen_mock.return_value.communicate.return_value[0], + popen_mock.return_value.communicate.return_value[1], + ) + + +@pytest.mark.parametrize(('first_prefix', 'postfix', 'expected_output'), ( + ('foo', '', 'foo/'), + ('foo', 'bar', 'foo/bar/'), + ('./', 'bar', './bar/'), +)) +def test_from_command_runner(first_prefix, postfix, expected_output): + first = PrefixedCommandRunner(first_prefix) + second = PrefixedCommandRunner.from_command_runner(first, postfix) + assert second.prefix_dir == expected_output + + +def test_from_command_runner_preserves_popen(popen_mock): + first = PrefixedCommandRunner('foo', popen=popen_mock) + second = PrefixedCommandRunner.from_command_runner(first, 'bar') + second.run(['foo/bar/baz']) + popen_mock.assert_called_once_with( + ['foo/bar/baz'], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) From 6f0d566199ad4c4937dd048a567013fd65844c6f Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sat, 29 Mar 2014 15:47:29 -0700 Subject: [PATCH 3/8] Added path function to PrefixedCommandRunner --- pre_commit/prefixed_command_runner.py | 11 +++++---- pre_commit/runner.py | 5 ++++ tests/prefixed_command_runner_test.py | 34 ++++++++++++++++++++------- tests/runner_test.py | 6 +++++ 4 files changed, 44 insertions(+), 12 deletions(-) diff --git a/pre_commit/prefixed_command_runner.py b/pre_commit/prefixed_command_runner.py index d079d7ea..0b3012c1 100644 --- a/pre_commit/prefixed_command_runner.py +++ b/pre_commit/prefixed_command_runner.py @@ -33,10 +33,13 @@ class PrefixedCommandRunner(object): return proc.returncode, stdout, stderr + def path(self, path_end): + path = os.path.join(self.prefix_dir, path_end) + return os.path.normpath(path) + @classmethod - def from_command_runner(cls, command_runner, prefix_postfix): + def from_command_runner(cls, command_runner, path_end): """Constructs a new command runner from an existing one by appending - `prefix_postfix` to the command runner's prefix directory. + `path_end` to the command runner's prefix directory. """ - new_prefix = os.path.join(command_runner.prefix_dir, prefix_postfix) - return cls(new_prefix, popen=command_runner.__popen) + return cls(command_runner.path(path_end), popen=command_runner.__popen) diff --git a/pre_commit/runner.py b/pre_commit/runner.py index 0c17cd76..b7263356 100644 --- a/pre_commit/runner.py +++ b/pre_commit/runner.py @@ -5,6 +5,7 @@ import os.path import pre_commit.constants as C from pre_commit import git from pre_commit.clientlib.validate_config import load_config +from pre_commit.prefixed_command_runner import PrefixedCommandRunner from pre_commit.repository import Repository from pre_commit.util import cached_property @@ -44,3 +45,7 @@ class Runner(object): @cached_property def pre_commit_path(self): return os.path.join(self.git_root, '.git/hooks/pre-commit') + + @cached_property + def workspace_runner(self): + return PrefixedCommandRunner(C.HOOKS_WORKSPACE) diff --git a/tests/prefixed_command_runner_test.py b/tests/prefixed_command_runner_test.py index 33100c69..630d8a4a 100644 --- a/tests/prefixed_command_runner_test.py +++ b/tests/prefixed_command_runner_test.py @@ -1,4 +1,5 @@ +import os import mock import pytest import subprocess @@ -55,14 +56,31 @@ def test_run_substitutes_prefix(popen_mock): ) -@pytest.mark.parametrize(('first_prefix', 'postfix', 'expected_output'), ( - ('foo', '', 'foo/'), - ('foo', 'bar', 'foo/bar/'), - ('./', 'bar', './bar/'), -)) -def test_from_command_runner(first_prefix, postfix, expected_output): - first = PrefixedCommandRunner(first_prefix) - second = PrefixedCommandRunner.from_command_runner(first, postfix) +PATH_TESTS = ( + ('foo', '', 'foo'), + ('foo', 'bar', 'foo/bar'), + ('foo/bar', '../baz', 'foo/baz'), + ('./', 'bar', 'bar'), + ('./', '', '.'), +) + + +@pytest.mark.parametrize(('prefix', 'path_end', 'expected_output'), PATH_TESTS) +def test_path(prefix, path_end, expected_output): + instance = PrefixedCommandRunner(prefix) + ret = instance.path(path_end) + assert ret == expected_output + + +@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 diff --git a/tests/runner_test.py b/tests/runner_test.py index 167b7bb6..beac918e 100644 --- a/tests/runner_test.py +++ b/tests/runner_test.py @@ -59,3 +59,9 @@ def test_pre_commit_path(): runner = Runner('foo/bar') expected_path = os.path.join('foo/bar', '.git/hooks/pre-commit') assert runner.pre_commit_path == expected_path + + +def test_workspace_runneR(): + runner = Runner('foo/bar') + ret = runner.workspace_runner + assert ret.prefix_dir == C.HOOKS_WORKSPACE + '/' From 216b5c6ab1bad14914d7bcbf6a1cb0b63c9bd309 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sat, 29 Mar 2014 23:23:43 -0700 Subject: [PATCH 4/8] Resolves cwd problem --- .travis.yml | 2 +- pre_commit/languages/all.py | 12 ++++- pre_commit/languages/helpers.py | 18 +++---- pre_commit/languages/node.py | 47 ++++++++++-------- pre_commit/languages/python.py | 25 +++++----- pre_commit/languages/ruby.py | 29 +++++------- pre_commit/prefixed_command_runner.py | 24 ++++++++-- pre_commit/repository.py | 68 ++++++++++++++++++++------- pre_commit/run.py | 7 +-- pre_commit/runner.py | 4 +- tests/prefixed_command_runner_test.py | 54 ++++++++++++++++++--- tests/repository_test.py | 23 +++++---- tests/runner_test.py | 6 +-- 13 files changed, 212 insertions(+), 107 deletions(-) diff --git a/.travis.yml b/.travis.yml index 976fde03..f5f7a807 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,7 +5,7 @@ python: - 2.7 install: pip install virtualenv -script: make +script: make coverage before_install: diff --git a/pre_commit/languages/all.py b/pre_commit/languages/all.py index 84f30771..858af7e5 100644 --- a/pre_commit/languages/all.py +++ b/pre_commit/languages/all.py @@ -5,14 +5,22 @@ from pre_commit.languages import ruby # A language implements the following two functions in its module: # -# def install_environment(): +# def install_environment(repo_cmd_runner): # """Installs a repository in the given repository. Note that the current # working directory will already be inside the repository. +# +# Args: +# repo_cmd_runner - `PrefixedCommandRunner` bound to the repository. # """ # -# def run_hook(hook, file_args): +# def run_hook(repo_cmd_runner, hook, file_args): # """Runs a hook and returns the returncode and output of running that hook. # +# Args: +# repo_cmd_runner - `PrefixedCommandRunner` bound to the repository. +# hook - Hook dictionary +# file_args - The files to be run +# # Returns: # (returncode, stdout, stderr) # """ diff --git a/pre_commit/languages/helpers.py b/pre_commit/languages/helpers.py index 1aaf80a8..4c05f3e7 100644 --- a/pre_commit/languages/helpers.py +++ b/pre_commit/languages/helpers.py @@ -1,15 +1,17 @@ -import subprocess - def run_hook(env, hook, file_args): return env.run( ' '.join(['xargs', hook['entry']] + hook.get('args', [])), stdin='\n'.join(list(file_args) + ['']), + retcode=None, ) class Environment(object): + def __init__(self, repo_cmd_runner): + self.repo_cmd_runner = repo_cmd_runner + @property def env_prefix(self): """env_prefix is a value that is prefixed to the command that is run. @@ -24,14 +26,8 @@ class Environment(object): """ raise NotImplementedError - def run(self, cmd, stdin=None): + def run(self, cmd, **kwargs): """Returns (returncode, stdout, stderr).""" - proc = subprocess.Popen( - ['bash', '-c', ' '.join([self.env_prefix, cmd])], - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + return self.repo_cmd_runner.run( + ['bash', '-c', ' '.join([self.env_prefix, cmd])], **kwargs ) - stdout, stderr = proc.communicate(stdin) - - return proc.returncode, stdout, stderr diff --git a/pre_commit/languages/node.py b/pre_commit/languages/node.py index 0776d75e..ec9da388 100644 --- a/pre_commit/languages/node.py +++ b/pre_commit/languages/node.py @@ -1,5 +1,5 @@ import contextlib -from plumbum import local +import subprocess from pre_commit.languages import helpers from pre_commit.languages import python @@ -12,37 +12,44 @@ class NodeEnv(python.PythonEnv): @property def env_prefix(self): base = super(NodeEnv, self).env_prefix - return ' '.join([base, '. {0}/bin/activate &&'.format(NODE_ENV)]) + return ' '.join([ + base, + '. {{prefix}}{0}/bin/activate &&'.format(NODE_ENV)] + ) @contextlib.contextmanager -def in_env(): - yield NodeEnv() +def in_env(repo_cmd_runner): + yield NodeEnv(repo_cmd_runner) -def install_environment(): - assert local.path('package.json').exists() +def install_environment(repo_cmd_runner): + assert repo_cmd_runner.exists('package.json') - if local.path(NODE_ENV).exists(): + # Return immediately if we already have a virtualenv + if repo_cmd_runner.exists(NODE_ENV): return - local['virtualenv'][python.PY_ENV]() + repo_cmd_runner.run(['virtualenv', '{{prefix}}{0}'.format(python.PY_ENV)]) - with python.in_env() as python_env: + with python.in_env(repo_cmd_runner) as python_env: python_env.run('pip install nodeenv') # Try and use the system level node executable first - retcode, _, _ = python_env.run('nodeenv -n system {0}'.format(NODE_ENV)) - # TODO: log failure here - # cleanup - if retcode: - local.path(NODE_ENV).delete() - python_env.run('nodeenv --jobs 4 {0}'.format(NODE_ENV)) + try: + python_env.run('nodeenv -n system {{prefix}}{0}'.format(NODE_ENV)) + except subprocess.CalledProcessError: + # TODO: log failure here + # cleanup + # TODO: local.path(NODE_ENV).delete() + python_env.run('nodeenv --jobs 4 {{prefix}}{0}'.format(NODE_ENV)) - with in_env() as node_env: - node_env.run('npm install -g') + with in_env(repo_cmd_runner) as node_env: + node_env.run( + 'cd {0} && npm install -g'.format(repo_cmd_runner.prefix_dir), + ) -def run_hook(hook, file_args): - with in_env() as node_env: - return helpers.run_hook(node_env, hook, file_args) +def run_hook(repo_cmd_runner, hook, file_args): + with in_env(repo_cmd_runner) as env: + return helpers.run_hook(env, hook, file_args) diff --git a/pre_commit/languages/python.py b/pre_commit/languages/python.py index 6be6c93b..91d43b2e 100644 --- a/pre_commit/languages/python.py +++ b/pre_commit/languages/python.py @@ -1,34 +1,35 @@ import contextlib -from plumbum import local + from pre_commit.languages import helpers + PY_ENV = 'py_env' class PythonEnv(helpers.Environment): @property def env_prefix(self): - return '. {0}/bin/activate &&'.format(PY_ENV) + return '. {{prefix}}{0}/bin/activate &&'.format(PY_ENV) @contextlib.contextmanager -def in_env(): - yield PythonEnv() +def in_env(repo_cmd_runner): + yield PythonEnv(repo_cmd_runner) -def install_environment(): - assert local.path('setup.py').exists() +def install_environment(repo_cmd_runner): + assert repo_cmd_runner.exists('setup.py') # Return immediately if we already have a virtualenv - if local.path(PY_ENV).exists(): + if repo_cmd_runner.exists(PY_ENV): return # Install a virtualenv - local['virtualenv'][PY_ENV]() - with in_env() as env: - env.run('pip install .') + repo_cmd_runner.run(['virtualenv', '{{prefix}}{0}'.format(PY_ENV)]) + with in_env(repo_cmd_runner) as env: + env.run('cd {0} && pip install .'.format(repo_cmd_runner.prefix_dir)) -def run_hook(hook, file_args): - with in_env() as env: +def run_hook(repo_cmd_runner, hook, file_args): + with in_env(repo_cmd_runner) as env: return helpers.run_hook(env, hook, file_args) diff --git a/pre_commit/languages/ruby.py b/pre_commit/languages/ruby.py index 219f74d7..fcd3ad42 100644 --- a/pre_commit/languages/ruby.py +++ b/pre_commit/languages/ruby.py @@ -1,6 +1,5 @@ import contextlib -from plumbum import local from pre_commit.languages import helpers @@ -8,29 +7,27 @@ from pre_commit.languages import helpers RVM_ENV = 'rvm_env' -class RubyEnv(object): - def __init__(self): - self.env_prefix = '. {0}/.rvm/scripts/rvm'.format(RVM_ENV) - - def run(self, cmd, **kwargs): - return local['bash']['-c', ' '.join([self.env_prefix, cmd])].run(**kwargs) +class RubyEnv(helpers.Environment): + @property + def env_prefix(self): + return '. {{prefix}}{0}/bin/activate &&'.format(RVM_ENV) @contextlib.contextmanager -def in_env(): - yield RubyEnv() +def in_env(repo_cmd_runner): + yield RubyEnv(repo_cmd_runner) -def install_environment(): +def install_environment(repo_cmd_runner): # Return immediately if we already have a virtualenv - if local.path(RVM_ENV).exists(): + if repo_cmd_runner.exists(RVM_ENV): return - local['__rvm-env.sh'][RVM_ENV]() - with in_env() as env: - env.run('bundle install') + repo_cmd_runner.run(['__rvm-env.sh', '{{prefix}}{0}'.format(RVM_ENV)]) + with in_env(repo_cmd_runner) as env: + env.run('bundle install', cwd=repo_cmd_runner.prefix_dir) -def run_hook(hook, file_args): - with in_env() as env: +def run_hook(repo_cmd_runner, hook, file_args): + with in_env(repo_cmd_runner) as env: return helpers.run_hook(env, hook, file_args) diff --git a/pre_commit/prefixed_command_runner.py b/pre_commit/prefixed_command_runner.py index 0b3012c1..b064b1ef 100644 --- a/pre_commit/prefixed_command_runner.py +++ b/pre_commit/prefixed_command_runner.py @@ -17,26 +17,42 @@ class PrefixedCommandRunner(object): will run ['/tmpl/foo/foo.sh', 'bar', 'baz'] """ - def __init__(self, prefix_dir, popen=subprocess.Popen): + def __init__(self, prefix_dir, popen=subprocess.Popen, makedirs=os.makedirs): self.prefix_dir = prefix_dir.rstrip(os.sep) + os.sep self.__popen = popen + self.__makedirs = makedirs - def run(self, cmd, stdin=None): + def _create_path_if_not_exists(self): + if not os.path.exists(self.prefix_dir): + self.__makedirs(self.prefix_dir) + + def run(self, cmd, retcode=0, stdin=None, **kwargs): + self._create_path_if_not_exists() replaced_cmd = _replace_cmd(cmd, prefix=self.prefix_dir) proc = self.__popen( replaced_cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, + **kwargs ) stdout, stderr = proc.communicate(stdin) + returncode = proc.returncode + + if retcode is not None and retcode != returncode: + raise subprocess.CalledProcessError( + returncode, replaced_cmd, output=(stdout, stderr), + ) return proc.returncode, stdout, stderr - def path(self, path_end): - path = os.path.join(self.prefix_dir, path_end) + def path(self, *parts): + path = os.path.join(self.prefix_dir, *parts) return os.path.normpath(path) + 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 diff --git a/pre_commit/repository.py b/pre_commit/repository.py index abb9044b..7e710aaa 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -7,12 +7,15 @@ from pre_commit.clientlib.validate_manifest import load_manifest from pre_commit.hooks_workspace import in_hooks_workspace from pre_commit.languages.all import languages from pre_commit.ordereddict import OrderedDict +from pre_commit.prefixed_command_runner import PrefixedCommandRunner from pre_commit.util import cached_property class Repository(object): def __init__(self, repo_config): self.repo_config = repo_config + self.__created = False + self.__installed = False @cached_property def repo_url(self): @@ -43,13 +46,17 @@ class Repository(object): for hook in load_manifest(C.MANIFEST_FILE) ) - @contextlib.contextmanager - def in_checkout(self): - with in_hooks_workspace(): - # SMELL: - self.create() - with local.cwd(self.sha): - yield + def get_cmd_runner(self, hooks_cmd_runner): + return PrefixedCommandRunner.from_command_runner( + hooks_cmd_runner, self.sha, + ) + + def require_created(self): + if self.__created: + return + + self.create() + self.__created = True def create(self): with in_hooks_workspace(): @@ -61,13 +68,42 @@ class Repository(object): with self.in_checkout(): local['git']['checkout', self.sha]() - def install(self): - with self.in_checkout(): - for language in C.SUPPORTED_LANGUAGES: - if language in self.languages: - languages[language].install_environment() + def require_installed(self, cmd_runner): + if self.__installed: + return - def run_hook(self, hook_id, file_args): - with self.in_checkout(): - hook = self.hooks[hook_id] - return languages[hook['language']].run_hook(hook, file_args) + self.install(cmd_runner) + + def install(self, cmd_runner): + """Install the hook repository. + + Args: + cmd_runner - A `PrefixedCommandRunner` bound to the hooks workspace + """ + self.require_created() + repo_cmd_runner = self.get_cmd_runner(cmd_runner) + for language in C.SUPPORTED_LANGUAGES: + if language in self.languages: + languages[language].install_environment(repo_cmd_runner) + + @contextlib.contextmanager + def in_checkout(self): + self.require_created() + with in_hooks_workspace(): + with local.cwd(self.sha): + yield + + def run_hook(self, cmd_runner, hook_id, file_args): + """Run a hook. + + Args: + cmd_runner - A `PrefixedCommandRunner` bound to the hooks workspace + hook_id - Id of the hook + file_args - List of files to run + """ + self.require_installed(cmd_runner) + repo_cmd_runner = self.get_cmd_runner(cmd_runner) + hook = self.hooks[hook_id] + return languages[hook['language']].run_hook( + repo_cmd_runner, hook, file_args, + ) diff --git a/pre_commit/run.py b/pre_commit/run.py index f83d629f..1fd9d5c0 100644 --- a/pre_commit/run.py +++ b/pre_commit/run.py @@ -18,9 +18,7 @@ COLS = int(subprocess.Popen(['tput', 'cols'], stdout=subprocess.PIPE).communicat PASS_FAIL_LENGTH = 6 -def _run_single_hook(repository, hook_id, all_files=False): - repository.install() - +def _run_single_hook(runner, repository, hook_id, all_files=False): if all_files: get_filenames = git.get_all_files_matching else: @@ -36,6 +34,7 @@ def _run_single_hook(repository, hook_id, all_files=False): ), retcode, stdout, stderr = repository.run_hook( + runner.cmd_runner, hook_id, map(os.path.abspath, get_filenames(hook['files'])), ) @@ -69,6 +68,7 @@ def run_hooks(runner, all_files=False): for repo in runner.repositories: for hook_id in repo.hooks: retval |= _run_single_hook( + runner, repo, hook_id, all_files=all_files, @@ -81,6 +81,7 @@ def run_single_hook(runner, hook_id, all_files=False): for repo in runner.repositories: if hook_id in repo.hooks: return _run_single_hook( + runner, repo, hook_id, all_files=all_files, diff --git a/pre_commit/runner.py b/pre_commit/runner.py index b7263356..93507f22 100644 --- a/pre_commit/runner.py +++ b/pre_commit/runner.py @@ -47,5 +47,5 @@ class Runner(object): return os.path.join(self.git_root, '.git/hooks/pre-commit') @cached_property - def workspace_runner(self): - return PrefixedCommandRunner(C.HOOKS_WORKSPACE) + def cmd_runner(self): + return PrefixedCommandRunner(self.hooks_workspace_path) diff --git a/tests/prefixed_command_runner_test.py b/tests/prefixed_command_runner_test.py index 630d8a4a..22b9e6de 100644 --- a/tests/prefixed_command_runner_test.py +++ b/tests/prefixed_command_runner_test.py @@ -3,6 +3,7 @@ import os import mock import pytest import subprocess +from plumbum import local from pre_commit.prefixed_command_runner import _replace_cmd from pre_commit.prefixed_command_runner import PrefixedCommandRunner @@ -10,11 +11,16 @@ from pre_commit.prefixed_command_runner import PrefixedCommandRunner @pytest.fixture def popen_mock(): - popen = mock.Mock() + popen = mock.Mock(spec=subprocess.Popen) popen.return_value.communicate.return_value = (mock.Mock(), mock.Mock()) return popen +@pytest.fixture +def makedirs_mock(): + return mock.Mock(spec=os.makedirs) + + @pytest.mark.parametrize(('input', 'kwargs', 'expected_output'), ( ([], {}, []), (['foo'], {}, ['foo']), @@ -40,9 +46,9 @@ def test_init_normalizes_path_endings(input, expected_prefix): assert instance.prefix_dir == expected_prefix -def test_run_substitutes_prefix(popen_mock): - instance = PrefixedCommandRunner('prefix', popen=popen_mock) - ret = instance.run(['{prefix}bar', 'baz']) +def test_run_substitutes_prefix(popen_mock, makedirs_mock): + instance = PrefixedCommandRunner('prefix', popen=popen_mock, makedirs=makedirs_mock) + ret = instance.run(['{prefix}bar', 'baz'], retcode=None) popen_mock.assert_called_once_with( ['prefix/bar', 'baz'], stdin=subprocess.PIPE, @@ -72,6 +78,12 @@ def test_path(prefix, path_end, expected_output): assert ret == expected_output +def test_path_multiple_args(): + instance = PrefixedCommandRunner('foo') + ret = instance.path('bar', 'baz') + assert ret == 'foo/bar/baz' + + @pytest.mark.parametrize(('prefix', 'path_end', 'expected_output'), tuple( (prefix, path_end, expected_output + os.sep) @@ -84,13 +96,41 @@ def test_from_command_runner(prefix, path_end, expected_output): assert second.prefix_dir == expected_output -def test_from_command_runner_preserves_popen(popen_mock): - first = PrefixedCommandRunner('foo', popen=popen_mock) +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']) + second.run(['foo/bar/baz'], retcode=None) popen_mock.assert_called_once_with( ['foo/bar/baz'], stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) + + +def test_create_path_if_not_exists(tmpdir): + with local.cwd(tmpdir.strpath): + instance = PrefixedCommandRunner('foo') + assert not os.path.exists('foo') + instance._create_path_if_not_exists() + assert os.path.exists('foo') + + +def test_exists_does_not_exist(tmpdir): + with local.cwd(tmpdir.strpath): + assert not PrefixedCommandRunner('.').exists('foo') + + +def test_exists_does_exist(tmpdir): + with local.cwd(tmpdir.strpath): + os.mkdir('foo') + assert PrefixedCommandRunner('.').exists('foo') + + +def test_raises_on_error(popen_mock, makedirs_mock): + popen_mock.return_value.returncode = 1 + with pytest.raises(subprocess.CalledProcessError): + instance = PrefixedCommandRunner( + '.', popen=popen_mock, makedirs=makedirs_mock, + ) + instance.run(['foo']) diff --git a/tests/repository_test.py b/tests/repository_test.py index 35550b3b..cf68bf9c 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -6,6 +6,7 @@ import pre_commit.constants as C from pre_commit import git from pre_commit.clientlib.validate_config import CONFIG_JSON_SCHEMA from pre_commit.clientlib.validate_config import validate_config_extra +from pre_commit.prefixed_command_runner import PrefixedCommandRunner from pre_commit.repository import Repository @@ -31,7 +32,7 @@ def test_create_repo_in_env(dummy_repo_config, dummy_git_repo): @pytest.mark.integration def test_install_python_repo_in_env(config_for_python_hooks_repo): repo = Repository(config_for_python_hooks_repo) - repo.install() + repo.install(PrefixedCommandRunner(C.HOOKS_WORKSPACE)) assert os.path.exists( os.path.join( @@ -43,11 +44,13 @@ def test_install_python_repo_in_env(config_for_python_hooks_repo): ) +@pytest.mark.herpderp @pytest.mark.integration def test_run_a_python_hook(config_for_python_hooks_repo): repo = Repository(config_for_python_hooks_repo) - repo.install() - ret = repo.run_hook('foo', ['/dev/null']) + ret = repo.run_hook( + PrefixedCommandRunner(C.HOOKS_WORKSPACE), 'foo', ['/dev/null'], + ) assert ret[0] == 0 assert ret[1] == "['/dev/null']\nHello World\n" @@ -56,18 +59,19 @@ def test_run_a_python_hook(config_for_python_hooks_repo): @pytest.mark.integration def test_run_a_hook_lots_of_files(config_for_python_hooks_repo): repo = Repository(config_for_python_hooks_repo) - repo.install() - ret = repo.run_hook('foo', ['/dev/null'] * 15000) + ret = repo.run_hook( + PrefixedCommandRunner(C.HOOKS_WORKSPACE), 'foo', ['/dev/null'] * 15000, + ) assert ret[0] == 0 -@pytest.mark.xfail @pytest.mark.integration def test_cwd_of_hook(config_for_prints_cwd_repo): repo = Repository(config_for_prints_cwd_repo) - repo.install() - ret = repo.run_hook('prints_cwd', []) + ret = repo.run_hook( + PrefixedCommandRunner(C.HOOKS_WORKSPACE), 'prints_cwd', [], + ) assert ret[0] == 0 assert ret[1] == '{0}\n'.format(repo.repo_url) @@ -80,8 +84,7 @@ def test_cwd_of_hook(config_for_prints_cwd_repo): @pytest.mark.integration def test_run_a_node_hook(config_for_node_hooks_repo): repo = Repository(config_for_node_hooks_repo) - repo.install() - ret = repo.run_hook('foo', []) + ret = repo.run_hook(PrefixedCommandRunner(C.HOOKS_WORKSPACE), 'foo', []) assert ret[0] == 0 assert ret[1] == 'Hello World\n' diff --git a/tests/runner_test.py b/tests/runner_test.py index beac918e..e6138df1 100644 --- a/tests/runner_test.py +++ b/tests/runner_test.py @@ -61,7 +61,7 @@ def test_pre_commit_path(): assert runner.pre_commit_path == expected_path -def test_workspace_runneR(): +def test_cmd_runner(): runner = Runner('foo/bar') - ret = runner.workspace_runner - assert ret.prefix_dir == C.HOOKS_WORKSPACE + '/' + ret = runner.cmd_runner + assert ret.prefix_dir == os.path.join('foo/bar', C.HOOKS_WORKSPACE) + '/' From 18249bec8ad3c72cfd134ad0768a9de9c3488652 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sat, 29 Mar 2014 23:27:26 -0700 Subject: [PATCH 5/8] Simplify installing into virtualenv --- pre_commit/languages/node.py | 4 +--- pre_commit/languages/python.py | 2 +- pre_commit/languages/ruby.py | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/pre_commit/languages/node.py b/pre_commit/languages/node.py index ec9da388..87312e84 100644 --- a/pre_commit/languages/node.py +++ b/pre_commit/languages/node.py @@ -45,9 +45,7 @@ def install_environment(repo_cmd_runner): python_env.run('nodeenv --jobs 4 {{prefix}}{0}'.format(NODE_ENV)) with in_env(repo_cmd_runner) as node_env: - node_env.run( - 'cd {0} && npm install -g'.format(repo_cmd_runner.prefix_dir), - ) + node_env.run('cd {prefix} && npm install -g') def run_hook(repo_cmd_runner, hook, file_args): diff --git a/pre_commit/languages/python.py b/pre_commit/languages/python.py index 91d43b2e..14f2c935 100644 --- a/pre_commit/languages/python.py +++ b/pre_commit/languages/python.py @@ -27,7 +27,7 @@ def install_environment(repo_cmd_runner): # Install a virtualenv repo_cmd_runner.run(['virtualenv', '{{prefix}}{0}'.format(PY_ENV)]) with in_env(repo_cmd_runner) as env: - env.run('cd {0} && pip install .'.format(repo_cmd_runner.prefix_dir)) + env.run('cd {prefix} && pip install .') def run_hook(repo_cmd_runner, hook, file_args): diff --git a/pre_commit/languages/ruby.py b/pre_commit/languages/ruby.py index fcd3ad42..9d3ef26c 100644 --- a/pre_commit/languages/ruby.py +++ b/pre_commit/languages/ruby.py @@ -25,7 +25,7 @@ def install_environment(repo_cmd_runner): repo_cmd_runner.run(['__rvm-env.sh', '{{prefix}}{0}'.format(RVM_ENV)]) with in_env(repo_cmd_runner) as env: - env.run('bundle install', cwd=repo_cmd_runner.prefix_dir) + env.run('cd {prefix} && bundle install') def run_hook(repo_cmd_runner, hook, file_args): From 1238368f88badf1d65ed96dbb65face0e7ea6e33 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sat, 29 Mar 2014 23:33:12 -0700 Subject: [PATCH 6/8] Fix python 2.6 --- pre_commit/languages/node.py | 4 ++-- pre_commit/prefixed_command_runner.py | 25 +++++++++++++++++++++++-- tests/prefixed_command_runner_test.py | 3 ++- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/pre_commit/languages/node.py b/pre_commit/languages/node.py index 87312e84..cf7c9ed6 100644 --- a/pre_commit/languages/node.py +++ b/pre_commit/languages/node.py @@ -1,8 +1,8 @@ import contextlib -import subprocess from pre_commit.languages import helpers from pre_commit.languages import python +from pre_commit.prefixed_command_runner import CalledProcessError NODE_ENV = 'node_env' @@ -38,7 +38,7 @@ def install_environment(repo_cmd_runner): # Try and use the system level node executable first try: python_env.run('nodeenv -n system {{prefix}}{0}'.format(NODE_ENV)) - except subprocess.CalledProcessError: + except CalledProcessError: # TODO: log failure here # cleanup # TODO: local.path(NODE_ENV).delete() diff --git a/pre_commit/prefixed_command_runner.py b/pre_commit/prefixed_command_runner.py index b064b1ef..c97ff07d 100644 --- a/pre_commit/prefixed_command_runner.py +++ b/pre_commit/prefixed_command_runner.py @@ -4,6 +4,27 @@ import os.path import subprocess +class CalledProcessError(RuntimeError): + def __init__(self, returncode, cmd, expected_returncode, output=None): + self.returncode = returncode + self.cmd = cmd + self.expected_returncode = expected_returncode + self.output = output + + def __str__(self): + return ( + 'Command: {0!r}\n' + 'Return code: {1}\n' + 'Expected return code {2}\n', + 'Output: {3!r}\n'.format( + self.cmd, + self.returncode, + self.expected_returncode, + self.output, + ), + ) + + def _replace_cmd(cmd, **kwargs): return [part.format(**kwargs) for part in cmd] @@ -40,8 +61,8 @@ class PrefixedCommandRunner(object): returncode = proc.returncode if retcode is not None and retcode != returncode: - raise subprocess.CalledProcessError( - returncode, replaced_cmd, output=(stdout, stderr), + raise CalledProcessError( + returncode, replaced_cmd, retcode, output=(stdout, stderr), ) return proc.returncode, stdout, stderr diff --git a/tests/prefixed_command_runner_test.py b/tests/prefixed_command_runner_test.py index 22b9e6de..f75bb8a6 100644 --- a/tests/prefixed_command_runner_test.py +++ b/tests/prefixed_command_runner_test.py @@ -6,6 +6,7 @@ import subprocess from plumbum import local from pre_commit.prefixed_command_runner import _replace_cmd +from pre_commit.prefixed_command_runner import CalledProcessError from pre_commit.prefixed_command_runner import PrefixedCommandRunner @@ -129,7 +130,7 @@ def test_exists_does_exist(tmpdir): def test_raises_on_error(popen_mock, makedirs_mock): popen_mock.return_value.returncode = 1 - with pytest.raises(subprocess.CalledProcessError): + with pytest.raises(CalledProcessError): instance = PrefixedCommandRunner( '.', popen=popen_mock, makedirs=makedirs_mock, ) From 75fc3a34810c076c9cf0681645764de2e43fe376 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sat, 29 Mar 2014 23:34:14 -0700 Subject: [PATCH 7/8] No more abspath --- pre_commit/run.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pre_commit/run.py b/pre_commit/run.py index 1fd9d5c0..0eda1584 100644 --- a/pre_commit/run.py +++ b/pre_commit/run.py @@ -1,6 +1,5 @@ import argparse -import os.path import subprocess import sys @@ -36,7 +35,7 @@ def _run_single_hook(runner, repository, hook_id, all_files=False): retcode, stdout, stderr = repository.run_hook( runner.cmd_runner, hook_id, - map(os.path.abspath, get_filenames(hook['files'])), + get_filenames(hook['files']), ) if retcode != repository.hooks[hook_id].get('expected_return_value', 0): From 53317adb1e9aa5f6a89a3ed352e09a9015ac891e Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sat, 29 Mar 2014 23:47:04 -0700 Subject: [PATCH 8/8] Forgot to remove a herpderp --- tests/repository_test.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/repository_test.py b/tests/repository_test.py index cf68bf9c..d7002355 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -44,7 +44,6 @@ def test_install_python_repo_in_env(config_for_python_hooks_repo): ) -@pytest.mark.herpderp @pytest.mark.integration def test_run_a_python_hook(config_for_python_hooks_repo): repo = Repository(config_for_python_hooks_repo)