display "latest GM failures" link in its own line under build step

display "latest GM failures" link in its own line under build step

BUG=skia:2639

[epoger, 2014-06-05 22:15:36]

before:
http://c128.i.corp.google.com:10115/builders/Test-Ubuntu13.10-ShuttleA-NoGPU-...

after:
http://c128.i.corp.google.com:10115/builders/Test-Ubuntu13.10-ShuttleA-NoGPU-...

Personally, I prefer the "after", especially given that the client can specify
any number of links (including 0 links), depending on what happens when the
command is run.

What do you think, Eric?

https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/f...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/f...
master/skia_master_scripts/factory.py:542: 'Latest GM failures on this builder':
For more flexibility, we could even allow the client-side script to set up the
number of links, and the text label for each one...

We could define a meta-tag like this:
BUILDBOT_DISPLAY_LINK('link name','http://www.cnn.com')

And every time it appears in the stdout, we would add a link to the buildbot
page.

I guess we would still want build steps to have to "opt in" to get it; otherwise
we would have to run regex matches over the stdout of every build step, just in
case...

[borenet, 2014-06-06 12:10:34]

https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/f...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/f...
master/skia_master_scripts/factory.py:542: 'Latest GM failures on this builder':
On 2014/06/05 22:15:36, epoger wrote:
> For more flexibility, we could even allow the client-side script to set up the
> number of links, and the text label for each one...
> 
> We could define a meta-tag like this:
> BUILDBOT_DISPLAY_LINK('link name','http://www.cnn.com')
> 
> And every time it appears in the stdout, we would add a link to the buildbot
> page.
> 
> I guess we would still want build steps to have to "opt in" to get it;
otherwise
> we would have to run regex matches over the stdout of every build step, just
in
> case...

This makes me wish we had better performance measurements for the master.  My
inclination is to request that we always scan for links and properties and then
add functions to skia_slave_scripts/utils/misc.py:

SetBuildProperty(k, v)
SetLink(name, url)

Each of these would print to stdout in a predictable format like you suggested
above, and then we'd no longer need to do a master restart to set build
properties from a slave-side script.

https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/s...
File master/skia_master_scripts/skia_build_step.py (right):

https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/s...
master/skia_master_scripts/skia_build_step.py:67: stdout =
''.join(log.getChunks([STDOUT], onlyText=True))
This is probably a performance killer. Seems like it wouldn't be hard to rewrite
this, as long as we don't care about properties/links with multi-line regexp.

[epoger, 2014-06-09 13:33:29]

Thanks, Eric.  Good suggestions.

I think I already have a "good enough" solution for my particular case from
https://codereview.chromium.org/313203003 ('on CompareGMs failure, show link to
most recent results on this builder'), and we have plenty of higher-priority
items to work on, so my inclination is to just leave this CL sitting here for
potential future use.  I just had to get it out of my system. :-)

But if you think it's worth cleaning up the rough edges to commit something
here, let me know and we'll figure out what should go in.

https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/f...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/f...
master/skia_master_scripts/factory.py:542: 'Latest GM failures on this builder':
On 2014/06/06 12:10:34, borenet wrote:
> On 2014/06/05 22:15:36, epoger wrote:
> > For more flexibility, we could even allow the client-side script to set up
the
> > number of links, and the text label for each one...
> > 
> > We could define a meta-tag like this:
> > BUILDBOT_DISPLAY_LINK('link name','http://www.cnn.com')
> > 
> > And every time it appears in the stdout, we would add a link to the buildbot
> > page.
> > 
> > I guess we would still want build steps to have to "opt in" to get it;
> otherwise
> > we would have to run regex matches over the stdout of every build step, just
> in
> > case...
> 
> This makes me wish we had better performance measurements for the master.  My
> inclination is to request that we always scan for links and properties and
then
> add functions to skia_slave_scripts/utils/misc.py:
> 
> SetBuildProperty(k, v)
> SetLink(name, url)
> 
> Each of these would print to stdout in a predictable format like you suggested
> above, and then we'd no longer need to do a master restart to set build
> properties from a slave-side script.

I think those functions sound like a good idea.  Although I could imagine
wanting our native tools (gm and its ilk) to be able to trigger the links also,
and those tools wouldn't be able to call into skia_slave_scripts/utils/misc.py .
 Perhaps the format for the special notations could be shared between buildbot
Python code and C++ code within the skia repo.

