Each set of additional dependencies gets its own env

asottile · asottile · commit b827694520be · 2018-02-24T15:28:43.000-08:00
diff --git 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
@@ -7,7 +7,6 @@
 import pipes
 import shutil
 import sys
-from collections import defaultdict
 
 import pkg_resources
 from cached_property import cached_property
@@ -149,22 +148,11 @@ def create(cls, config, store):
         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 @@ def hooks(self):
             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 @@ def hooks(self):
             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 @@ def hooks(self):
             (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
@@ -72,9 +72,9 @@ def _write_sqlite_db(self):
         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 @@ def require_created(self):
             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 @@ def _get_result():
                 )
         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 @@ def clone_strategy(directory):
                     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 @@ def _git_cmd(*args):
             _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
@@ -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
@@ -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):
+def test_additional_dependencies_roll_forward(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
 
+    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]
 
-@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
+    # 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 @@ class MyKeyboardInterrupt(KeyboardInterrupt):
                 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__')
     ]
