Adding ability to bisect recipe to bisect into dependency repos.

scripts/slave/recipe_modules/auto_bisect/bisector.py

 # Copyright 2015 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import re
+
 from . import bisect_results
+from . import depot_config
+
+DEPS_SHA_PATCH = """diff --git DEPS.sha DEPS.sha

[qyearsley, 2015/02/22 20:37:54]

Would it make sense to mark this as private with a underscore prefix?

[RobertoCN, 2015/02/24 20:01:13]

Done.

+new file mode 100644
+--- /dev/null
++++ DEPS.sha
+@@ -0,0 +1 @@
++%(deps_sha)s
+"""
+
 
 class Bisector(object):
   """This class abstracts an ongoing bisect (or n-sect) job."""
 
   def __init__(self, api, bisect_config, revision_class):
     """Initializes the state of a new bisect job from a dictionary.
 
     Note that the initial good_rev and bad_rev MUST resolve to a commit position
     in the chromium repo.
     """
     super(Bisector, self).__init__()
     self._api = api
     self.bisect_config = bisect_config
     self.revision_class = revision_class
 
-    # Test-only properties. 
+    # Test-only properties.
     # TODO: Replace these with proper mod_test_data
     self.dummy_regression_confidence = bisect_config.get(
         'dummy_regression_confidence', None)
     self.dummy_builds = bisect_config.get('dummy_builds', False)
 
     # Loading configuration items
     self.test_type = bisect_config.get('test_type', 'perf')
     self.improvement_direction = int(bisect_config.get(
         'improvement_direction', 0)) or None
 
@@ skipping 22 matching lines @@
   @property
   def api(self):
     return self._api
 
   @staticmethod
   def _commit_pos_range(a, b):
     """Given 2 commit positions, returns a list with the ones between."""
     a, b = sorted(map(int, [a, b]))
     return xrange(a + 1, b)
 
+  def make_deps_sha_file(self, deps_sha):
+    """Make a diff patch that creates DEPS.sha.
+
+    Args:
+      deps_sha (str): Hash of the diff that patches DEPS.
+
+    Returns: A string containing a git diff.

[qyearsley, 2015/02/22 20:37:54]

Usually the description of the return value goes on the next line after
"Returns:". Same below.

[RobertoCN, 2015/02/24 20:01:13]

Done.

+    """
+    return '\n' + DEPS_SHA_PATCH % {'deps_sha': deps_sha}

[qyearsley, 2015/02/22 20:37:54]

Maybe the newline should be part of DEPS_PATCH_SHA?

[RobertoCN, 2015/02/24 20:01:13]

Done.

+
+  def make_deps_patch(self, base_revision, base_file_contents, deps_var,
+                      new_commit_hash):
+    """Make a diff patch that updates a specifc dependency revision.

[qyearsley, 2015/02/22 20:37:54]

specifc -> specific

[RobertoCN, 2015/02/24 20:01:13]

Done.

+
+    Args:
+      base_revision (RevisionState): The revision whose DEPS is to be patched.

[qyearsley, 2015/02/22 20:37:53]

Possible alternative phrasing:
The revision for which the DEPS file is to be patched.

[RobertoCN, 2015/02/24 20:01:13]

Done.

+      base_file_contents (str): The content of the original DEPS file.
+      deps_var (str): The variable name identifying the dependency to modify.
+      new_commit_hash (str): The revision to put in place of the old one.
+
+    Returns: A pair containing the git diff patch that updates DEPS, and the
+      full text of such modified DEPS, both as strings.

[qyearsley, 2015/02/22 20:37:53]

full text of the modified DEPS file, ...

[RobertoCN, 2015/02/24 20:01:13]

Done.

+    """
+    original_contents = str(base_file_contents)
+    patched_contents = str(original_contents)
+
+    # Modify DEPS
+    rxp = re.compile(
+        r'(?<=["\']%s["\']: ["\'])([a-fA-F0-9]{40})(?=["\'])' % deps_var,
+        re.MULTILINE)

[qyearsley, 2015/02/22 20:37:54]

This variable name could be changed -- maybe something like deps_item_pattern or
deps_item_regexp?

[RobertoCN, 2015/02/24 20:01:13]

Done.

+    if not re.search(rxp, original_contents):
+      raise ValueError('DEPS file does not contain entry for ' + deps_var)
+    re.sub(rxp, new_commit_hash, patched_contents)
+
+    # Intern modified DEPS
+    cmd = 'hash-object -t blob -w --stdin'.split(' ')