At any rate, like you say, it would be awfully helpful to have better
performance measurements for the master before making such a change.  We know
that there are some steps that have ridiculously long stdio logs; I'm afraid
that parsing all of the logs by default could be problematic.

Also... there may be some way we could log these notations into a different
stream, or make them appear at the top of the stdio log and thus only parse the
first part of the log.

https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/s...
File master/skia_master_scripts/skia_build_step.py (right):

https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/s...
master/skia_master_scripts/skia_build_step.py:67: stdout =
''.join(log.getChunks([STDOUT], onlyText=True))
On 2014/06/06 12:10:34, borenet wrote:
> This is probably a performance killer. Seems like it wouldn't be hard to
rewrite
> this, as long as we don't care about properties/links with multi-line regexp.

I don't know how the chunks are split.  It would not be ideal to sometimes miss
an item because it straddled the borderline between two chunks.

[borenet, 2014-06-09 14:22:03]

On 2014/06/09 13:33:29, epoger wrote:
> Thanks, Eric.  Good suggestions.
> 
> I think I already have a "good enough" solution for my particular case from
> https://codereview.chromium.org/313203003 ('on CompareGMs failure, show link
to
> most recent results on this builder'), and we have plenty of higher-priority
> items to work on, so my inclination is to just leave this CL sitting here for
> potential future use.  I just had to get it out of my system. :-)
> 
> But if you think it's worth cleaning up the rough edges to commit something
> here, let me know and we'll figure out what should go in.
> 

Up to you.  Upstream code already has this figured out, so hopefully we won't
need this for much longer anyway.

>
https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/f...
> File master/skia_master_scripts/factory.py (right):
> 
>
https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/f...
> master/skia_master_scripts/factory.py:542: 'Latest GM failures on this
builder':
> On 2014/06/06 12:10:34, borenet wrote:
> > On 2014/06/05 22:15:36, epoger wrote:
> > > For more flexibility, we could even allow the client-side script to set up
> the
> > > number of links, and the text label for each one...
> > > 
> > > We could define a meta-tag like this:
> > > BUILDBOT_DISPLAY_LINK('link name','http://www.cnn.com')
> > > 
> > > And every time it appears in the stdout, we would add a link to the
buildbot
> > > page.
> > > 
> > > I guess we would still want build steps to have to "opt in" to get it;
> > otherwise
> > > we would have to run regex matches over the stdout of every build step,
just
> > in
> > > case...
> > 
> > This makes me wish we had better performance measurements for the master. 
My
> > inclination is to request that we always scan for links and properties and
> then
> > add functions to skia_slave_scripts/utils/misc.py:
> > 
> > SetBuildProperty(k, v)
> > SetLink(name, url)
> > 
> > Each of these would print to stdout in a predictable format like you
suggested
> > above, and then we'd no longer need to do a master restart to set build
> > properties from a slave-side script.
> 
> I think those functions sound like a good idea.  Although I could imagine
> wanting our native tools (gm and its ilk) to be able to trigger the links
also,
> and those tools wouldn't be able to call into skia_slave_scripts/utils/misc.py
.
>  Perhaps the format for the special notations could be shared between buildbot
> Python code and C++ code within the skia repo.
> 
> At any rate, like you say, it would be awfully helpful to have better
> performance measurements for the master before making such a change.  We know
> that there are some steps that have ridiculously long stdio logs; I'm afraid
> that parsing all of the logs by default could be problematic.
> 
> Also... there may be some way we could log these notations into a different
> stream, or make them appear at the top of the stdio log and thus only parse
the
> first part of the log.
> 
>
https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/s...
> File master/skia_master_scripts/skia_build_step.py (right):
> 
>
https://codereview.chromium.org/316333002/diff/1/master/skia_master_scripts/s...
> master/skia_master_scripts/skia_build_step.py:67: stdout =
> ''.join(log.getChunks([STDOUT], onlyText=True))
> On 2014/06/06 12:10:34, borenet wrote:
> > This is probably a performance killer. Seems like it wouldn't be hard to
> rewrite
> > this, as long as we don't care about properties/links with multi-line
regexp.
> 
> I don't know how the chunks are split.  It would not be ideal to sometimes
miss
> an item because it straddled the borderline between two chunks.

[borenet, 2014-09-26 17:04:46]

borenet@google.com changed reviewers:
- borenet@google.com