Modified the bisect script to work for official Chrome builds.

bisect-builds.py

Index: bisect-builds.py
===================================================================
--- bisect-builds.py	(revision 129264)
+++ bisect-builds.py	(working copy)
@@ -15,16 +15,21 @@
 # The root URL for storage.
 BASE_URL = 'http://commondatastorage.googleapis.com/chromium-browser-snapshots'
 
+# The root URL for official builds.
+OFFICIAL_BASE_URL = 'http://chrome4linux.mtv.corp.google.com/archives/'

[Robert Sesek, 2012/05/21 21:39:47]

Do not end this with a / for consistency with the above URL. Also update any
format strings that use this.

[Robert Sesek, 2012/05/21 21:39:47]

This instead of chrome-master2.mtv/official_builds/?

[anantha, 2012/05/22 22:55:39]

Done.

[anantha, 2012/05/22 22:55:39]

This location has files from chrome-master2.mtv and chromesigning.
On 2012/05/21 21:39:47, rsesek wrote:

+
 # URL to the ViewVC commit page.
 BUILD_VIEWVC_URL = 'http://src.chromium.org/viewvc/chrome?view=rev&revision=%d'
 
 # Changelogs URL.
 CHANGELOG_URL = 'http://build.chromium.org/f/chromium/' \
                 'perf/dashboard/ui/changelog.html?url=/trunk/src&range=%d:%d'
+# 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'
@@ -42,6 +47,7 @@
 import tempfile
 import threading
 import urllib
+from distutils.version import LooseVersion
 from xml.etree import ElementTree
 import zipfile
 
@@ -49,12 +55,13 @@
 class PathContext(object):
   """A PathContext is used to carry the information used to construct URLs and
   paths when dealing with the storage server and archives."""
-  def __init__(self, platform, good_revision, bad_revision):
+  def __init__(self, platform, good_revision, bad_revision, build_type):
     super(PathContext, self).__init__()
     # Store off the input parameters.
     self.platform = platform  # What's passed in to the '-a/--archive' option.
     self.good_revision = good_revision
     self.bad_revision = bad_revision
+    self.build_type = build_type

[Robert Sesek, 2012/05/21 21:39:47]

self.is_official per comment in Main()

[anantha, 2012/05/22 22:55:39]

Done.

 
     # The name of the ZIP file in a revision directory on the server.
     self.archive_name = None
@@ -65,19 +72,41 @@
     #   _binary_name = The name of the executable to run.
     if self.platform == 'linux' or self.platform == 'linux64':
       self._listing_platform_dir = 'Linux/'
-      self.archive_name = 'chrome-linux.zip'
-      self._archive_extract_dir = 'chrome-linux'
+      if build_type != 'Official':

[Robert Sesek, 2012/05/21 21:39:47]

This is getting messy. Beak this down into two top-level if branches:

if self.is_official:
  if self.platform == 'linux' ...
else:
  ...

[anantha, 2012/05/22 22:55:39]

Done.

+        self._archive_extract_dir = 'chrome-linux'
+      else:
+        self._archive_extract_dir = 'chrome-lucid64bit'
       self._binary_name = 'chrome'
       # Linux and x64 share all the same path data except for the archive dir.
       if self.platform == 'linux64':
-        self._listing_platform_dir = 'Linux_x64/'
+        if build_type != 'Official':
+          self.archive_name = 'chrome-linux.zip'
+          self._listing_platform_dir = 'Linux_x64/'
+        else:
+          self.archive_name = 'chrome-lucid64bit.zip'
+          self._listing_platform_dir = 'lucid64bit/'
+      else:
+        if build_type != 'Official':
+           self.archive_name = 'chrome-linux.zip'
+           self._listing_platform_dir = 'Linux_x32/'
+        else:
+           self.archive_name = 'chrome-lucid32bit.zip'
+           self._listing_platform_dir = 'lucid32bit/'
     elif self.platform == 'mac':
