Update bisect-builds.py to support bisecting at better granularity

This change adds a --recent option to bisect-builds that tells it to use the build.chromium.org/f/chromium build listings which have much better granularity, but only 2-3 months of history.

One minor bug was also fixed that caused bisect-builds to report a bigger revision range than necessary.

BUG=82421
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93257

[jbates, 2011-07-20 00:47:17]

Please review.. a couple updates to our awesome bisect-builds tool to possibly
make it a little awesomer.

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py
File tools/bisect-builds.py (left):

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#oldco...
tools/bisect-builds.py:379: good_rev = revlist[0]
good_rev is not used anymore, so this was unnecessary.

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#oldco...
tools/bisect-builds.py:383: bad = len(revlist) - 1
Moved to inside Bisect -- it already is passed the revlist.

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py
File tools/bisect-builds.py (right):

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:310: bad = len(revlist) - 1
Cleanup

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:331: last_known_good_rev = test_rev
This fixes a bug that sometimes caused the reported URL to contain an extra set
of revisions.

[Robert Sesek, 2011-07-20 19:05:11]

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py
File tools/bisect-builds.py (right):

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:47: def __init__(self, platform, good_revision,
bad_revision, is_recent):
|is_recent| sounds like it's describing the context. I'd call this |use_recent|.

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:207: index = document.find(RECENT_REV_HTML_PREFIX)
Why not use a regex to do this? Use re.compile for the constant and then just
findall()?

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:331: last_known_good_rev = test_rev
On 2011/07/20 00:47:17, jbates wrote:
> This fixes a bug that sometimes caused the reported URL to contain an extra
set
> of revisions.

Looks like this fixes http://code.google.com/p/chromium/issues/detail?id=82421 ?

If so, add a BUG= line to the CL description.

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:365: parser.add_option('-h', '--help',
I think this is done for you automatically by the option parser...

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:422: context.is_recent = opts.recent
You're not validating this, so why not pass it directly to the PathContext
constructor?

[jbates, 2011-07-20 21:08:42]

Thanks for the feedback, fixed it up.

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py
File tools/bisect-builds.py (right):

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:47: def __init__(self, platform, good_revision,
bad_revision, is_recent):
On 2011/07/20 19:05:11, rsesek wrote:
> |is_recent| sounds like it's describing the context. I'd call this
|use_recent|.

Done.

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:207: index = document.find(RECENT_REV_HTML_PREFIX)
On 2011/07/20 19:05:11, rsesek wrote:
> Why not use a regex to do this? Use re.compile for the constant and then just
> findall()?

Done.

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:331: last_known_good_rev = test_rev
On 2011/07/20 19:05:11, rsesek wrote:
> On 2011/07/20 00:47:17, jbates wrote:
> > This fixes a bug that sometimes caused the reported URL to contain an extra
> set
> > of revisions.
> 
> Looks like this fixes http://code.google.com/p/chromium/issues/detail?id=82421
?
> 
> If so, add a BUG= line to the CL description.

Done.

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:365: parser.add_option('-h', '--help',
On 2011/07/20 19:05:11, rsesek wrote:
> I think this is done for you automatically by the option parser...

Done.

http://codereview.chromium.org/7461012/diff/2001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:422: context.is_recent = opts.recent
On 2011/07/20 19:05:11, rsesek wrote:
> You're not validating this, so why not pass it directly to the PathContext
> constructor?

Done.

[Robert Sesek, 2011-07-20 21:12:50]

http://codereview.chromium.org/7461012/diff/6001/tools/bisect-builds.py
File tools/bisect-builds.py (right):

http://codereview.chromium.org/7461012/diff/6001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:203: return re.findall(r"\"\d+/\">(\d+)/</a>", document)
Maybe for some extra sanity, pattern-match the first \d+ and then use
substitution to make the value between the <a>/</a> match that first pattern
group. Something like this:
  r"<a href=\"(\d+)/\">\1/</a>"

[jbates, 2011-07-20 21:25:12]

http://codereview.chromium.org/7461012/diff/6001/tools/bisect-builds.py
File tools/bisect-builds.py (right):

http://codereview.chromium.org/7461012/diff/6001/tools/bisect-builds.py#newco...
tools/bisect-builds.py:203: return re.findall(r"\"\d+/\">(\d+)/</a>", document)
On 2011/07/20 21:12:51, rsesek wrote:
> Maybe for some extra sanity, pattern-match the first \d+ and then use
> substitution to make the value between the <a>/</a> match that first pattern
> group. Something like this:
>   r"<a href=\"(\d+)/\">\1/</a>"

Good idea, Done.

[Robert Sesek, 2011-07-20 21:29:17]

LGTM with nit

http://codereview.chromium.org/7461012/diff/5002/tools/bisect-builds.py
File tools/bisect-builds.py (right):

http://codereview.chromium.org/7461012/diff/5002/tools/bisect-builds.py#newco...
tools/bisect-builds.py:195: 
nit: two blank lines between top-level definitions

[jbates, 2011-07-20 21:32:10]

http://codereview.chromium.org/7461012/diff/5002/tools/bisect-builds.py
File tools/bisect-builds.py (right):

http://codereview.chromium.org/7461012/diff/5002/tools/bisect-builds.py#newco...
tools/bisect-builds.py:195: 
On 2011/07/20 21:29:17, rsesek wrote:
> nit: two blank lines between top-level definitions

Done.

[Nico, 2011-07-20 21:50:46]

post-commit drive-by (sorry!): Why didn't you let the tool transparently use the
better-granularity repo for recent revisions? Then no flag would be necessary.

(I'm mostly curious and don't want to complain about you improving this script!
I'm happy with "that'd be awkward to code")

[jbates, 2011-07-20 21:59:25]

On 2011/07/20 21:50:46, Nico wrote:
> post-commit drive-by (sorry!): Why didn't you let the tool transparently use
the
> better-granularity repo for recent revisions? Then no flag would be necessary.
> 
> (I'm mostly curious and don't want to complain about you improving this
script!
> I'm happy with "that'd be awkward to code")

I wanted to do it that way first actually. It just got slightly awkward as you
guessed (also I'm a bit of a python novice). I'm open to a followup patch
though. Here is what I was thinking:

- bisect with the low granularity server.
- if the resulting range is within the recent builds, continue bisecting the
recent builds.

How does that sound?

[Nico, 2011-07-20 22:03:08]

On Wed, Jul 20, 2011 at 2:59 PM,  <jbates@chromium.org> wrote:
> On 2011/07/20 21:50:46, Nico wrote:
>>
>> post-commit drive-by (sorry!): Why didn't you let the tool transparently
>> use
>
> the
>>
>> better-granularity repo for recent revisions? Then no flag would be
>> necessary.
>
>> (I'm mostly curious and don't want to complain about you improving this
>
> script!
>>
>> I'm happy with "that'd be awkward to code")
>
> I wanted to do it that way first actually. It just got slightly awkward as
> you
> guessed (also I'm a bit of a python novice). I'm open to a followup patch
> though. Here is what I was thinking:
>
> - bisect with the low granularity server.
> - if the resulting range is within the recent builds, continue bisecting the
> recent builds.
>
> How does that sound?

Sounds great, thanks!

>
> http://codereview.chromium.org/7461012/
>