Make additional_dependencies rollforward safe

asottile · asottile · commit b85a674026ba · 2015-12-10T12:21:03.000-08:00
diff --git a/pre_commit/repository.py b/pre_commit/repository.py
@@ -1,12 +1,16 @@
 from __future__ import unicode_literals
 
+import io
+import json
 import logging
+import os
 import shutil
 from collections import defaultdict
 
 import pkg_resources
 from cached_property import cached_property
 
+from pre_commit import five
 from pre_commit import git
 from pre_commit.clientlib.validate_config import is_local_hooks
 from pre_commit.clientlib.validate_manifest import MANIFEST_JSON_SCHEMA
@@ -23,6 +27,9 @@
     pkg_resources.get_distribution('pre-commit').version
 )
 
+# Bump when installation changes in a backwards / forwards incompatible way
+INSTALLED_STATE_VERSION = '1'
+
 
 class Repository(object):
     def __init__(self, repo_config, repo_path_getter):
@@ -110,14 +117,45 @@ def require_installed(self):
 
     def install(self):
         """Install the hook repository."""
+        def state(language_name, language_version):
+            return {
+                'additional_dependencies': sorted(
+                    self.additional_dependencies[
+                        language_name
+                    ][language_version],
+                )
+            }
+
+        def state_filename(venv, suffix=''):
+            return self.cmd_runner.path(
+                venv, '.install_state_v' + INSTALLED_STATE_VERSION + suffix,
+            )
+
+        def read_state(venv):
+            if not os.path.exists(state_filename(venv)):
+                return None
+            else:
+                return json.loads(io.open(state_filename(venv)).read())
+
+        def write_state(venv, language_name, language_version):
+            with io.open(
+                    state_filename(venv, suffix='staging'), 'w',
+            ) as state_file:
+                state_file.write(five.to_text(json.dumps(
+                    state(language_name, language_version),
+                )))
+            # Move the file into place atomically to indicate we've installed
+            os.rename(
+                state_filename(venv, suffix='staging'),
+                state_filename(venv),
+            )
+
         def language_is_installed(language_name, language_version):
             language = languages[language_name]
-            directory = environment_dir(
-                language.ENVIRONMENT_DIR, language_version,
-            )
+            venv = environment_dir(language.ENVIRONMENT_DIR, language_version)
             return (
-                directory is None or
-                self.cmd_runner.exists(directory, '.installed')
+                venv is None or
+                read_state(venv) == state(language_name, language_version)
             )
 
         if not all(
@@ -131,24 +169,23 @@ def language_is_installed(language_name, language_version):
             logger.info('This may take a few minutes...')
 
         for language_name, language_version in self.languages:
-            language = languages[language_name]
             if language_is_installed(language_name, language_version):
                 continue
 
-            directory = environment_dir(
-                language.ENVIRONMENT_DIR, language_version,
-            )
+            language = languages[language_name]
+            venv = environment_dir(language.ENVIRONMENT_DIR, language_version)
+
             # There's potentially incomplete cleanup from previous runs
             # Clean it up!
-            if self.cmd_runner.exists(directory):
-                shutil.rmtree(self.cmd_runner.path(directory))
+            if self.cmd_runner.exists(venv):
+                shutil.rmtree(self.cmd_runner.path(venv))
 
             language.install_environment(
                 self.cmd_runner, language_version,
                 self.additional_dependencies[language_name][language_version],
             )
-            # Touch the .installed file (atomic) to indicate we've installed
-            open(self.cmd_runner.path(directory, '.installed'), 'w').close()
+            # Write our state to indicate we're installed
+            write_state(venv, language_name, language_version)
 
     def run_hook(self, hook, file_args):
         """Run a hook.
diff --git a/setup.py b/setup.py
@@ -46,7 +46,6 @@
         'nodeenv>=0.11.1',
         'ordereddict',
         'pyyaml',
-        'simplejson',
         'virtualenv',
     ],
     entry_points={
diff --git a/tests/repository_test.py b/tests/repository_test.py
@@ -360,6 +360,23 @@ def test_additional_python_dependencies_installed(tempdir_factory, store):
         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.run_hook(repo.hooks[0][1], [])
+    # Now run it with additional_dependencies
+    config['hooks'][0]['additional_dependencies'] = ['mccabe']
+    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]
+        assert 'mccabe' in output
+
+
 @xfailif_windows_no_ruby
 @pytest.mark.integration
 def test_additional_ruby_dependencies_installed(
