Make allow_subannotations more robust.

Make allow_subannotations more robust.

This patch makes the recipe engine parse and re-emit subannotations when allow_subannotations is True.  This means that any of the subannotator's steps which are duplicates of other steps executed will be disambiguated.  When an allow_subannotations step is actually emits annotations, an (end) step is emitted after it is done to have something to hang metadata on (it also conveniently communicates when we re-enter recipes).

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

[luqui, 2015-03-14 04:48:03]

luqui@chromium.org changed reviewers:
+ iannucci@chromium.org

[luqui, 2015-03-14 04:48:03]

ptal

Remaining issues:

* Running expectations, many tests added (start) and (end) versions because
apparently they allow subannotations. This may increase waterfall noise... what
do you think of this  approach?  I didn't include the expectations so far for
brevity (those were the only changes), but I know that many of those steps do
not emit subannotations.

* The non-simulation-test kernel is nontrivial, with threads calling callbacks
and junk.  It looks like we don't have any existing testing of the kernel,
correct?  Will you help me devise a testing strategy?

[luqui, 2015-03-23 23:26:33]

+ offline comments

https://codereview.chromium.org/1001183002/diff/80001/scripts/common/annotato...
File scripts/common/annotator.py (right):

https://codereview.chromium.org/1001183002/diff/80001/scripts/common/annotato...
scripts/common/annotator.py:466: def run_step(stream, name, cmd,
This function has no business being in this file.

https://codereview.chromium.org/1001183002/diff/80001/scripts/slave/annotated...
File scripts/slave/annotated_run.py (right):

https://codereview.chromium.org/1001183002/diff/80001/scripts/slave/annotated...
scripts/slave/annotated_run.py:644: start_step = _merge(step, { 'name':
step['name'] + ' (start)' })
Just emit name.  Only emit (end) step if any subannotations were emitted.

[luqui, 2015-03-27 22:46:58]

ptal

https://codereview.chromium.org/1001183002/diff/80001/scripts/common/annotato...
File scripts/common/annotator.py (right):

https://codereview.chromium.org/1001183002/diff/80001/scripts/common/annotato...
scripts/common/annotator.py:466: def run_step(stream, name, cmd,
On 2015/03/23 23:26:32, luqui wrote:
> This function has no business being in this file.

Not actually true, it's called by run_steps, which is called by main.  I don't
feel like digging deeper than that...

[iannucci, 2015-04-03 20:46:00]

lgtm. probably want to PSA the sherriff/troopers +blink +v8 before this lands,
and be ready to revert for ~4 hours after that.

https://codereview.chromium.org/1001183002/diff/100001/scripts/common/annotat...
File scripts/common/annotator.py (right):

https://codereview.chromium.org/1001183002/diff/100001/scripts/common/annotat...
scripts/common/annotator.py:543: def filter_lines(inhandle, outhandle):
yay closures

https://codereview.chromium.org/1001183002/diff/100001/scripts/common/annotat...
scripts/common/annotator.py:551: # The subannotator might write to the handle,
thus the lock.
where does subannotator get bound to outhandle?

https://codereview.chromium.org/1001183002/diff/100001/scripts/common/annotat...
scripts/common/annotator.py:615: if build_failure and not step.get('always_run',
False):
this function isn't used for anything... it should just be removed (maybe in a
prior CL?)

https://codereview.chromium.org/1001183002/diff/100001/scripts/slave/annotate...
File scripts/slave/annotated_run.py (right):

https://codereview.chromium.org/1001183002/diff/100001/scripts/slave/annotate...
scripts/slave/annotated_run.py:566: # SequentialRecipeEngine self.
is that a good idea? Can we just hold a presentation object in here and then
assign it after the completion of the step? Then this could be defined at the
module level instead of inside this function.

edit: oh, it's because we need to support BUILD_STEP. bleh.

P.S. BUILD_STEP is deprecated... Should really switch to STEP_STARTED instead.

https://codereview.chromium.org/1001183002/diff/100001/scripts/slave/annotate...
scripts/slave/annotated_run.py:569: # TODO(luqui): disambiguate
still a todo?

https://codereview.chromium.org/1001183002/diff/100001/scripts/slave/annotate...
scripts/slave/annotated_run.py:611: # Warning: run_step cah change the current
self._annotation and
typo cah?

https://codereview.chromium.org/1001183002/diff/100001/scripts/slave/annotate...
scripts/slave/annotated_run.py:621: self._step_result.step['name'])
it was a bandaid to try to fix subannoations from crashing the master, iirc.
Check git blame.

[luqui, 2015-04-07 23:26:54]

I'm planning on landing this around noon tomorrow

https://codereview.chromium.org/1001183002/diff/100001/scripts/common/annotat...
File scripts/common/annotator.py (right):

https://codereview.chromium.org/1001183002/diff/100001/scripts/common/annotat...
scripts/common/annotator.py:543: def filter_lines(inhandle, outhandle):
On 2015/04/03 20:45:59, iannucci wrote:
> yay closures

Acknowledged.

https://codereview.chromium.org/1001183002/diff/100001/scripts/common/annotat...
scripts/common/annotator.py:551: # The subannotator might write to the handle,
thus the lock.
On 2015/04/03 20:45:59, iannucci wrote:
> where does subannotator get bound to outhandle?

outhandle can be sys.stdout, which is written to by _open_step in annotated_run

https://codereview.chromium.org/1001183002/diff/100001/scripts/common/annotat...
scripts/common/annotator.py:615: if build_failure and not step.get('always_run',
False):
On 2015/04/03 20:45:59, iannucci wrote:
> this function isn't used for anything... it should just be removed (maybe in a
> prior CL?)

A bit of code removal and unittest changes to do that.  I'll ship it off
separately (I hope?)

https://codereview.chromium.org/1001183002/diff/100001/scripts/slave/annotate...
File scripts/slave/annotated_run.py (right):

https://codereview.chromium.org/1001183002/diff/100001/scripts/slave/annotate...
scripts/slave/annotated_run.py:566: # SequentialRecipeEngine self.
On 2015/04/03 20:46:00, iannucci wrote:
> is that a good idea? Can we just hold a presentation object in here and then
> assign it after the completion of the step? Then this could be defined at the
> module level instead of inside this function.
> 
> edit: oh, it's because we need to support BUILD_STEP. bleh.
> 
> P.S. BUILD_STEP is deprecated... Should really switch to STEP_STARTED instead.

Yeah I noticed that. The script I'm trying support uses it though, and we need
it from the past.

https://codereview.chromium.org/1001183002/diff/100001/scripts/slave/annotate...
scripts/slave/annotated_run.py:569: # TODO(luqui): disambiguate
On 2015/04/03 20:46:00, iannucci wrote:
> still a todo?

Done.

https://codereview.chromium.org/1001183002/diff/100001/scripts/slave/annotate...
scripts/slave/annotated_run.py:611: # Warning: run_step cah change the current
self._annotation and
On 2015/04/03 20:46:00, iannucci wrote:
> typo cah?

Done.

[luqui, 2015-04-08 19:14:42]

The CQ bit was checked by luqui@chromium.org

[luqui, 2015-04-08 19:14:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/1001183002/#ps120001
(title: "fixnits")

[commit-bot: I haz the power, 2015-04-08 19:15:02]

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

[commit-bot: I haz the power, 2015-04-08 19:19:07]

Committed patchset #7 (id:120001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=294727

[luqui, 2015-04-08 20:57:21]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1071733003/ by luqui@chromium.org.

The reason for reverting is: Maybe it broke extract_build.