Trigger build request using git try instead of try_job_http on bisect bots

Trigger build request using git try instead of try_job_http on bisect bots

Bisect builders fail to apply patches sent via try_job_http,
because of this we were not able to build revisions with DEPS patches.
Now we are using "git try" instead to trigger build request on bisect builders.

BUG=411418
NOTRY=true

Committed: https://crrev.com/82f6f89aedce4fd05f782e8e9b0147c5cf781b08
Cr-Commit-Position: refs/heads/master@{#297918}

[prasadv, 2014-09-27 00:50:19]

prasadv@chromium.org changed reviewers:
+ qyearsley@chromium.org

[prasadv, 2014-09-27 00:50:20]

Hi, 
This cl is same as ttps://codereview.chromium.org/598283002/, because of moving
tools/bisect-perf-bisect-regression.py to
tools/auto_bisect/bisect_perf_bisect_regression.py I was unable rebase or merge
my changes.

I addressed most of the changes suggested by tonyg in this cl.

[qyearsley, 2014-09-27 01:45:34]

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:885: exception.
No need to return anything if the lack of an exception indicates success.

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:907: # run diff-index
Nit: should add a period at the end of the sentence.
Also, below we're running "git diff-index HEAD", not just "diff-index". What
does "git diff-index HEAD" do? I guess it outputs the changes that are staged
for commit but haven't yet been committed?

Maybe these few lines could be extracted to a IsGitTreeDirty function?

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:1449: except RunGitError, e:
The style guide suggests using "as", e.g. "except RunGitError as e".
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Excep...

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_uti...
File tools/auto_bisect/bisect_utils.py (right):

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_uti...
tools/auto_bisect/bisect_utils.py:336: """Checkouts master branch."""
"Checks out the master branch." or "Moves to the master branch."

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_uti...
tools/auto_bisect/bisect_utils.py:341: return False
As you noted, running "git rev-parse --abbrev-ref HEAD" is probably unnecessary
here because we're not using the result, and "git checkout --force" will work
even if we're already on the master branch.

Is there anything that could go wrong when running "git checkout --force
master"? If so, maybe we want to raise a RuntimeError from here rather than
returning False?

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_uti...
tools/auto_bisect/bisect_utils.py:431: 'bisect', 'src')
By the way, do you think it's a good idea to make "bisect" a global constant at
the top of this file?

It's not really related to this CL, but in general it's often a good idea to use
global constants rather than literal strings.

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_uti...
tools/auto_bisect/bisect_utils.py:433: cwd=path_to_dir)
Under what circumstances are we not "inside work tree"?

[prasadv, 2014-09-29 17:36:14]

PTAL

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:885: exception.
On 2014/09/27 01:45:33, qyearsley wrote:
> No need to return anything if the lack of an exception indicates success.

Done.

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:907: # run diff-index
Correct it lists if we have any uncommitted changes. 
Not sure, we want to extract a new method  IsGitTreeDirty(), since it is used
only here.
Let me know if you still want to it.

On 2014/09/27 01:45:33, qyearsley wrote:
> Nit: should add a period at the end of the sentence.
> Also, below we're running "git diff-index HEAD", not just "diff-index". What
> does "git diff-index HEAD" do? I guess it outputs the changes that are staged
> for commit but haven't yet been committed?
> 
> Maybe these few lines could be extracted to a IsGitTreeDirty function?

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_uti...
File tools/auto_bisect/bisect_utils.py (right):

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_uti...
tools/auto_bisect/bisect_utils.py:336: """Checkouts master branch."""
On 2014/09/27 01:45:34, qyearsley wrote:
> "Checks out the master branch." or "Moves to the master branch."

Done.

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_uti...
tools/auto_bisect/bisect_utils.py:341: return False
Actually we don't need this function, I removed this.
On 2014/09/27 01:45:34, qyearsley wrote:
> As you noted, running "git rev-parse --abbrev-ref HEAD" is probably
unnecessary
> here because we're not using the result, and "git checkout --force" will work
> even if we're already on the master branch.
> 
> Is there anything that could go wrong when running "git checkout --force
> master"? If so, maybe we want to raise a RuntimeError from here rather than
> returning False?

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_uti...
tools/auto_bisect/bisect_utils.py:431: 'bisect', 'src')
On 2014/09/27 01:45:34, qyearsley wrote:
> By the way, do you think it's a good idea to make "bisect" a global constant
at
> the top of this file?
> 
> It's not really related to this CL, but in general it's often a good idea to
use
> global constants rather than literal strings.

Done.

https://codereview.chromium.org/607803005/diff/1/tools/auto_bisect/bisect_uti...
tools/auto_bisect/bisect_utils.py:433: cwd=path_to_dir)
On 2014/09/27 01:45:33, qyearsley wrote:
> Under what circumstances are we not "inside work tree"?

This can happen when you are bisect folder but gclient client is not configure
for some reason. In such case we don't have to checkout master since doesn't
exists.

[prasadv, 2014-10-01 22:55:44]