-      self._listing_platform_dir = 'Mac/'
+      if build_type != 'Official':
+        self._listing_platform_dir = 'Mac/'
+        self._binary_name = 'Chromium.app/Contents/MacOS/Chromium'
+      else:
+        self._listing_platform_dir = 'mac/'
+        self._binary_name = 'Google Chrome.app/Contents/MacOS/Google Chrome'
       self.archive_name = 'chrome-mac.zip'
       self._archive_extract_dir = 'chrome-mac'
-      self._binary_name = 'Chromium.app/Contents/MacOS/Chromium'
+
     elif self.platform == 'win':
-      self._listing_platform_dir = 'Win/'
+      if build_type != 'Official':
+        self._listing_platform_dir = 'Win/'
+      else:
+        self._listing_platform_dir = 'win/'
       self.archive_name = 'chrome-win32.zip'
       self._archive_extract_dir = 'chrome-win32'
       self._binary_name = 'chrome.exe'
@@ -94,8 +123,13 @@
 
   def GetDownloadURL(self, revision):
     """Gets the download URL for a build archive of a specific revision."""
-    return "%s/%s%d/%s" % (
-        BASE_URL, self._listing_platform_dir, revision, self.archive_name)
+    if self.build_type != 'Official':
+      return "%s/%s%d/%s" % (
+          BASE_URL, self._listing_platform_dir, revision, self.archive_name)
+    else:
+      return "%s%s/%s%s" % (
+          OFFICIAL_BASE_URL, revision, self._listing_platform_dir,
+          self.archive_name)
 
   def GetLastChangeURL(self):
     """Returns a URL to the LAST_CHANGE file."""
@@ -150,19 +184,23 @@
         except ValueError:
           pass
       return (revisions, next_marker)
+    if self.build_type != 'Official':

[Robert Sesek, 2012/05/21 21:39:47]

This logic doesn't belong in this function, as it's not talking to the Google
Data Storage. Why is this here if you have GetBuildsList()?

[anantha, 2012/05/22 22:55:39]

Correct. I forgot to clean this up.

+      # Fetch the first list of revisions.
+      (revisions, next_marker) = _FetchAndParse(self.GetListingURL())
 
-    # Fetch the first list of revisions.
-    (revisions, next_marker) = _FetchAndParse(self.GetListingURL())
+      # If the result list was truncated, refetch with the next marker. Do this
+      # until an entire directory listing is done.
+      while next_marker:
+        next_url = self.GetListingURL(next_marker)
+        (new_revisions, next_marker) = _FetchAndParse(next_url)
+        revisions.extend(new_revisions)
+      return revisions
+    else:
+      handle = urllib.urlopen(OFFICIAL_BASE_URL)
+      dirindex = handle.read()
+      handle.close()
+      return re.findall(r'<a href="([0-9][0-9].*)/">', dirindex)
 
-    # If the result list was truncated, refetch with the next marker. Do this
-    # until an entire directory listing is done.
-    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."""
@@ -170,11 +208,42 @@
     minrev = self.good_revision
     maxrev = self.bad_revision
     revlist = map(int, self.ParseDirectoryIndex())
-    revlist = [x for x in revlist if x >= minrev and x <= maxrev]
+    revlist = [x for x in revlist if x >= int(minrev) and x <= int(maxrev)]
     revlist.sort()
     return revlist
 
+  def GetBuildsList(self):

[Robert Sesek, 2012/05/21 21:39:47]

naming: GetOfficialBuildsList

[anantha, 2012/05/22 22:55:39]

Done.

+    """Gets the list of revision numbers between self.good_revision and

[Robert Sesek, 2012/05/21 21:39:47]

nit: this isn't returning revision numbers, this is returning Chrome version
numbers

[anantha, 2012/05/22 22:55:39]

Done.

[anantha, 2012/05/22 22:55:39]

Done.

