From b827694520be0f39bfc0599f3680b6c08b4516cf Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Sat, 24 Feb 2018 14:29:32 -0800 Subject: [PATCH] Each set of additional dependencies gets its own env --- pre_commit/commands/autoupdate.py | 6 +-- pre_commit/repository.py | 64 +++++++------------------ pre_commit/store.py | 19 ++++---- tests/conftest.py | 6 --- tests/repository_test.py | 79 +++++++++++++------------------ 5 files changed, 62 insertions(+), 112 deletions(-) diff --git a/pre_commit/commands/autoupdate.py b/pre_commit/commands/autoupdate.py index ca83a588..666cd117 100644 --- a/pre_commit/commands/autoupdate.py +++ b/pre_commit/commands/autoupdate.py @@ -33,9 +33,9 @@ def _update_repo(repo_config, runner, tags_only): Args: repo_config - A config for a repository """ - repo = Repository.create(repo_config, runner.store) + repo_path = runner.store.clone(repo_config['repo'], repo_config['sha']) - with cwd(repo._repo_path): + with cwd(repo_path): cmd_output('git', 'fetch') tag_cmd = ('git', 'describe', 'origin/master', '--tags') if tags_only: @@ -57,7 +57,7 @@ def _update_repo(repo_config, runner, tags_only): new_repo = Repository.create(new_config, runner.store) # See if any of our hooks were deleted with the new commits - hooks = {hook['id'] for hook in repo.repo_config['hooks']} + hooks = {hook['id'] for hook in repo_config['hooks']} hooks_missing = hooks - (hooks & set(new_repo.manifest_hooks)) if hooks_missing: raise RepositoryCannotBeUpdatedError( diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 3ed160af..624ccd00 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -7,7 +7,6 @@ import os import pipes import shutil import sys -from collections import defaultdict import pkg_resources from cached_property import cached_property @@ -149,22 +148,11 @@ class Repository(object): else: return cls(config, store) - @cached_property - def _repo_path(self): - return self.store.clone( - self.repo_config['repo'], self.repo_config['sha'], - ) - - @cached_property - def _prefix(self): - return Prefix(self._repo_path) - - def _prefix_from_deps(self, language_name, deps): - return self._prefix - @cached_property def manifest_hooks(self): - manifest_path = os.path.join(self._repo_path, C.MANIFEST_FILE) + repo, sha = self.repo_config['repo'], self.repo_config['sha'] + repo_path = self.store.clone(repo, sha) + manifest_path = os.path.join(repo_path, C.MANIFEST_FILE) return {hook['id']: hook for hook in load_manifest(manifest_path)} @cached_property @@ -185,21 +173,25 @@ class Repository(object): for hook in self.repo_config['hooks'] ) - @cached_property + def _prefix_from_deps(self, language_name, deps): + repo, sha = self.repo_config['repo'], self.repo_config['sha'] + return Prefix(self.store.clone(repo, sha, deps)) + def _venvs(self): - deps_dict = defaultdict(_UniqueList) - for _, hook in self.hooks: - deps_dict[(hook['language'], hook['language_version'])].update( - hook['additional_dependencies'], - ) ret = [] - for (language, version), deps in deps_dict.items(): - ret.append((self._prefix, language, version, deps)) + for _, hook in self.hooks: + language = hook['language'] + version = hook['language_version'] + deps = hook['additional_dependencies'] + ret.append(( + self._prefix_from_deps(language, deps), + language, version, deps, + )) return tuple(ret) def require_installed(self): if not self.__installed: - _install_all(self._venvs, self.repo_config['repo'], self.store) + _install_all(self._venvs(), self.repo_config['repo'], self.store) self.__installed = True def run_hook(self, hook, file_args): @@ -237,19 +229,6 @@ class LocalRepository(Repository): for hook in self.repo_config['hooks'] ) - @cached_property - def _venvs(self): - ret = [] - for _, hook in self.hooks: - language = hook['language'] - version = hook['language_version'] - deps = hook['additional_dependencies'] - ret.append(( - self._prefix_from_deps(language, deps), - language, version, deps, - )) - return tuple(ret) - class MetaRepository(LocalRepository): @cached_property @@ -303,14 +282,3 @@ class MetaRepository(LocalRepository): (hook['id'], _hook(self.manifest_hooks[hook['id']], hook)) for hook in self.repo_config['hooks'] ) - - -class _UniqueList(list): - def __init__(self): - self._set = set() - - def update(self, obj): - for item in obj: - if item not in self._set: - self._set.add(item) - self.append(item) diff --git a/pre_commit/store.py b/pre_commit/store.py index 13119840..7e49c8fd 100644 --- a/pre_commit/store.py +++ b/pre_commit/store.py @@ -72,9 +72,9 @@ class Store(object): with contextlib.closing(sqlite3.connect(tmpfile)) as db: db.executescript( 'CREATE TABLE repos (' - ' repo CHAR(255) NOT NULL,' - ' ref CHAR(255) NOT NULL,' - ' path CHAR(255) NOT NULL,' + ' repo TEXT NOT NULL,' + ' ref TEXT NOT NULL,' + ' path TEXT NOT NULL,' ' PRIMARY KEY (repo, ref)' ');', ) @@ -101,15 +101,17 @@ class Store(object): self._create() self.__created = True - def _new_repo(self, repo, ref, make_strategy): + def _new_repo(self, repo, ref, deps, make_strategy): self.require_created() + if deps: + repo = '{}:{}'.format(repo, ','.join(sorted(deps))) def _get_result(): # Check if we already exist with sqlite3.connect(self.db_path) as db: result = db.execute( 'SELECT path FROM repos WHERE repo = ? AND ref = ?', - [repo, ref], + (repo, ref), ).fetchone() if result: return result[0] @@ -137,7 +139,7 @@ class Store(object): ) return directory - def clone(self, repo, ref): + def clone(self, repo, ref, deps=()): """Clone the given url and checkout the specific ref.""" def clone_strategy(directory): cmd_output( @@ -151,7 +153,7 @@ class Store(object): env=no_git_env(), ) - return self._new_repo(repo, ref, clone_strategy) + return self._new_repo(repo, ref, deps, clone_strategy) def make_local(self, deps): def make_local_strategy(directory): @@ -172,8 +174,7 @@ class Store(object): _git_cmd('commit', '--no-edit', '--no-gpg-sign', '-n', '-minit') return self._new_repo( - 'local:{}'.format(','.join(sorted(deps))), C.LOCAL_REPO_VERSION, - make_local_strategy, + 'local', C.LOCAL_REPO_VERSION, deps, make_local_strategy, ) @cached_property diff --git a/tests/conftest.py b/tests/conftest.py index fd3784df..246820e9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -165,12 +165,6 @@ def log_info_mock(): yield mck -@pytest.fixture -def log_warning_mock(): - with mock.patch.object(logging.getLogger('pre_commit'), 'warning') as mck: - yield mck - - class FakeStream(object): def __init__(self): self.data = io.BytesIO() diff --git a/tests/repository_test.py b/tests/repository_test.py index 0123ce4c..dea387f2 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -433,7 +433,7 @@ def test_venvs(tempdir_factory, store): path = make_repo(tempdir_factory, 'python_hooks_repo') config = make_config_from_repo(path) repo = Repository.create(config, store) - venv, = repo._venvs + venv, = repo._venvs() assert venv == (mock.ANY, 'python', python.get_default_version(), []) @@ -443,50 +443,33 @@ def test_additional_dependencies(tempdir_factory, store): config = make_config_from_repo(path) config['hooks'][0]['additional_dependencies'] = ['pep8'] repo = Repository.create(config, store) - venv, = repo._venvs + venv, = repo._venvs() assert venv == (mock.ANY, 'python', python.get_default_version(), ['pep8']) -@pytest.mark.integration -def test_additional_dependencies_duplicated( - tempdir_factory, store, log_warning_mock, -): - path = make_repo(tempdir_factory, 'ruby_hooks_repo') - config = make_config_from_repo(path) - deps = ['thread_safe', 'tins', 'thread_safe'] - config['hooks'][0]['additional_dependencies'] = deps - repo = Repository.create(config, store) - venv, = repo._venvs - assert venv == (mock.ANY, 'ruby', 'default', ['thread_safe', 'tins']) - - -@pytest.mark.integration -def test_additional_python_dependencies_installed(tempdir_factory, store): - path = make_repo(tempdir_factory, 'python_hooks_repo') - config = make_config_from_repo(path) - config['hooks'][0]['additional_dependencies'] = ['mccabe'] - repo = Repository.create(config, store) - repo.require_installed() - with python.in_env(repo._prefix, 'default'): - output = cmd_output('pip', 'freeze', '-l')[1] - assert 'mccabe' in output - - @pytest.mark.integration def test_additional_dependencies_roll_forward(tempdir_factory, store): path = make_repo(tempdir_factory, 'python_hooks_repo') - config = make_config_from_repo(path) - # Run the repo once without additional_dependencies - repo = Repository.create(config, store) - repo.require_installed() - # Now run it with additional_dependencies - config['hooks'][0]['additional_dependencies'] = ['mccabe'] - repo = Repository.create(config, store) - repo.require_installed() - # We should see our additional dependency installed - with python.in_env(repo._prefix, 'default'): - output = cmd_output('pip', 'freeze', '-l')[1] - assert 'mccabe' in output + + config1 = make_config_from_repo(path) + repo1 = Repository.create(config1, store) + repo1.require_installed() + (prefix1, _, version1, _), = repo1._venvs() + with python.in_env(prefix1, version1): + assert 'mccabe' not in cmd_output('pip', 'freeze', '-l')[1] + + # Make another repo with additional dependencies + config2 = make_config_from_repo(path) + config2['hooks'][0]['additional_dependencies'] = ['mccabe'] + repo2 = Repository.create(config2, store) + repo2.require_installed() + (prefix2, _, version2, _), = repo2._venvs() + with python.in_env(prefix2, version2): + assert 'mccabe' in cmd_output('pip', 'freeze', '-l')[1] + + # should not have affected original + with python.in_env(prefix1, version1): + assert 'mccabe' not in cmd_output('pip', 'freeze', '-l')[1] @xfailif_windows_no_ruby @@ -499,7 +482,8 @@ def test_additional_ruby_dependencies_installed( config['hooks'][0]['additional_dependencies'] = ['thread_safe', 'tins'] repo = Repository.create(config, store) repo.require_installed() - with ruby.in_env(repo._prefix, 'default'): + (prefix, _, version, _), = repo._venvs() + with ruby.in_env(prefix, version): output = cmd_output('gem', 'list', '--local')[1] assert 'thread_safe' in output assert 'tins' in output @@ -516,7 +500,8 @@ def test_additional_node_dependencies_installed( config['hooks'][0]['additional_dependencies'] = ['lodash'] repo = Repository.create(config, store) repo.require_installed() - with node.in_env(repo._prefix, 'default'): + (prefix, _, version, _), = repo._venvs() + with node.in_env(prefix, version): output = cmd_output('npm', 'ls', '-g')[1] assert 'lodash' in output @@ -532,7 +517,8 @@ def test_additional_golang_dependencies_installed( config['hooks'][0]['additional_dependencies'] = deps repo = Repository.create(config, store) repo.require_installed() - binaries = os.listdir(repo._prefix.path( + (prefix, _, _, _), = repo._venvs() + binaries = os.listdir(prefix.path( helpers.environment_dir(golang.ENVIRONMENT_DIR, 'default'), 'bin', )) # normalize for windows @@ -598,8 +584,9 @@ def test_control_c_control_c_on_install(tempdir_factory, store): repo.run_hook(hook, []) # Should have made an environment, however this environment is broken! - envdir = 'py_env-{}'.format(python.get_default_version()) - assert repo._prefix.exists(envdir) + (prefix, _, version, _), = repo._venvs() + envdir = 'py_env-{}'.format(version) + assert prefix.exists(envdir) # However, it should be perfectly runnable (reinstall after botched # install) @@ -616,8 +603,8 @@ def test_invalidated_virtualenv(tempdir_factory, store): # Simulate breaking of the virtualenv repo.require_installed() - version = python.get_default_version() - libdir = repo._prefix.path('py_env-{}'.format(version), 'lib', version) + (prefix, _, version, _), = repo._venvs() + libdir = prefix.path('py_env-{}'.format(version), 'lib', version) paths = [ os.path.join(libdir, p) for p in ('site.py', 'site.pyc', '__pycache__') ]