[qyearsley, 2015/02/22 20:37:53]

1. split() with no args would have the same effect here. This way is also OK.
2. It would be good to have an explanation of what it means to "intern" a patch.

[RobertoCN, 2015/02/24 20:01:13]

Leaving the .split(' ') as it is explicit.

Separated the intern file logic into its own private function with its own
docstring.

+    stdin = self.api.m.raw_io.input(patched_contents)
+    stdout = self.api.m.raw_io.output()
+    step_result = self.api.m.git(*cmd, cwd=self.api.m.path['checkout'],
+                                 stdin=stdin, stdout=stdout)
+    interned_deps_hash = step_result.stdout.splitlines()[0]
+
+    # Gen diff patch
+    cmd = 'diff %s:DEPS %s --src-prefix=IAMSRC: --dst-prefix=IAMDST:'
+    cmd %= (base_revision, interned_deps_hash)
+    cmd = cmd.split(' ')
+    stdout = self.api.m.raw_io.output()
+    step_result = self.api.m.git(*cmd, cwd=self.api.m.path['checkout'],
+                                 stdout=stdout)
+    patch_text = step_result.stdout
+    src_string = 'IAMSRC:%s:DEPS' % base_revision
+    dst_string = 'IAMDST:' + interned_deps_hash
+    patch_text = patch_text.replace(src_string, 'a/DEPS')
+    patch_text = patch_text.replace(dst_string, 'b/DEPS')
+
+    return patch_text, patched_contents

[qyearsley, 2015/02/22 20:37:54]

I think that parts of this function could potentially be more understandable if
they were broken into individual functions with more comments. For example, you
could have functions like:
_modify_deps (input: deps_var, new_commit_hash, original_contents; output:
patched_contents)
_intern_modified_deps (input: patched_contents, output: interned_deps_hash)
_gen_diff_patch (input: base_revision, interned_deps_hash, patch_text).

[RobertoCN, 2015/02/24 20:01:13]

Done.

+
+  def _get_rev_range_for_depot(self, depot, min_rev, max_rev):
+    results = []
+    depot_path = self.api.m.path['checkout'].join(depot['src'])
+    step_name = 'Expanding revision range for ' + depot['deps_var']
+    step_result = self.api.m.git('log', '--format==%H', min_rev, max_rev,
+                                 stdout=self.api.m.raw_io.output(),
+                                 cwd=depot_path, name=step_name)
+    # We skip the first revision in the list as it is max_rev
+    new_revisions = step_result.stdout.split_lines()[1:]
+    for revision in new_revisions:
+      results.append(self.revision_class(None, self,
+                                         base_revision=min_rev,
+                                         deps_revision=revision,
+                                         depot=depot))
+    results.reverse()
+    return results
+
   def _expand_revision_range(self, revision_to_expand=None):
     """This method populates the revisions attribute.

[qyearsley, 2015/02/22 20:37:53]

You could drop the words "This method" here.

[RobertoCN, 2015/02/24 20:01:13]

Done.

 
     After running method self.revisions should contain all the revisions

[qyearsley, 2015/02/22 20:37:53]

-> After running this method, self.revisions should contain...

[RobertoCN, 2015/02/24 20:01:13]

Done.

     between the good and bad revisions. If given a `revision_to_expand`, it'll
     insert the revisions from the external repos in the appropriate place.
 
     Args:
       revision_to_expand: A revision where there is a deps change.

[qyearsley, 2015/02/22 20:37:54]

Would it make sense to possibly have two separate functions, one that takes a
revision_to_expand and inserts deps revisions, and one that just initially
populates the revisions attribute?

[RobertoCN, 2015/02/24 20:01:13]

Done.

+
+    Returns:
+      A boolean indicating whether any revisions were inserted.
     """
     if revision_to_expand is not None:
-      # TODO: Implement this path (insert revisions when deps change)
-      raise NotImplementedError()
+      try:
+        min_revision = revision_to_expand.previous_revision
+        max_revision = revision_to_expand
+        min_revision.read_deps()
+        max_revision.read_deps()

[qyearsley, 2015/02/22 20:37:54]

What does read_deps() do here? It sets the deps attribute right?

[RobertoCN, 2015/02/24 20:01:13]

Yes, added in-line comment.

