Make tools/bisect_builds.py handle reversed bad/good revision ranges

tools/bisect-builds.py

 #!/usr/bin/env python
 # Copyright (c) 2012 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.
 
 """Snapshot Build Bisect Tool
 
 This script bisects a snapshot archive using binary search. It starts at
 a bad revision (it will try to guess HEAD) and asks for a last known-good
 revision. It will then binary search across this revision range by downloading,
@@ skipping 14 matching lines @@
 # Official Changelogs URL.
 OFFICIAL_CHANGELOG_URL = 'http://omahaproxy.appspot.com/'\
                          'changelog?old_version=%s&new_version=%s'
 
 # DEPS file URL.
 DEPS_FILE= 'http://src.chromium.org/viewvc/chrome/trunk/src/DEPS?revision=%d'
 # WebKit Changelogs URL.
 WEBKIT_CHANGELOG_URL = 'http://trac.webkit.org/log/' \
                        'trunk/?rev=%d&stop_rev=%d&verbose=on&limit=10000'
 
-DONE_MESSAGE = 'You are probably looking for a change made after ' \
-               '%s (known good), but no later than %s (first known bad).'
+DONE_MESSAGE_GOOD_MIN = 'You are probably looking for a change made after %s ' \
+                        '(known good), but no later than %s (first known bad).'
+DONE_MESSAGE_GOOD_MAX = 'You are probably looking for a change made after %s ' \
+                        '(known bad), but no later than %s (first known good).'
 
 ###############################################################################
 
 import math
 import optparse
 import os
 import pipes
 import re
 import shutil
 import subprocess
@@ skipping 86 matching lines @@
     """Returns a URL to the LAST_CHANGE file."""
     return BASE_URL + '/' + self._listing_platform_dir + 'LAST_CHANGE'
 
   def GetLaunchPath(self):
     """Returns a relative path (presumably from the archive extraction location)
     that is used to run the executable."""
     return os.path.join(self._archive_extract_dir, self._binary_name)
 
   def ParseDirectoryIndex(self):
     """Parses the Google Storage directory listing into a list of revision
     numbers. The range starts with self.good_revision and goes until

[Nico, 2012/11/01 19:06:28]

It looks like the 2nd sentence in the comment is no longer true. Can you remove
it?

[enne (OOO), 2012/11/02 22:26:57]

Done.

     self.bad_revision."""
 
     def _FetchAndParse(url):
       """Fetches a URL and returns a 2-Tuple of ([revisions], next-marker). If
       next-marker is not None, then the listing is a partial listing and another
       fetch should be performed with next-marker being the marker= GET
       parameter."""
       handle = urllib.urlopen(url)
       document = ElementTree.parse(handle)
 
@@ skipping 37 matching lines @@
     while next_marker:
       next_url = self.GetListingURL(next_marker)
       (new_revisions, next_marker) = _FetchAndParse(next_url)
       revisions.extend(new_revisions)
     return revisions
 
   def GetRevList(self):
     """Gets the list of revision numbers between self.good_revision and
     self.bad_revision."""
     # Download the revlist and filter for just the range between good and bad.
-    minrev = self.good_revision
-    maxrev = self.bad_revision
+    minrev = min(self.good_revision, self.bad_revision)
+    maxrev = max(self.good_revision, self.bad_revision)
     revlist = map(int, self.ParseDirectoryIndex())
     revlist = [x for x in revlist if x >= int(minrev) and x <= int(maxrev)]
     revlist.sort()
     return revlist
 
   def GetOfficialBuildsList(self):
     """Gets the list of official build numbers between self.good_revision and
     self.bad_revision."""
     # Download the revlist and filter for just the range between good and bad.
-    minrev = self.good_revision
-    maxrev = self.bad_revision
+    minrev = min(self.good_revision, self.bad_revision)
+    maxrev = max(self.good_revision, self.bad_revision)
     handle = urllib.urlopen(OFFICIAL_BASE_URL)
     dirindex = handle.read()
     handle.close()
     build_numbers = re.findall(r'<a href="([0-9][0-9].*)/">', dirindex)
     final_list = []
     i = 0
     parsed_build_numbers = [LooseVersion(x) for x in build_numbers]
     for build_number in sorted(parsed_build_numbers):
       path = OFFICIAL_BASE_URL + '/' + str(build_number) + '/' + \
              self._listing_platform_dir + self.archive_name
@@ skipping 161 matching lines @@
            bad_rev=0,
            num_runs=1,
            try_args=(),
            profile=None,
            evaluate=AskIsGoodBuild):
   """Given known good and known bad revisions, run a binary search on all
   archived revisions to determine the last known good revision.
 
   @param platform Which build to download/run ('mac', 'win', 'linux64', etc.).
   @param official_builds Specify build type (Chromium or Official build).