+    self.bad_revision."""
+    # Download the revlist and filter for just the range between good and bad.
+    minrev = self.good_revision
+    maxrev = 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)

[Robert Sesek, 2012/05/21 21:39:47]

This logic is duplicated above.

[anantha, 2012/05/22 22:55:39]

Removed from above.

+    final_list = []
+    start_index = '0'
+    end_index = '0'
+    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) + '/' + \

[Robert Sesek, 2012/05/21 21:39:47]

nit: should be indented 2

[anantha, 2012/05/22 22:55:39]

Done.

+            self._listing_platform_dir + self.archive_name
+     i = i+1
+     try:
+        connection = urllib.urlopen(path)

[Robert Sesek, 2012/05/21 21:39:47]

nit: over-indented

[anantha, 2012/05/22 22:55:39]

Done.

[anantha, 2012/05/22 22:55:39]

Done.

+        connection.close()
+        final_list.append(str(build_number))
+        if str(build_number) == minrev:
+          start_index = i
+        if str(build_number) == maxrev:
+          end_index = i
+     except urllib.HTTPError, e:
+        print e.getcode()
+    return final_list[start_index:end_index]
+
 def UnzipFilenameToDir(filename, dir):
   """Unzip |filename| to directory |dir|."""
   cwd = os.getcwd()
@@ -220,7 +289,10 @@
   """
   def ReportHook(blocknum, blocksize, totalsize):
     if quit_event and quit_event.isSet():
-     raise RuntimeError("Aborting download of revision %d" % rev)
+      if context.build_type != 'Official':

[Robert Sesek, 2012/05/21 21:39:47]

Don't do this. Just cast rev to a string and format it as such. Here and
elsewhere.

[anantha, 2012/05/22 22:55:39]

Done.

+        raise RuntimeError("Aborting download of revision %d" % rev)
+      else:
+        raise RuntimeError("Aborting download of revision %s" % rev)
     if progress_event and progress_event.isSet():
       size = blocknum * blocksize
       if totalsize == -1:  # Total size not known.
@@ -235,16 +307,17 @@
 
   download_url = context.GetDownloadURL(rev)
   try:
-    urllib.urlretrieve(download_url, filename, ReportHook)
-    if progress_event and progress_event.isSet():

[Robert Sesek, 2012/05/21 21:39:47]

Why'd you remove this?

-      print
+     urllib.urlretrieve(download_url, filename, ReportHook)
   except RuntimeError, e:
-    pass
+       pass

[Robert Sesek, 2012/05/21 21:39:47]

nit: over-indented

[anantha, 2012/05/22 22:55:39]

Done.

 
 
 def RunRevision(context, revision, zipfile, profile, num_runs, args):
   """Given a zipped revision, unzip it and run the test."""
-  print "Trying revision %d..." % revision
+  if context.build_type != 'Official':
+    print "Trying revision %d..." % revision
+  else:
+    print "Trying revision %s..." % revision
 
   # Create a temp directory and unzip the revision into it.
   cwd = os.getcwd()
@@ -252,8 +325,19 @@
   UnzipFilenameToDir(zipfile, tempdir)
   os.chdir(tempdir)
 
