Add programmatic interface

If the good/bad test can be written as a predicate function (e.g., by parsing stdout/stderr), then bisect_builds.py can be imported from a separate script and run non-interactively.

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

[Evan Martin, 2011-07-14 18:57:52]

rsesek should probably be the primary reviewer.
I think it would help to do the code movement in a separate change first.

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

http://codereview.chromium.org/7329005/diff/1/tools/bisect-builds.py#newcode39
tools/bisect-builds.py:39: import threading
I think this list is alphabetized.

http://codereview.chromium.org/7329005/diff/1/tools/bisect-builds.py#newcode168
tools/bisect-builds.py:168: #revisions = map(lambda x: x.text[len(prefix):-1],
all_prefixes)
Did you mean to leave in this commented-out code?

http://codereview.chromium.org/7329005/diff/1/tools/bisect-builds.py#newcode252
tools/bisect-builds.py:252: def bisect(platform=None,
Capital B on function names.  (Our Python style is weird, I know.)

http://codereview.chromium.org/7329005/diff/1/tools/bisect-builds.py#newcode352
tools/bisect-builds.py:352: sys.exit(0)
Is this both moving the code and making changes to it?  It'd be easier to review
if you uploaded a change that just moved it, and then a second one that modifies
it.

[szager, 2011-07-14 19:23:03]

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

http://codereview.chromium.org/7329005/diff/1/tools/bisect-builds.py#newcode39
tools/bisect-builds.py:39: import threading
On 2011/07/14 18:57:52, Evan Martin wrote:
> I think this list is alphabetized.

Done.

http://codereview.chromium.org/7329005/diff/1/tools/bisect-builds.py#newcode168
tools/bisect-builds.py:168: #revisions = map(lambda x: x.text[len(prefix):-1],
all_prefixes)
On 2011/07/14 18:57:52, Evan Martin wrote:
> Did you mean to leave in this commented-out code?

Deleted!

http://codereview.chromium.org/7329005/diff/1/tools/bisect-builds.py#newcode252
tools/bisect-builds.py:252: def bisect(platform=None,
On 2011/07/14 18:57:52, Evan Martin wrote:
> Capital B on function names.  (Our Python style is weird, I know.)

Done.

http://codereview.chromium.org/7329005/diff/1/tools/bisect-builds.py#newcode352
tools/bisect-builds.py:352: sys.exit(0)
On 2011/07/14 18:57:52, Evan Martin wrote:
> Is this both moving the code and making changes to it?  It'd be easier to
review
> if you uploaded a change that just moved it, and then a second one that
modifies
> it.

I had move the main loop out of main() to enable the programmatic interface; and
I had to reorganize the main loop to enable threading.  I can probably split
those into two changes.

[Robert Sesek, 2011-07-15 16:27:14]

Is there a second patchset? I may have more comments once Evan's are addressed.

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

http://codereview.chromium.org/7329005/diff/1/tools/bisect-builds.py#newcode170
tools/bisect-builds.py:170: for pf in all_prefixes:
What's "pf"? |prefix| is fine.

[szager, 2011-07-15 18:35:13]

On Fri, Jul 15, 2011 at 9:27 AM, <rsesek@chromium.org> wrote:

> Is there a second patchset? I may have more comments once Evan's are
> addressed.


Not yet; I had to switch tasks yesterday.  Will probably work on this today.

Stefan

[szager, 2011-07-15 21:16:20]

On 2011/07/15 18:35:13, szager wrote:
> On Fri, Jul 15, 2011 at 9:27 AM, <mailto:rsesek@chromium.org> wrote:
> 
> > Is there a second patchset? I may have more comments once Evan's are
> > addressed.

OK, this patch set includes only the programmatic interface, but no threading.

Thanks,

Stefan

[Robert Sesek, 2011-07-15 21:41:43]

Thanks for splitting it out. This is much more manageable to review, now.

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

http://codereview.chromium.org/7329005/diff/5002/tools/bisect-builds.py#oldco...
tools/bisect-builds.py:16: BASE_URL =
'http://commondatastorage.googleapis.com/chromium-browser-continuous'
Why this change?

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

http://codereview.chromium.org/7329005/diff/5002/tools/bisect-builds.py#newco...
tools/bisect-builds.py:254: def Bisect(good,
This needs a docstring describing the params.

[szager, 2011-07-15 22:37:39]

Should I land this patch and do the threading in a separate one?

Thanks,

Stefan

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

http://codereview.chromium.org/7329005/diff/5002/tools/bisect-builds.py#oldco...
tools/bisect-builds.py:16: BASE_URL =
'http://commondatastorage.googleapis.com/chromium-browser-continuous'
On 2011/07/15 21:41:43, rsesek wrote:
> Why this change?

The 'continuous' store was a temporary hack; going forward, all full builds go
to the 'snapshot' store.

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

http://codereview.chromium.org/7329005/diff/5002/tools/bisect-builds.py#newco...
tools/bisect-builds.py:254: def Bisect(good,
On 2011/07/15 21:41:43, rsesek wrote:
> This needs a docstring describing the params.

Done.

[Robert Sesek, 2011-07-18 15:40:33]

Yes, please add the threading as a separate patch. CLs are best done in smaller,
logical units.

LGTM with a nit

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

http://codereview.chromium.org/7329005/diff/11001/tools/bisect-builds.py#newc...
tools/bisect-builds.py:264: @param good The index in revlist of the last known
good revision
nit: each of these should end with a full-stop

[Evan Martin, 2011-07-18 15:45:28]

(PS: please update the review description to describe whatever you land)