-  @param good_rev Number/tag of the last known good revision.
-  @param bad_rev Number/tag of the first known bad revision.
+  @param good_rev Number/tag of the known good revision.
+  @param bad_rev Number/tag of the known bad revision.
   @param num_runs Number of times to run each build for asking good/bad.
   @param try_args A tuple of arguments to pass to the test application.
   @param profile The name of the user profile to run with.
   @param evaluate A function which returns 'g' if the argument build is good,
                   'b' if it's bad or 'u' if unknown.
 
   Threading is used to fetch Chromium revisions in the background, speeding up
   the user's experience. For example, suppose the bounds of the search are
   good_rev=0, bad_rev=100. The first revision to be checked is 50. Depending on
   whether revision 50 is good or bad, the next revision to check will be either
@@ skipping 22 matching lines @@
   if official_builds:
     revlist = context.GetOfficialBuildsList()
   else:
     revlist = context.GetRevList()
 
   # Get a list of revisions to bisect across.
   if len(revlist) < 2:  # Don't have enough builds to bisect.
     msg = 'We don\'t have enough builds to bisect. revlist: %s' % revlist
     raise RuntimeError(msg)
 
-  print 'Bisecting range [%s, %s].' % (revlist[0], revlist[-1])
-
   # Figure out our bookends and first pivot point; fetch the pivot revision.
-  good = 0
-  bad = len(revlist) - 1
-  pivot = bad / 2
+  minrev = 0
+  maxrev = len(revlist) - 1
+  pivot = maxrev / 2
   rev = revlist[pivot]
   zipfile = _GetDownloadPath(rev)
   initial_fetch = DownloadJob(context, 'initial_fetch', rev, zipfile)
   initial_fetch.Start()
   initial_fetch.WaitFor()
 
   # Binary search time!
-  while zipfile and bad - good > 1:
+  while zipfile and maxrev - minrev > 1:
+    min_str, max_str = ["good", "bad"][::1 - 2 * (bad_rev < good_rev)]

[Nico, 2012/11/01 19:06:28]

What is this, perl? Just use an if and 4 lines.

[enne (OOO), 2012/11/01 19:20:32]

Haha.  Will do.

+    print 'Bisecting range [%s (%s), %s (%s)].' % (revlist[minrev], min_str, \
+                                                   revlist[maxrev], max_str)
+
     # Pre-fetch next two possible pivots
     #   - down_pivot is the next revision to check if the current revision turns
     #     out to be bad.
     #   - up_pivot is the next revision to check if the current revision turns
     #     out to be good.
-    down_pivot = int((pivot - good) / 2) + good
+    down_pivot = int((pivot - minrev) / 2) + minrev
     down_fetch = None
-    if down_pivot != pivot and down_pivot != good:
+    if down_pivot != pivot and down_pivot != minrev:
       down_rev = revlist[down_pivot]
       down_fetch = DownloadJob(context, 'down_fetch', down_rev,
                                _GetDownloadPath(down_rev))
       down_fetch.Start()
 
-    up_pivot = int((bad - pivot) / 2) + pivot
+    up_pivot = int((maxrev - pivot) / 2) + pivot
     up_fetch = None
-    if up_pivot != pivot and up_pivot != bad:
+    if up_pivot != pivot and up_pivot != maxrev:
       up_rev = revlist[up_pivot]
       up_fetch = DownloadJob(context, 'up_fetch', up_rev,
                              _GetDownloadPath(up_rev))
       up_fetch.Start()
 
     # Run test on the pivot revision.
     status = None
     stdout = None
     stderr = None
     try:
       (status, stdout, stderr) = RunRevision(context,
                                              rev,
                                              zipfile,
                                              profile,
                                              num_runs,
                                              try_args)
     except Exception, e:
       print >>sys.stderr, e
     os.unlink(zipfile)
     zipfile = None
 
     # Call the evaluate function to see if the current revision is good or bad.
     # On that basis, kill one of the background downloads and complete the
     # other, as described in the comments above.
     try:
       answer = evaluate(rev, official_builds, status, stdout, stderr)
-      if answer == 'g':
-        good = pivot
+      if answer == 'g' and good_rev < bad_rev or \
+          answer == 'b' and bad_rev < good_rev:
+        minrev = pivot
         if down_fetch:
           down_fetch.Stop()  # Kill the download of the older revision.
         if up_fetch:
           up_fetch.WaitFor()
           pivot = up_pivot
           zipfile = up_fetch.zipfile
-      elif answer == 'b':
-        bad = pivot
+      elif answer == 'b' and good_rev < bad_rev or \
+          answer == 'g' and bad_rev < good_rev:
+        maxrev = pivot
         if up_fetch:
           up_fetch.Stop()  # Kill the download of the newer revision.
         if down_fetch:
           down_fetch.WaitFor()
           pivot = down_pivot
           zipfile = down_fetch.zipfile
       elif answer == 'u':
         # Nuke the revision from the revlist and choose a new pivot.
         revlist.pop(pivot)
