rebaseline_server: make --reload work in git checkout

gm/rebaseline_server/server.py

Index: gm/rebaseline_server/server.py
===================================================================
--- gm/rebaseline_server/server.py	(revision 12384)
+++ gm/rebaseline_server/server.py	(working copy)
@@ -19,8 +19,10 @@
 import re
 import shutil
 import socket
+import subprocess
 import sys
 import thread
+import threading
 import time
 import urlparse
 
@@ -42,8 +44,6 @@
 
 ACTUALS_SVN_REPO = 'http://skia-autogen.googlecode.com/svn/gm-actual'
 PATHSPLIT_RE = re.compile('/([^/]+)/(.+)')
-TRUNK_DIRECTORY = os.path.dirname(os.path.dirname(os.path.dirname(
-    os.path.realpath(__file__))))
 EXPECTATIONS_DIR = os.path.join(TRUNK_DIRECTORY, 'expectations', 'gm')
 GENERATED_IMAGES_ROOT = os.path.join(PARENT_DIRECTORY, 'static',
                                      'generated-images')
@@ -67,6 +67,27 @@
 
 _SERVER = None   # This gets filled in by main()
 
+def _run_command(args, directory):
+  """Runs a command and returns stdout as a single string.
+
+  Args:
+    args: the command to run, as a list of arguments
+    directory: directory within which to run the command
+
+  Returns: stdout, as a string
+
+  Raises an Exception if the command failed (exited with nonzero return code).
+  """
+  logging.debug('_run_command: %s in directory %s' % (args, directory))
+  proc = subprocess.Popen(args, cwd=directory,
+                          stdout=subprocess.PIPE,
+                          stderr=subprocess.PIPE)
+  (stdout, stderr) = proc.communicate()
+  if proc.returncode is not 0:
+    raise Exception('command "%s" failed in dir "%s": %s' %
+                    (args, directory, stderr))
+  return stdout
+

[rmistry, 2013/11/26 12:25:17]

Nit: There should be two newlines between top level functions and classes.

[epoger, 2013/11/26 17:04:35]

Done.

 def _get_routable_ip_address():
   """Returns routable IP address of this host (the IP address of its network
      interface that would be used for most traffic, not its localhost
@@ -120,21 +141,10 @@
     self._actuals_repo = _create_svn_checkout(
         dir_path=actuals_dir, repo_url=ACTUALS_SVN_REPO)
 
-    # We only update the expectations dir if the server was run with a
-    # nonzero --reload argument; otherwise, we expect the user to maintain
-    # her own expectations as she sees fit.
-    #
-    # TODO(epoger): Use git instead of svn to update the expectations dir, since
-    # the Skia repo is moving to git.
-    # When we make that change, we will have to update the entire workspace,
-    # not just the expectations dir, because git only operates on the repo
-    # as a whole.
-    # And since Skia uses depot_tools to manage its dependencies, we will have
-    # to run "gclient sync" rather than a raw "git pull".
-    if reload_seconds:
-      self._expectations_repo = svn.Svn(EXPECTATIONS_DIR)
-    else:
-      self._expectations_repo = None
+    # Reentrant lock that must be held whenever updating EITHER of:
+    # 1. self.results
+    # 2. the expected or actual results on local disk
+    self.results_rlock = threading.RLock()
 
   def is_exported(self):
     """ Returns true iff HTTP clients on other hosts are allowed to access
@@ -153,24 +163,41 @@
   def update_results(self):
     """ Create or update self.results, based on the expectations in
     EXPECTATIONS_DIR and the latest actuals from skia-autogen.
+
+    We hold self.results_rlock while we do this, to guarantee that no other
+    thread attempts to update either self.results or the underlying files at
+    the same time.
     """
-    logging.info('Updating actual GM results in %s from SVN repo %s ...' % (
-        self._actuals_dir, ACTUALS_SVN_REPO))
-    self._actuals_repo.Update('.')
+    with self.results_rlock:
+      logging.info('Updating actual GM results in %s from SVN repo %s ...' % (
+          self._actuals_dir, ACTUALS_SVN_REPO))
+      self._actuals_repo.Update('.')
 
