[bisect] - Parse DEPS file manually if execfile fails.

Parse DEPS file manually if execfile fails. Some DEPS files pull in a bunch of libraries, so execfile fails. If it does, we'll manually parse out revisions from the vars section.

NOTRY=true
BUG=366392
Bisect could not reproduce a change in the not/valid metric.

Confidence in Bisection Results: 0%

To reproduce locally:
$ echo android-chrome

Tested commits:
         Depot                                        Commit SHA                                    Mean       Std. Error       State    
     android-chrome                    0000000000000000000000000000000000000000                     1.00         +-0.00                  
        chromium                       0000000000000000000000000000000000000000                     0.00         +-0.00                  
        chromium         http://src.chromium.org/viewvc/chrome?view=revision&revision=265919        0.00         +-0.00                  
        chromium         http://src.chromium.org/viewvc/chrome?view=revision&revision=265917        0.00         +-0.00                  
        chromium         http://src.chromium.org/viewvc/chrome?view=revision&revision=265912        0.00         +-0.00                  
        chromium         http://src.chromium.org/viewvc/chrome?view=revision&revision=265904        0.00         +-0.00                  
     android-chrome                    0000000000000000000000000000000000000000                     0.00         +-0.00                  

Average build time : 0:00:00
Average test time  : 0:00:00

WARNINGS:
  !!! Due to build errors, regression range could not be narrowed down to a single commit.
  !!! Tests were only set to run once. This may be insufficient to get meaningful results.
  !!! Confidence is 0%. Try bisecting again on another platform, with increased repeat_count or on a sub-metric that shows the regression more clearly.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266322

[prasadv, 2014-04-24 22:24:06]

lgtm

[shatch, 2014-04-25 16:34:45]

The CQ bit was checked by simonhatch@chromium.org

[qyearsley, 2014-04-25 16:46:19]

https://codereview.chromium.org/256593004/diff/1/tools/bisect-perf-regression.py
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/256593004/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:1421: except ImportError:
This block (line 1422 to 1439) is a bit dense, and could perhaps be extracted to
be a separate function.

https://codereview.chromium.org/256593004/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:1423: rxp = re.compile("vars =
{(?P<revision>[^}]+)", re.MULTILINE)
<revision> isn't a very clear name for "everything inside of the curly braces in
the vars section". Maybe vars_body?

https://codereview.chromium.org/256593004/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:1426: if re_results:
To avoid nesting one level here, maybe:

if not re_results:
  return {}
vars_body = re_results.group("revision")
for current_line in vars_body.splitlines():
  ...

https://codereview.chromium.org/256593004/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:1430: depot_revision = current_line.split(":
'")[1].split("'")[0]
These two lines are particularly hard for me to understand. It may be clearer
with an example of what we're expecting current_line to look like?

[shatch, 2014-04-25 16:46:57]

The CQ bit was unchecked by simonhatch@chromium.org

[shatch, 2014-04-25 17:17:14]

New snapshot uploaded.

https://codereview.chromium.org/256593004/diff/1/tools/bisect-perf-regression.py
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/256593004/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:1421: except ImportError:
On 2014/04/25 16:46:19, qyearsley wrote:
> This block (line 1422 to 1439) is a bit dense, and could perhaps be extracted
to
> be a separate function.

Done.

https://codereview.chromium.org/256593004/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:1423: rxp = re.compile("vars =
{(?P<revision>[^}]+)", re.MULTILINE)
On 2014/04/25 16:46:19, qyearsley wrote:
> <revision> isn't a very clear name for "everything inside of the curly braces
in
> the vars section". Maybe vars_body?

Done.

https://codereview.chromium.org/256593004/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:1426: if re_results:
On 2014/04/25 16:46:19, qyearsley wrote:
> To avoid nesting one level here, maybe:
> 
> if not re_results:
>   return {}
> vars_body = re_results.group("revision")
> for current_line in vars_body.splitlines():
>   ...

Done.

