[Findit] Collect failed output nodes of compile and link failures by strict regex

[Findit] Collect failed output nodes of compile and link failures by strict regex

This is a temporary solution before the ninja 1.7 is released and used by all bots.
This worth a try as it speeds up the culprit-finding try-jobs so much.

BUG=592808
Committed: https://chromium.googlesource.com/infra/infra/+/8f5d6b5fbb6086297bb8bbb26b6f7392766ba53b

[stgao, 2016-03-15 00:33:21]

stgao@chromium.org changed reviewers:
+ chanli@chromium.org, lijeffrey@chromium.org

[stgao, 2016-03-15 00:33:21]

PTAL.

BTW, I've reviewed the CL by myself yet, so there might be still some style
nits.

[chanli, 2016-03-15 06:07:49]

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/extractors.py (right):

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/extractors.py:134: STRICT_COMPILE_FAILURE_PATTEN =
re.compile(
nit: It seems that the first 2 lines of compile pattern are the same as link
pattern, maybe we can share them?

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/extractors.py:138: '-c ([^\s-]+(?:cc|c|cpp)) -o
(obj/[^\s-]+\.o)$'
I think this one can be simplified a little as :
r'^FAILED: \S*/b/build/goma/gomacc '
 '\S*third_party/llvm-build/Release\+Asserts/bin/clang\+\+ '
'-MMD -MF (obj/[^\s-]+\.o)\.d .* '
 '-c ([^\s-]+(?:cc|c|cpp)) -o \1$'

And naturally if  'dep_obj_name' and 'target' are different, there will be no
match. So you don't need the comparison as well.

The only thing is it'll just be no match so you can't really log error.

WDYT?

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/extractors.py:243:
self.ExtractFailedOutputNodes(line, signal)
What if strict_regex is true but the line doesn't match any pattern?

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/test/extractors_test.py (right):

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/test/extractors_test.py:557:
Mocked_EnableStrictRegexForCompileLinkFailures)
nit: we can reuse this mock from  testCompileStepStrictRegexForCompileFailures

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/waterfall_config.py (right):

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/waterfall_config.py:133: def
EnableStrictRegexForCompileLinkFailures(wf_mastername, wf_buildername):
Question: the ninja change will be done one builder by another?

