Changed script to pickup right chromium revision regardless of what's passed in

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 151 matching lines @@
 
   def ParseDirectoryIndex(self):
     """Parses the Google Storage directory listing into a list of revision
     numbers."""
 
     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."""
+

[Robert Sesek, 2013/09/23 16:37:15]

remove

[kareng, 2013/09/23 21:20:32]

Done.

       handle = urllib.urlopen(url)
       document = ElementTree.parse(handle)
 
       # All nodes in the tree are namespaced. Get the root's tag name to extract
       # the namespace. Etree does namespaces as |{namespace}tag|.
       root_tag = document.getroot().tag
       end_ns_pos = root_tag.find('}')
       if end_ns_pos == -1:
         raise Exception("Could not locate end namespace for directory index")
       namespace = root_tag[:end_ns_pos + 1]
@@ skipping 32 matching lines @@
       (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 = 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_all = map(int, self.ParseDirectoryIndex())
+
+    revlist = [x for x in revlist_all if x >= int(minrev) and x <= int(maxrev)]
     revlist.sort()
-    return revlist
+
+    # set good and bad revisions to be legit revisions

[Robert Sesek, 2013/09/23 16:37:15]

Proper punctuation, capitalization, and grammar for all comments is required.

[kareng, 2013/09/23 21:20:32]

Done.

+    if revlist:
+      if (self.good_revision < self.bad_revision):

[Robert Sesek, 2013/09/23 16:37:15]

nit: no () around conditions unless necessary for precedence

[kareng, 2013/09/23 21:20:32]

Done.

+        self.good_revision = revlist[0]
+        self.bad_revision = revlist[-1]
+      else:
+        self.bad_revision = revlist[0]
+        self.good_revision = revlist[-1]
+
+      #fix chromium rev so that the deps blink revision matches VERSIONS file

[Robert Sesek, 2013/09/23 16:37:15]

nit: same as above, but add a space after #

[kareng, 2013/09/23 21:20:32]

Done.

+      if (self.base_url == WEBKIT_BASE_URL):
+        revlist_all.sort()
+        self.good_revision, revlist = FixChromiumRevForBlink(revlist,
+                                                             revlist_all,
+                                                             self,
+                                                             self.good_revision)
+        self.bad_revision, revlist = FixChromiumRevForBlink(revlist,
+                                                            revlist_all,
+                                                            self,
+                                                            self.bad_revision)
+      return revlist

[Michael Moss, 2013/09/20 21:10:12]

You probably want to always return 'revlist' (i.e. outside this if block), to be
consistent with previous behavior. In particular, if this block isn't run, then
when it's used below with:

if len(revlist) ...

It will throw an exception for calling len() on NoneType.

[kareng, 2013/09/23 21:20:32]

yes that was a mistake of tabbing. ty!

 
   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 = 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()
@@ skipping 363 matching lines @@
           os.unlink(f)
         except OSError:
           pass
       sys.exit(0)
 
     rev = revlist[pivot]
 
   return (revlist[minrev], revlist[maxrev])
 
 
+def GetBlinkDEPSREvisionForChromiumRevision(rev):

[Robert Sesek, 2013/09/23 16:37:15]

Lowercase the E in Revision

[kareng, 2013/09/23 21:20:32]

Done.

+  """Returns the blink revision that was in REVISIONS file at
+  chromium revision |rev|."""
+  # . doesn't match newlines without re.DOTALL, so this is safe.
+  blink_re = re.compile(r'webkit_revision\D*(\d+)')
+  url = urllib.urlopen(DEPS_FILE % rev)
+  m = blink_re.search(url.read())
+  url.close()
+  if m:
+    return int(m.group(1))
+  else:
+    raise Exception('Could not get blink revision for cr rev %d' % rev)

[Robert Sesek, 2013/09/23 16:37:15]

Capitalize Blink and spell out Chromium. This is a message people may see. It
should be clear.

[kareng, 2013/09/23 21:20:32]

Done.

+
+
 def GetBlinkRevisionForChromiumRevision(self, rev):
   """Returns the blink revision that was in REVISIONS file at
   chromium revision |rev|."""
-  # . doesn't match newlines without re.DOTALL, so this is safe.
   file_url = "%s/%s%d/REVISIONS" % (self.base_url,
                                     self._listing_platform_dir, rev)
   url = urllib.urlopen(file_url)
   data = json.loads(url.read())
   url.close()
   if 'webkit_revision' in data:
     return data['webkit_revision']
   else:
     raise Exception('Could not get blink revision for cr rev %d' % rev)
 
+def FixChromiumRevForBlink(revisions_final, revisions, self, rev):
+  """Returns the chromium revision that has the correct blink revision
+  for blink bisect, deps and REVISIONS file might not match since
+  blink snapshots point to tip of tree blink.
+  chromium revision |rev."""

[Michael Moss, 2013/09/20 21:10:12]

Last line in the comment looks like copy-paste error.

[kareng, 2013/09/23 21:20:32]

Done.

+
+  blink_deps_rev = GetBlinkDEPSREvisionForChromiumRevision(rev)
+
+  while (GetBlinkRevisionForChromiumRevision(self, rev) > blink_deps_rev):
+    idx = revisions.index(rev)
+    rev = revisions[idx-1]

[Michael Moss, 2013/09/20 21:10:12]

What happens if 'rev' is the first item in 'revisions' (idx==0)?

[kareng, 2013/09/23 21:20:32]

heh good, point. i changed so now it doesn't continue otherwise, which means we
return what was sent in. i could also error out wasn't sure what's best to do.

+    if rev not in revisions_final:
+      revisions_final.insert(0, rev)
+
+  revisions_final.sort()
+  return rev, revisions_final

[Michael Moss, 2013/09/20 21:10:12]

It's probably worth noting that when you modify 'revisions_final' above, you are
modifying the list that the caller sees. In other words, it's kind of redundant,
and bit a confusing, to include it in the return values.

If you don't intend to modify it, then you need to make a copy (e.g.
"revisions_copy = list(revisions_final)") and return the copy. If you aren't
worried about that side-effect, then maybe don't return it, so it's more obvious
that it's intended to be modified "in place". And adding something about that
side-effect to the docstring wouldn't hurt.

[kareng, 2013/09/23 21:20:32]

so you think its better to copy and return the copy? isn't that more costly?

 On 2013/09/20 21:10:12, Michael Moss wrote:

[Michael Moss, 2013/09/23 22:01:56]

Modifying in-place is fine with me, just make it explicit in docstring and
return value.

 
 def GetChromiumRevision(url):
   """Returns the chromium revision read from given URL."""
   try:
     # Location of the latest build revision number
     return int(urllib.urlopen(url).read())
   except Exception, e:
     print('Could not determine latest revision. This could be bad...')
     return 999999999
 
@@ skipping 130 matching lines @@
     print 'BLINK CHANGELOG URL:'
     print '  ' + BLINK_CHANGELOG_URL % (max_blink_rev, min_blink_rev)
   print 'CHANGELOG URL:'
   if opts.official_builds:
     print OFFICIAL_CHANGELOG_URL % (min_chromium_rev, max_chromium_rev)
   else:
     print '  ' + CHANGELOG_URL % (min_chromium_rev, max_chromium_rev)
 
 if __name__ == '__main__':
   sys.exit(main())