https://codereview.chromium.org/256593004/diff/1/tools/bisect-perf-regression...
tools/bisect-perf-regression.py:1430: depot_revision = current_line.split(":
'")[1].split("'")[0]
On 2014/04/25 16:46:19, qyearsley wrote:
> These two lines are particularly hard for me to understand. It may be clearer
> with an example of what we're expecting current_line to look like?

Done.

[qyearsley, 2014-04-25 18:11:18]

https://codereview.chromium.org/256593004/diff/20001/tools/bisect-perf-regres...
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/256593004/diff/20001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:1392: # 'depot_name': 'revision',
I think this might not work for some .DEPS.git files, since in some cases the
depot name and revision are split up onto different lines, like:

vars = {
    'ffmpeg_hash':
         '@ac4a9f31fe2610bd146857bbd55d7a260003a888',
    'webkit_url':
         'https://chromium.googlesource.com/chromium/blink.git',
    'git_url':
         'https://chromium.googlesource.com',
    'webkit_rev':
         '@e01ac0a267d1017288bc67fa3c366b10469d8a24',
    'angle_revision':
         '74697cf2064c0a2c0d7e1b1b28db439286766a05'
}

https://code.google.com/p/chromium/codesearch#chromium/src/.DEPS.git

Will this still work in that case? Maybe in that case we'd need to do something
else, like use a regex to match each depot_name / depot_revision pair even if
there's a newline between them?

[shatch, 2014-04-25 20:24:16]

New snapshot uploaded.

https://codereview.chromium.org/256593004/diff/20001/tools/bisect-perf-regres...
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/256593004/diff/20001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:1392: # 'depot_name': 'revision',
True, changed it to use a regex to match up name/revision pairs.

On 2014/04/25 18:11:18, qyearsley wrote:
> I think this might not work for some .DEPS.git files, since in some cases the
> depot name and revision are split up onto different lines, like:
> 
> vars = {
>     'ffmpeg_hash':
>          '@ac4a9f31fe2610bd146857bbd55d7a260003a888',
>     'webkit_url':
>          'https://chromium.googlesource.com/chromium/blink.git',
>     'git_url':
>          'https://chromium.googlesource.com',
>     'webkit_rev':
>          '@e01ac0a267d1017288bc67fa3c366b10469d8a24',
>     'angle_revision':
>          '74697cf2064c0a2c0d7e1b1b28db439286766a05'
> }
> 
> https://code.google.com/p/chromium/codesearch#chromium/src/.DEPS.git
> 
> Will this still work in that case? Maybe in that case we'd need to do
something
> else, like use a regex to match each depot_name / depot_revision pair even if
> there's a newline between them?

[qyearsley, 2014-04-25 20:53:29]

Looks good -- I would slightly more confident if there were a unit test for the
_ParseRevisionsFromDEPSFileManually method in BisectPerformanceMetrics.

Probably wouldn't be a very long test method... also might be easier to write
the test method if _ParseRevisionsFromDEPSFileManually took the DEPS file
contents string, instead of reading it in.

BTW, what happens if there's an IO error in
  ReadStringFromFile(bisect_utils.FILE_DEPS_GIT)
? Maybe that's caught somewhere else?

https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:1391: vars_body = re_results.group("vars_body")
Line 1381 and 1391 -- single quotes preferred over double quotes for
consistency.

https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:1393: re.MULTILINE)
Style nit: line break style.
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Indent...

https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:1457: except ImportError:
This ImportError is thrown on line 1419 (in execfile) when the DEPS file
contains an import statement, right?

Would it be more complicated if we just always "manually" parsed the DEPS file,
even if the DEPS file doesn't contain import statements?

[shatch, 2014-04-25 22:01:40]

New snapshot uploaded.

Added a test for DEPS file parsing, and moved the filtering of depots the bisect
tool is interested in outside of _ParseRevisionsFromDEPSFileManually.

https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
File tools/bisect-perf-regression.py (right):

https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:743: bisect_utils.RunGClient(['runhooks'])
Unnecessary call, and ends up running when we run the bisect unit tests as well.

https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:1391: vars_body = re_results.group("vars_body")
On 2014/04/25 20:53:30, qyearsley wrote:
> Line 1381 and 1391 -- single quotes preferred over double quotes for
> consistency.

Done.

https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:1393: re.MULTILINE)
On 2014/04/25 20:53:30, qyearsley wrote:
> Style nit: line break style.
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Indent...

Done.

https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
tools/bisect-perf-regression.py:1457: except ImportError:
Yep.

I'm thinking it would be a lot more complicated to manually parse it. If you
want to go that route, might be better as a follow up patch later on so that we
can get android-chrome folks on their feet in the meantime.

On 2014/04/25 20:53:30, qyearsley wrote:
> This ImportError is thrown on line 1419 (in execfile) when the DEPS file
> contains an import statement, right?
> 
> Would it be more complicated if we just always "manually" parsed the DEPS
file,
> even if the DEPS file doesn't contain import statements?

[qyearsley, 2014-04-25 22:41:57]

On 2014/04/25 22:01:40, shatch wrote:
> New snapshot uploaded.
> 
> Added a test for DEPS file parsing, and moved the filtering of depots the
bisect
> tool is interested in outside of _ParseRevisionsFromDEPSFileManually.
> 
>
https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
> File tools/bisect-perf-regression.py (right):
> 
>
https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
> tools/bisect-perf-regression.py:743: bisect_utils.RunGClient(['runhooks'])
> Unnecessary call, and ends up running when we run the bisect unit tests as
well.
> 
>
https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
> tools/bisect-perf-regression.py:1391: vars_body =
re_results.group("vars_body")
> On 2014/04/25 20:53:30, qyearsley wrote:
> > Line 1381 and 1391 -- single quotes preferred over double quotes for
> > consistency.
> 
> Done.
> 
>
https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
> tools/bisect-perf-regression.py:1393: re.MULTILINE)
> On 2014/04/25 20:53:30, qyearsley wrote:
> > Style nit: line break style.
> >
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Indent...
> 
> Done.
> 
>
https://codereview.chromium.org/256593004/diff/40001/tools/bisect-perf-regres...
> tools/bisect-perf-regression.py:1457: except ImportError:
> Yep.
> 
> I'm thinking it would be a lot more complicated to manually parse it. If you
> want to go that route, might be better as a follow up patch later on so that
we
> can get android-chrome folks on their feet in the meantime.
> 
> On 2014/04/25 20:53:30, qyearsley wrote:
> > This ImportError is thrown on line 1419 (in execfile) when the DEPS file
> > contains an import statement, right?
> > 
> > Would it be more complicated if we just always "manually" parsed the DEPS
> file,
> > even if the DEPS file doesn't contain import statements?

LGTM :-)

[shatch, 2014-04-25 23:07:29]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-04-25 23:15:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/256593004/60001

[commit-bot: I haz the power, 2014-04-26 02:20:33]

Change committed as 266322