-        bad -= 1  # Assumes bad >= pivot.
+        maxrev -= 1  # Assumes maxrev >= pivot.
 
         fetch = None
-        if bad - good > 1:
+        if maxrev - minrev > 1:
           # Alternate between using down_pivot or up_pivot for the new pivot
           # point, without affecting the range. Do this instead of setting the
           # pivot to the midpoint of the new range because adjacent revisions
           # are likely affected by the same issue that caused the (u)nknown
           # response.
           if up_fetch and down_fetch:
             fetch = [up_fetch, down_fetch][len(revlist) % 2]
           elif up_fetch:
             fetch = up_fetch
           else:
@@ skipping 16 matching lines @@
       for f in [_GetDownloadPath(revlist[down_pivot]),
                 _GetDownloadPath(revlist[up_pivot])]:
         try:
           os.unlink(f)
         except OSError:
           pass
       sys.exit(0)
 
     rev = revlist[pivot]
 
-  return (revlist[good], revlist[bad])
+  return (revlist[minrev], revlist[maxrev])
 
 
 def GetWebKitRevisionForChromiumRevision(rev):
   """Returns the webkit revision that was in chromium's DEPS file at
   chromium revision |rev|."""
   # . doesn't match newlines without re.DOTALL, so this is safe.
   webkit_re = re.compile(r'webkit_revision.:\D*(\d+)')
   url = urllib.urlopen(DEPS_FILE % rev)
   m = webkit_re.search(url.read())
   url.close()
@@ skipping 66 matching lines @@
   else:
     good_rev = '0.0.0.0' if opts.official_builds else 0
 
   if opts.official_builds:
     good_rev = LooseVersion(good_rev)
     bad_rev = LooseVersion(bad_rev)
   else:
     good_rev = int(good_rev)
     bad_rev = int(bad_rev)
 
-  if good_rev > bad_rev:
-    print ('The good revision (%s) must precede the bad revision (%s).\n' %
-           (good_rev, bad_rev))
-    parser.print_help()
-    return 1
-
   if opts.times < 1:
     print('Number of times to run (%d) must be greater than or equal to 1.' %
           opts.times)
     parser.print_help()
     return 1
 
-  (last_known_good_rev, first_known_bad_rev) = Bisect(
+  (min_chromium_rev, max_chromium_rev) = Bisect(
       opts.archive, opts.official_builds, good_rev, bad_rev, opts.times, args,
       opts.profile)
 
   # Get corresponding webkit revisions.
   try:
-    last_known_good_webkit_rev = GetWebKitRevisionForChromiumRevision(
-        last_known_good_rev)
-    first_known_bad_webkit_rev = GetWebKitRevisionForChromiumRevision(
-        first_known_bad_rev)
+    min_webkit_rev = GetWebKitRevisionForChromiumRevision(min_chromium_rev)
+    max_webkit_rev = GetWebKitRevisionForChromiumRevision(max_chromium_rev)
   except Exception, e:
     # Silently ignore the failure.
-    last_known_good_webkit_rev, first_known_bad_webkit_rev = 0, 0
+    min_webkit_rev, max_webkit_rev = 0, 0
 
   # We're done. Let the user know the results in an official manner.
-  print DONE_MESSAGE % (str(last_known_good_rev), str(first_known_bad_rev))
-  if last_known_good_webkit_rev != first_known_bad_webkit_rev:
+  if good_rev > bad_rev:
+    print DONE_MESSAGE_GOOD_MAX % (str(min_chromium_rev), str(max_chromium_rev))
+  else:
+    print DONE_MESSAGE_GOOD_MIN % (str(min_chromium_rev), str(max_chromium_rev))
+
+  if min_webkit_rev != max_webkit_rev:
     print 'WEBKIT CHANGELOG URL:'
-    print '  ' + WEBKIT_CHANGELOG_URL % (first_known_bad_webkit_rev,
-                                         last_known_good_webkit_rev)
+    print '  ' + WEBKIT_CHANGELOG_URL % (max_webkit_rev, min_webkit_rev)

[Nico, 2012/11/01 19:06:28]

Should this be printed backwards too?

[enne (OOO), 2012/11/02 22:26:57]

This is correct.

   print 'CHANGELOG URL:'
   if opts.official_builds:
-    print OFFICIAL_CHANGELOG_URL % (last_known_good_rev, first_known_bad_rev)
+    print OFFICIAL_CHANGELOG_URL % (min_chromium_rev, max_chromium_rev)
   else:
-    print '  ' + CHANGELOG_URL % (last_known_good_rev, first_known_bad_rev)
+    print '  ' + CHANGELOG_URL % (min_chromium_rev, max_chromium_rev)
 
 if __name__ == '__main__':
   sys.exit(main())