-    if self._expectations_repo:
+      # We only update the expectations dir if the server was run with a
+      # nonzero --reload argument; otherwise, we expect the user to maintain
+      # her own expectations as she sees fit.
+      #
+      # Because the Skia repo is moving from SVN to git, and git does not
+      # support updating a single directory tree, we have to update the entire
+      # repo checkout.
+      #
+      # Because Skia uses depot_tools, we have to update using "gclient sync"
+      # instead of raw git (or SVN) update.  Happily, this will work whether
+      # the checkout was created using git or SVN.
+      if self._reload_seconds:
+        logging.info(
+            'Updating expected GM results in %s by syncing Skia repo ...' %
+            EXPECTATIONS_DIR)
+        _run_command(['gclient', 'sync'], TRUNK_DIRECTORY)
+
       logging.info(
-          'Updating expected GM results in %s ...' % EXPECTATIONS_DIR)
-      self._expectations_repo.Update('.')
-
-    logging.info(
           ('Parsing results from actuals in %s and expectations in %s, '
-          + 'and generating pixel diffs (may take a while) ...') % (
-          self._actuals_dir, EXPECTATIONS_DIR))
-    self.results = results.Results(
-        actuals_root=self._actuals_dir,
-        expected_root=EXPECTATIONS_DIR,
-        generated_images_root=GENERATED_IMAGES_ROOT)
+           + 'and generating pixel diffs (may take a while) ...') % (
+               self._actuals_dir, EXPECTATIONS_DIR))
+      self.results = results.Results(

[rmistry, 2013/11/26 12:25:17]

I recommend making self._results that is None in the constructor with a comment
saying it will be set when update_results is called successfully.

Then making a
@property
def results(self):
  return self._results

[epoger, 2013/11/26 17:04:35]

Done.

+          actuals_root=self._actuals_dir,
+          expected_root=EXPECTATIONS_DIR,
+          generated_images_root=GENERATED_IMAGES_ROOT)
 
   def _result_reloader(self):
     """ If --reload argument was specified, reload results at the appropriate
@@ -357,23 +384,24 @@
     logging.debug('do_POST_edits: received new GM expectations data [%s]' %
                   data)
 
-    # Since we must make multiple calls to the Results object, grab a
-    # reference to it in case it is updated to point at a new Results
-    # object within another thread.
-    results_obj = _SERVER.results
-    oldResultsType = data['oldResultsType']
-    oldResults = results_obj.get_results_of_type(oldResultsType)
-    oldResultsHash = str(hash(repr(oldResults['testData'])))
-    if oldResultsHash != data['oldResultsHash']:
-      raise Exception('results of type "%s" changed while the client was '
-                      'making modifications. The client should reload the '
-                      'results and submit the modifications again.' %
-                      oldResultsType)
-    results_obj.edit_expectations(data['modifications'])
+    # Update the results on disk with the information we received from the
+    # client.
+    # We must hold _SERVER.results_rlock while we do this, to guarantee that
+    # no other thread updates expectations (from the Skia repo) while we are
+    # updating them (using the info we received from the client).
+    with _SERVER.results_rlock:
+      oldResultsType = data['oldResultsType']
+      oldResults = _SERVER.results.get_results_of_type(oldResultsType)
+      oldResultsHash = str(hash(repr(oldResults['testData'])))
+      if oldResultsHash != data['oldResultsHash']:
+        raise Exception('results of type "%s" changed while the client was '
+                        'making modifications. The client should reload the '
+                        'results and submit the modifications again.' %
+                        oldResultsType)
+      _SERVER.results.edit_expectations(data['modifications'])
+      # Read the updated results back from disk.
+      _SERVER.update_results()
 
-    # Now that the edits have been committed, update results to reflect them.
-    _SERVER.update_results()
-
   def redirect_to(self, url):
     """ Redirect the HTTP client to a different url.
 
@@ -445,7 +473,9 @@
   parser.add_argument('--reload', type=int,
                       help=('How often (a period in seconds) to update the '
                             'results.  If specified, both expected and actual '
-                            'results will be updated.  '
+                            'results will be updated.  In fact, your entire '

[rmistry, 2013/11/26 12:25:17]

Could reword this by saying something like:
If specified, both expected and actual results will be updated by running
gclient sync on the Skia checkout. By default, ...

[epoger, 2013/11/26 17:04:35]

Done.

+                            'Skia checkout will be updated, by running '

[epoger, 2013/11/25 19:48:22]

Patchset 2: just added a bit more to the help message.  Still ready for review.

+                            '"gclient sync"!  '
                             'By default, we do not reload at all, and you '
                             'must restart the server to pick up new data.'),
                       default=0)