Hi Quinten,
  I added unittest for bisect git try job, since this uses mock, I'd to modify
PRESUMIT.py and run_tests to set sys path acoordingly(basically same as you cl
https://codereview.chromium.org/548233002)

[prasadv, 2014-10-01 23:58:47]

PTAL

[qyearsley, 2014-10-02 01:07:35]

On 2014/10/01 23:58:47, prasadv wrote:
> PTAL

LGTM with nits.

[qyearsley, 2014-10-02 01:07:53]

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression.py:956: return True
Probably unnecessary to return something here; if you change this, remember to
change the docstring as well.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:355: 
Two newlines between top-level classes.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:365: def
_AssertRunGitExceptions(self, git_cmds, func, *args):
I think it would be helpful to explain the parameters to this funciton in a
docstring.

In particular, git_cmds is not just a list of commands, it's a sequence of pairs
of input and output, right?

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:366: def side_effect(values):
A name like git_cmd_args or something might be better than "values".

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:367: for val in git_cmds:
If the items you're iterating over are pairs, you can directly name the parts in
the for loop, for example:

for input, output in git_cmds:
  if input == values:
    return output

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:371:
self.assertRaises(bisect_perf_regression.RunGitError,
If you wanted to add another test method below that passes when no exceptions
are thrown, it might be easier if you take the "assertRaises" out of this
function (and put it into the sub-functions below) -- then the purpose of this
function is only to be a helper function for setting up the mock RunGit with
some canned inputs and outputs.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:378: cmds = (
This could also be a list (with square brackets) right? Then there's no need for
that comma before the closing parenthesis.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:379: (['rev-parse',
'--abbrev-ref', 'HEAD'], (None, 128)),)
I think indentation here should be 4 spaces.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:389: (['checkout', '-f',
parent_branch], ('Checkout Failed', 1)),)
Nit: inconsistent indentation.

[prasadv, 2014-10-02 17:27:27]

PTAL for final pass please

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression.py:956: return True
On 2014/10/02 01:07:53, qyearsley wrote:
> Probably unnecessary to return something here; if you change this, remember to
> change the docstring as well.

Done.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:355: 
On 2014/10/02 01:07:53, qyearsley wrote:
> Two newlines between top-level classes.

Done.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:365: def
_AssertRunGitExceptions(self, git_cmds, func, *args):
On 2014/10/02 01:07:53, qyearsley wrote:
> I think it would be helpful to explain the parameters to this funciton in a
> docstring.
> 
> In particular, git_cmds is not just a list of commands, it's a sequence of
pairs
> of input and output, right?

Done.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:366: def side_effect(values):
On 2014/10/02 01:07:53, qyearsley wrote:
> A name like git_cmd_args or something might be better than "values".

Done.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:367: for val in git_cmds:
On 2014/10/02 01:07:53, qyearsley wrote:
> If the items you're iterating over are pairs, you can directly name the parts
in
> the for loop, for example:
> 
> for input, output in git_cmds:
>   if input == values:
>     return ouput

"git_cmds" is a list of tuples with git commands and expected output.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:371:
self.assertRaises(bisect_perf_regression.RunGitError,
On 2014/10/02 01:07:53, qyearsley wrote:
> If you wanted to add another test method below that passes when no exceptions
> are thrown, it might be easier if you take the "assertRaises" out of this
> function (and put it into the sub-functions below) -- then the purpose of this
> function is only to be a helper function for setting up the mock RunGit with
> some canned inputs and outputs.

Done.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:378: cmds = (
On 2014/10/02 01:07:53, qyearsley wrote:
> This could also be a list (with square brackets) right? Then there's no need
for
> that comma before the closing parenthesis.

Done.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:379: (['rev-parse',
'--abbrev-ref', 'HEAD'], (None, 128)),)
On 2014/10/02 01:07:53, qyearsley wrote:
> I think indentation here should be 4 spaces.

Done.

https://codereview.chromium.org/607803005/diff/60001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:389: (['checkout', '-f',
parent_branch], ('Checkout Failed', 1)),)
On 2014/10/02 01:07:53, qyearsley wrote:
> Nit: inconsistent indentation.

Done.

[qyearsley, 2014-10-02 17:42:21]

On 2014/10/02 17:27:27, prasadv wrote:
> PTAL for final pass please
> 

LGTM -- not sure if Ojan or Tony want to take a look before submitting this
though?

[prasadv, 2014-10-02 21:45:15]

The CQ bit was checked by prasadv@chromium.org

[commit-bot: I haz the power, 2014-10-02 21:46:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/607803005/100001

[commit-bot: I haz the power, 2014-10-02 22:00:27]

Committed patchset #6 (id:100001) as a6b372e418ae6678a591b5ae09ad86ce59371f43

[commit-bot: I haz the power, 2014-10-02 22:01:24]

Patchset 6 (id:??) landed as
https://crrev.com/82f6f89aedce4fd05f782e8e9b0147c5cf781b08
Cr-Commit-Position: refs/heads/master@{#297918}