[lijeffrey, 2016-03-15 07:12:26]

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/extractors.py (right):

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/extractors.py:141: STRICT_LINK_FAILURE_PATTEN =
re.compile(
I'm thinking, would it make sense to make these part of config too for a future
CL? In case the output changes or we find some cases where these regexes fail to
determine the target (which has happened to me before with mac) it may be useful
to be able to adjust them.

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/try_job_util.py (right):

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/try_job_util.py:132: if not
source_target.get('source'):
nit: I think this if can be simplified to

if not source_target.get('source') or strict_regex:
  compile_targets.append(source_target.get('target'))

[stgao, 2016-03-15 18:32:21]

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/extractors.py (right):

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/extractors.py:134: STRICT_COMPILE_FAILURE_PATTEN =
re.compile(
On 2016/03/15 06:07:49, chanli wrote:
> nit: It seems that the first 2 lines of compile pattern are the same as link
> pattern, maybe we can share them?

Done.

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/extractors.py:138: '-c ([^\s-]+(?:cc|c|cpp)) -o
(obj/[^\s-]+\.o)$'
On 2016/03/15 06:07:49, chanli wrote:
> I think this one can be simplified a little as :
> r'^FAILED: \S*/b/build/goma/gomacc '
>  '\S*third_party/llvm-build/Release\+Asserts/bin/clang\+\+ '
> '-MMD -MF (obj/[^\s-]+\.o)\.d .* '
>  '-c ([^\s-]+(?:cc|c|cpp)) -o \1$'
> 
> And naturally if  'dep_obj_name' and 'target' are different, there will be no
> match. So you don't need the comparison as well.
> 
> The only thing is it'll just be no match so you can't really log error.
> 
> WDYT?

Actually, I went with the \number approach before falling back to this one,
because it doesn't work for me.

Does it work for you? Or maybe my regex has some subtle bug with the \1?

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/extractors.py:141: STRICT_LINK_FAILURE_PATTEN =
re.compile(
On 2016/03/15 07:12:26, lijeffrey wrote:
> I'm thinking, would it make sense to make these part of config too for a
future
> CL? In case the output changes or we find some cases where these regexes fail
to
> determine the target (which has happened to me before with mac) it may be
useful
> to be able to adjust them.

I'd not make them into the config. Because this is a temporary solution before
the next ninja release. Once ninja 1.7 is released, this CL will be reverted and
all such info will be from ninja directly through a build property in the build
json data.

And since this speeds up the try-job so much, I think the temporary solution
still worths it.

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/extractors.py:243:
self.ExtractFailedOutputNodes(line, signal)
On 2016/03/15 06:07:49, chanli wrote:
> What if strict_regex is true but the line doesn't match any pattern?

Then we ignore and let "analyze" to figure out what to compile.
Because we are not 100% sure which "target" failed in that case.

With the strict regex, we know for sure which "target" failed.

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/test/extractors_test.py (right):

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/test/extractors_test.py:557:
Mocked_EnableStrictRegexForCompileLinkFailures)
On 2016/03/15 06:07:49, chanli wrote:
> nit: we can reuse this mock from  testCompileStepStrictRegexForCompileFailures

Done.

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/try_job_util.py (right):

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/try_job_util.py:132: if not
source_target.get('source'):
On 2016/03/15 07:12:26, lijeffrey wrote:
> nit: I think this if can be simplified to
> 
> if not source_target.get('source') or strict_regex:
>   compile_targets.append(source_target.get('target'))

sg. Done.

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/waterfall_config.py (right):

https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/waterfall_config.py:133: def
EnableStrictRegexForCompileLinkFailures(wf_mastername, wf_buildername):
On 2016/03/15 06:07:49, chanli wrote:
> Question: the ninja change will be done one builder by another?

No. The ninja change will be on recipe side on the waterfall bots. Once that is
done, we can deprecate this CL.
However, we need to wait for the next ninja release for that change to take
effect.

Since compiling the object files could speed up the culprit finding so much,
this CL will help a lot before the ninja release is available on the waterfall
bots.

[stgao, 2016-03-15 19:07:37]

Patchset #2 (id:20001) has been deleted

[stgao, 2016-03-15 19:09:47]

Description was changed from

==========
[Findit] Make regex more strict for compile and link failures.

BUG=
==========

to

==========
[Findit] Use strict regex to extract failed output nodes of compile and link
failures.

This is a temporary solution before the ninja 1.7 is released and used by all
bots.
BUG=592808
==========

[stgao, 2016-03-15 19:14:52]

Description was changed from

==========
[Findit] Use strict regex to extract failed output nodes of compile and link
failures.

This is a temporary solution before the ninja 1.7 is released and used by all
bots.
BUG=592808
==========

to

==========
[Findit] Collect failed output nodes of compile and link failures by strict
regex

This is a temporary solution before the ninja 1.7 is released and used by all
bots.

BUG=592808
==========

[stgao, 2016-03-15 19:15:53]

Description was changed from

==========
[Findit] Collect failed output nodes of compile and link failures by strict
regex

This is a temporary solution before the ninja 1.7 is released and used by all
bots.

BUG=592808
==========

to

==========
[Findit] Collect failed output nodes of compile and link failures by strict
regex

This is a temporary solution before the ninja 1.7 is released and used by all
bots.
This worth a try as it speeds up the culprit-finding try-jobs so much.

BUG=592808
==========

[chanli, 2016-03-15 20:10:12]

On 2016/03/15 18:32:21, stgao wrote:
>
https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
> File appengine/findit/waterfall/extractors.py (right):
> 
>
https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
> appengine/findit/waterfall/extractors.py:134: STRICT_COMPILE_FAILURE_PATTEN =
> re.compile(
> On 2016/03/15 06:07:49, chanli wrote:
> > nit: It seems that the first 2 lines of compile pattern are the same as link
> > pattern, maybe we can share them?
> 
> Done.
> 
>
https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
> appengine/findit/waterfall/extractors.py:138: '-c ([^\s-]+(?:cc|c|cpp)) -o
> (obj/[^\s-]+\.o)$'
> On 2016/03/15 06:07:49, chanli wrote:
> > I think this one can be simplified a little as :
> > r'^FAILED: \S*/b/build/goma/gomacc '
> >  '\S*third_party/llvm-build/Release\+Asserts/bin/clang\+\+ '
> > '-MMD -MF (obj/[^\s-]+\.o)\.d .* '
> >  '-c ([^\s-]+(?:cc|c|cpp)) -o \1$'
> > 
> > And naturally if  'dep_obj_name' and 'target' are different, there will be
no
> > match. So you don't need the comparison as well.
> > 
> > The only thing is it'll just be no match so you can't really log error.
> > 
> > WDYT?
> 
> Actually, I went with the \number approach before falling back to this one,
> because it doesn't work for me.
> 
> Does it work for you? Or maybe my regex has some subtle bug with the \1?
> 
>
https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
> appengine/findit/waterfall/extractors.py:141: STRICT_LINK_FAILURE_PATTEN =
> re.compile(
> On 2016/03/15 07:12:26, lijeffrey wrote:
> > I'm thinking, would it make sense to make these part of config too for a
> future
> > CL? In case the output changes or we find some cases where these regexes
fail
> to
> > determine the target (which has happened to me before with mac) it may be
> useful
> > to be able to adjust them.
> 
> I'd not make them into the config. Because this is a temporary solution before
> the next ninja release. Once ninja 1.7 is released, this CL will be reverted
and
> all such info will be from ninja directly through a build property in the
build
> json data.
> 
> And since this speeds up the try-job so much, I think the temporary solution
> still worths it.
> 
>
https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
> appengine/findit/waterfall/extractors.py:243:
> self.ExtractFailedOutputNodes(line, signal)
> On 2016/03/15 06:07:49, chanli wrote:
> > What if strict_regex is true but the line doesn't match any pattern?
> 
> Then we ignore and let "analyze" to figure out what to compile.
> Because we are not 100% sure which "target" failed in that case.
> 
> With the strict regex, we know for sure which "target" failed.
> 
>
https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
> File appengine/findit/waterfall/test/extractors_test.py (right):
> 
>
https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
> appengine/findit/waterfall/test/extractors_test.py:557:
> Mocked_EnableStrictRegexForCompileLinkFailures)
> On 2016/03/15 06:07:49, chanli wrote:
> > nit: we can reuse this mock from 
testCompileStepStrictRegexForCompileFailures
> 
> Done.
> 
>
https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
> File appengine/findit/waterfall/try_job_util.py (right):
> 
>
https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
> appengine/findit/waterfall/try_job_util.py:132: if not
> source_target.get('source'):
> On 2016/03/15 07:12:26, lijeffrey wrote:
> > nit: I think this if can be simplified to
> > 
> > if not source_target.get('source') or strict_regex:
> >   compile_targets.append(source_target.get('target'))
> 
> sg. Done.
> 
>
https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
> File appengine/findit/waterfall/waterfall_config.py (right):
> 
>
https://codereview.chromium.org/1799313002/diff/1/appengine/findit/waterfall/...
> appengine/findit/waterfall/waterfall_config.py:133: def
> EnableStrictRegexForCompileLinkFailures(wf_mastername, wf_buildername):
> On 2016/03/15 06:07:49, chanli wrote:
> > Question: the ninja change will be done one builder by another?
> 
> No. The ninja change will be on recipe side on the waterfall bots. Once that
is
> done, we can deprecate this CL.
> However, we need to wait for the next ninja release for that change to take
> effect.
> 
> Since compiling the object files could speed up the culprit finding so much,
> this CL will help a lot before the ninja release is available on the waterfall
> bots.

lgtm.

[stgao, 2016-03-15 20:20:16]

The CQ bit was checked by stgao@chromium.org

[commit-bot: I haz the power, 2016-03-15 20:20:18]

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

[commit-bot: I haz the power, 2016-03-15 20:23:30]

Description was changed from

==========
[Findit] Collect failed output nodes of compile and link failures by strict
regex

This is a temporary solution before the ninja 1.7 is released and used by all
bots.
This worth a try as it speeds up the culprit-finding try-jobs so much.

BUG=592808
==========

to

==========
[Findit] Collect failed output nodes of compile and link failures by strict
regex

This is a temporary solution before the ninja 1.7 is released and used by all
bots.
This worth a try as it speeds up the culprit-finding try-jobs so much.

BUG=592808

Committed:
https://chromium.googlesource.com/infra/infra/+/8f5d6b5fbb6086297bb8bbb26b6f7...
==========

[commit-bot: I haz the power, 2016-03-15 20:23:31]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/infra/infra/+/8f5d6b5fbb6086297bb8bbb26b6f7...

[stgao, 2016-03-15 23:27:34]

Patchset #4 (id:80001) has been deleted

[stgao, 2016-03-15 23:32:40]

Patchset #3 (id:60001) has been deleted