+        for depot in depot_config.DEPOT_DEPS_NAME:
+          deps_var = depot['deps_var']
+          if (deps_var in min_revision.deps and deps_var in max_revision.deps):

[qyearsley, 2015/02/22 20:37:53]

Parentheses unnecessary.

[RobertoCN, 2015/02/24 20:01:13]

Done.

+            dep_revision_min = min_revision.deps[deps_var]
+            dep_revision_max = max_revision.deps[deps_var]
+            if (dep_revision_min and dep_revision_max and
+                dep_revision_min != dep_revision_max):
+              rev_list = self._get_rev_range_for_depot(depot, dep_revision_min,
+                                                       dep_revision_max)
+              new_revisions = self.revisions[:max_revision.list_index]
+              new_revisions += rev_list
+              new_revisions += self.revisions[max_revision.list_index:]
+              self.revisions = new_revisions
+              self._update_revision_list_indexes()
+              return True
+      except RuntimeError:
+        warning_text = ("Could not expand dependency revisions for " +

[qyearsley, 2015/02/22 20:37:53]

Single quotes.

[RobertoCN, 2015/02/24 20:01:13]

Done.

+                        revision_to_expand.revision_string)
+        if warning_text not in self.bisector.warnings:
+          self.bisector.warnings.append(warning_text)
+      return False
+
     rev_list = self._commit_pos_range(
         self.good_rev.commit_pos, self.bad_rev.commit_pos)
     intermediate_revs = [self.revision_class(str(x), self) for x in rev_list]
     self.revisions = [self.good_rev] + intermediate_revs + [self.bad_rev]
+    self._update_revision_list_indexes()
+    return True
+
+  def _update_revision_list_indexes(self):
+    """Sets list_index, next and previous properties for each revision."""
     for i, rev in enumerate(self.revisions):
       rev.list_index = i
     for i in xrange(len(self.revisions)):
       if i:
         self.revisions[i].previous_revision = self.revisions[i - 1]
       if i < len(self.revisions) - 1:
         self.revisions[i].next_revision = self.revisions[i + 1]
 
   def check_improvement_direction(self):
     """Verifies that the change from 'good' to 'bad' is in the right direction.
 
-    The change between the test results obtained for the given 'good' and 'bad'
-    revisions is expected to be considered a regression. The `improvement_direction`
-    attribute is positive if a larger number is considered better, and negative if a
-    smaller number is considered better.
+    The change between the test results obtained for the given 'good' and
+    'bad' revisions is expected to be considered a regression. The
+    `improvement_direction` attribute is positive if a larger number is
+    considered better, and negative if a smaller number is considered better.
     """
     direction = self.improvement_direction
     if direction is None:
       return True
     good = self.good_rev.mean_value
     bad = self.bad_rev.mean_value
     if ((bad > good and direction > 0) or
         (bad < good and direction < 0)):
       self._set_failed_direction_results()
       return False
@@ skipping 71 matching lines @@
   def check_bisect_finished(self, revision):
     """Checks if this revision completes the bisection process.
 
     In this case 'finished' refers to finding one revision considered 'good'
     immediately preceding a revision considered 'bad' where the 'bad' revision
     does not contain a deps change.
     """
     if (revision.bad and revision.previous_revision and
         revision.previous_revision.good):
       if revision.deps_change():
-        self._expand_revision_range(revision)
-        return False
+        more_revisions = self._expand_revision_range(revision)
+        return not more_revisions
       self.culprit = revision
       return True
     if (revision.good and revision.next_revision and
         revision.next_revision.bad):
       if revision.next_revision.deps_change():
-        self._expand_revision_range(revision.next_revision)
-        return False
+        more_revisions = self._expand_revision_range(revision.next_revision)
+        return not more_revisions
       self.culprit = revision.next_revision
       return True
     return False
 
   def wait_for_all(self, revision_list):
     """Waits for all revisions in list to finish."""
     while any([r.in_progress for r in revision_list]):
       self.wait_for_any(revision_list)
       for revision in revision_list:
         revision.update_status()
@@ skipping 63 matching lines @@
     return 'linux_perf_tester'
 
   def get_builder_bot_for_this_platform(self):
     # TODO: Actually look at the current platform.
     return 'linux_perf_bisect_builder'
 
   def get_platform_gs_prefix(self):
     # TODO: Actually check the current platform
     return 'gs://chrome-perf/Linux Builder/full-build-linux_'