-  # Run the build as many times as specified.
-  testargs = [context.GetLaunchPath(), '--user-data-dir=%s' % profile] + args
+  if context.platform == 'linux' or context.platform == 'linux64':
+    if context.build_type == 'Official':
+      # Run the build as many times as specified.
+      testargs = [context.GetLaunchPath(), '--no-sandbox',

[Robert Sesek, 2012/05/21 21:39:47]

Why this? I think this should just be done manually and this logic shouldn't be
in the script.

[anantha, 2012/05/22 22:55:39]

Looks like we need root permission to setup the sandbox for Linux Official
Chrome builds.
On 2012/05/21 21:39:47, rsesek wrote:

+                  '--user-data-dir=%s' % profile] + args
+    else:
+      # Run the build as many times as specified.
+      testargs = [context.GetLaunchPath(),
+                  '--user-data-dir=%s' % profile] + args
+  else:
+    # Run the build as many times as specified.
+    testargs = [context.GetLaunchPath(), '--user-data-dir=%s' % profile] + args
+
   for i in range(0, num_runs):
     subproc = subprocess.Popen(testargs,
                                bufsize=-1,
@@ -270,18 +354,21 @@
   return (subproc.returncode, stdout, stderr)
 
 
-def AskIsGoodBuild(rev, status, stdout, stderr):
+def AskIsGoodBuild(rev, build_type, status, stdout, stderr):
   """Ask the user whether build |rev| is good or bad."""
   # Loop until we get a response that we can parse.
   while True:
-    response = raw_input('Revision %d is [(g)ood/(b)ad/(q)uit]: ' % int(rev))
+    if build_type != 'Official':
+      response = raw_input('Revision %d is [(g)ood/(b)ad/(q)uit]: ' % int(rev))
+    else:
+      response = raw_input('Revision %s is [(g)ood/(b)ad/(q)uit]: ' % str(rev))
     if response and response in ('g', 'b'):
       return response == 'g'
     if response and response == 'q':
       raise SystemExit()
 
 
-def Bisect(platform,
+def Bisect(platform, build_type,

[Robert Sesek, 2012/05/21 21:39:47]

nit: goes on its own line, and the parameter also needs to be documented below

[anantha, 2012/05/22 22:55:39]

Done.

            good_rev=0,
            bad_rev=0,
            num_runs=1,
@@ -318,16 +405,21 @@
   if not profile:
     profile = 'profile'
 
-  context = PathContext(platform, good_rev, bad_rev)
+  context = PathContext(platform, good_rev, bad_rev, build_type)
   cwd = os.getcwd()
 
-  _GetDownloadPath = lambda rev: os.path.join(cwd,
-      '%d-%s' % (rev, context.archive_name))
 
+
   print "Downloading list of known revisions..."
+  if build_type != 'Official':
+    _GetDownloadPath = lambda rev: os.path.join(cwd,
+      '%d-%s' % (rev, context.archive_name))
+    revlist = context.GetRevList()
+  else:
+    _GetDownloadPath = lambda rev: os.path.join(cwd,
+      '%s-%s' % (rev, context.archive_name))
+    revlist = context.GetBuildsList()
 
-  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
@@ -341,7 +433,10 @@
   zipfile = _GetDownloadPath(rev)
   progress_event = threading.Event()
   progress_event.set()
-  print "Downloading revision %d..." % rev
+  if build_type != 'Official':
+    print "Downloading revision %d..." % rev
+  else:
+    print "Downloading revision %s..." % rev
   FetchRevision(context, rev, zipfile,
                 quit_event=None, progress_event=progress_event)
 
@@ -400,14 +495,17 @@
     # On that basis, kill one of the background downloads and complete the
     # other, as described in the comments above.
     try:
-      if predicate(rev, status, stdout, stderr):
+      if predicate(rev, build_type, status, stdout, stderr):
         good = pivot
         if down_thread:
           down_quit_event.set()  # Kill the download of older revision.
           down_thread.join()
           os.unlink(down_zipfile)
         if up_thread:
-          print "Downloading revision %d..." % up_rev
+          if build_type == 'Official':
+            print "Downloading revision %s..." % up_rev
+          else:
+            print "Downloading revision %d..." % up_rev
           up_progress_event.set()  # Display progress of download.
           up_thread.join()  # Wait for newer revision to finish downloading.
           pivot = up_pivot
@@ -419,7 +517,10 @@
           up_thread.join()
           os.unlink(up_zipfile)
         if down_thread:
-          print "Downloading revision %d..." % down_rev
+          if build_type == 'Official':
+            print "Downloading revision %s..." % down_rev
+          else:
+            print "Downloading revision %d..." % down_rev
           down_progress_event.set()  # Display progress of download.
           down_thread.join()  # Wait for older revision to finish downloading.
           pivot = down_pivot
@@ -466,9 +567,11 @@
                     choices = choices,
                     help = 'The buildbot archive to bisect [%s].' %
                            '|'.join(choices))
-  parser.add_option('-b', '--bad', type = 'int',
+  parser.add_option('-o', '--build_type', type = 'str',

[Robert Sesek, 2012/05/21 21:39:47]

This should just be type bool and make the flag --official-chrome or
--official-builds

[anantha, 2012/05/22 22:55:39]

Done.

+                    help = 'Specify if it is a chromium or official build.')
+  parser.add_option('-b', '--bad', type = 'str',
                     help = 'The bad revision to bisect to.')
-  parser.add_option('-g', '--good', type = 'int',
+  parser.add_option('-g', '--good', type = 'str',
                     help = 'The last known good revision to bisect from.')
   parser.add_option('-p', '--profile', '--user-data-dir', type = 'str',
                     help = 'Profile to use; this will not reset every run. ' +
@@ -492,8 +595,15 @@
     return 1
 
   # Create the context. Initialize 0 for the revisions as they are set below.
-  context = PathContext(opts.archive, 0, 0)
+  context = PathContext(opts.archive, 0, 0, opts.build_type)
 
+  if opts.build_type == 'Official':
+    if opts.bad is None:

[Robert Sesek, 2012/05/21 21:39:47]

Join this and the previous line with an |and|

[anantha, 2012/05/22 22:55:39]

Done.

+      print ' Need a bad build range'

[Robert Sesek, 2012/05/21 21:39:47]

print >>sys.stderr, 'Needs...'

[anantha, 2012/05/22 22:55:39]

Done.

[Robert Sesek, 2012/05/23 21:24:00]

Not done. Also, maybe change the error to "Bisecting official builds requires a
bad build number."

+      print
+      parser.print_help()
+      return 1
+
   # Pick a starting point, try to get HEAD for this.
   if opts.bad:
     bad_rev = opts.bad
@@ -529,7 +639,8 @@
     return 1
 
   (last_known_good_rev, first_known_bad_rev) = Bisect(
-      opts.archive, good_rev, bad_rev, opts.times, args, opts.profile)
+      opts.archive, opts.build_type, good_rev, bad_rev, opts.times, args,
+      opts.profile)
 
   # Get corresponding webkit revisions.
   try:
@@ -542,16 +653,23 @@
     last_known_good_webkit_rev, first_known_bad_webkit_rev = 0, 0
 
   # We're done. Let the user know the results in an official manner.
-  print('You are probably looking for build %d.' % first_known_bad_rev)
+  if opts.build_type != 'Official':
+    print('You are probably looking for build %d.' % first_known_bad_rev)
+  else:
+    print('You are probably looking for build %s.' % first_known_bad_rev)
+
   if last_known_good_webkit_rev != first_known_bad_webkit_rev:
     print 'WEBKIT CHANGELOG URL:'
     print WEBKIT_CHANGELOG_URL % (first_known_bad_webkit_rev,
                                   last_known_good_webkit_rev)
   print 'CHANGELOG URL:'
-  print CHANGELOG_URL % (last_known_good_rev, first_known_bad_rev)
-  print 'Built at revision:'
-  print BUILD_VIEWVC_URL % first_known_bad_rev
+  if opts.build_type == 'Official':
+    print OFFICIAL_CHANGELOG_URL % (last_known_good_rev, first_known_bad_rev)
+  else:
+    print CHANGELOG_URL % (last_known_good_rev, first_known_bad_rev)
+    print 'Built at revision:'
+    print BUILD_VIEWVC_URL % first_known_bad_rev
 
 
 if __name__ == '__main__':
-  sys.exit(main())
+  sys.exit(main())

[Robert Sesek, 2012/05/21 21:39:47]

What's this change?

[anantha, 2012/05/22 22:55:39]

I somehow ended up adding two spaces. So deleted them.