Added buildbot appengine frontend for chromium-build app

Added buildbot appengine frontend for chromium-build app

Added changes in app.yaml to point /buildbot/... to the buildlogparse.py file.
It currently runs as its own little mini-app.

BUG=

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

[agable, 2013-04-15 19:33:30]

Comments inline.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py
File buildlogparse.py (right):

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode25
buildlogparse.py:25: from google.appengine.api import mail
Please cleanup imports to be only what you actually use, and alphabetize them.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode107
buildlogparse.py:107: result += '%d hrs ' % hours
nit: add commas after hr/hrs, min/mins, and a period after sec/secs.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode160
buildlogparse.py:160: def render(template_filename):
Docstring, similar to the one for render_json below.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode170
buildlogparse.py:170: # The function is expected to return a dict, and we want
to render json.
Make this a real docstring.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode176
buildlogparse.py:176: def
return_json_if_flag_is_set_else_render(template_filename):
maybe_return_json?

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode218
buildlogparse.py:218: """Return 403 unless an admin is logged in"""
Give all of these docstrings periods -- they're sentences after all :)

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode231
buildlogparse.py:231: def expect_request(*request_args):
expect_request_param? expect_request sounds like it expects a request object,
rather than expecting that object to contain a specific url parameter.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode254
buildlogparse.py:254: return _decorator
All these wrappers are really nice and general. Once this first check-in has
happened, let's refactor these out into utils.py

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode261
buildlogparse.py:261: title = source
Remove this, title is never used.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode270
buildlogparse.py:270: test_match = re.search(r'\[ RUN      \]\s*([^() ]*)\s*',
line)
Here you're searching for [ RUN     ], while earlier in the REPLACEMENTS list
you were using &nbsp. Why?

Additionally, why not make REPLACEMENTS a dict of replacement type (e.g. RUN) ->
regex tuple. Then you could just reuse regexes from that dict here, instead of
having to define them again here just for the sake of searching.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode298
buildlogparse.py:298: elif re.search(r'\[\s*\d+/\d+\]\s*\d+\.\d+s\s+[\w/]+\.'
Document your regexes :)

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode307
buildlogparse.py:307: if len(line) > 160:
Why 160?

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode321
buildlogparse.py:321: lines.append((None, line, line_attr))
Can pull this duplicated code (line.replace; for foo in REPLACEMENTS;
lines.append) out of both branches of the if-else with line_abbr initialized to
None before the if.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode322
buildlogparse.py:322: return (title, lines)
Remove 'return title', it is identical to the input |source|.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode326
buildlogparse.py:326: """Prases a build step page."""
Parses.

He how prases the build step page.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode331
buildlogparse.py:331: self.redirect('/buildbot/')
See comment below about having url be a required url param.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode335
buildlogparse.py:335: url_m =
re.match(r'^/((p/)?)(.*)/builders/(.*)/builds/(\d+)$', path)
Offline comment about this (p/)? to follow.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode365
buildlogparse.py:365: @expect_request('url')
Having a *required* url parameter is kinda weird. You could make it slightly
more RESTful by changing the application handler to accept all urls of the form
"/buildbot/(*)" (placed after /buildbot/step/(*) for proper prioritizing) and
then whatever is contained in the (*) will be captured and passed to this get
method as an extra argument.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode366
buildlogparse.py:366: def get(self, url):
I'd reorder the steps this method performs for better readability and debugging:
1) redirect if necessary
2) check for cached result. If we have one, return it.
3) check for cached log. If we have it, use it. Else, fetch it and cache it.
4) Process log.
5) Cache result and return it.
WDYT?

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode378
buildlogparse.py:378: buildlog = buildlog_query.get()
377 and 378 can be one line.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode385
buildlogparse.py:385: all_output = re.findall(r'<span
class="(header|stdout)">(.*?)</span>',
Don't bother performing this regex unless the cached lookup failed.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode411
buildlogparse.py:411: if len(compressed_result) < 1000 * 1000:
Use 10**6

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode463
buildlogparse.py:463: }
Could cache the compressed version of this whole json dict rather than having to
do the same post-processing every time, even if the log is cached.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode474
buildlogparse.py:474: ('/buildbot/step/?', BuildStep),
See comments on MainPage and BuildStep get methods regarding the use of url
capture groups in this wsgi handler.

https://codereview.chromium.org/13892003/diff/1/static/css/bootstrap-responsi...
File static/css/bootstrap-responsive.css (right):

https://codereview.chromium.org/13892003/diff/1/static/css/bootstrap-responsi...
static/css/bootstrap-responsive.css:1: /*!
Do we need both the regular and .min versions of all the CSS?

https://codereview.chromium.org/13892003/diff/1/static/css/bootstrap-responsi...
static/css/bootstrap-responsive.css:9: */
I'm not up on all my licensing stuff. Make sure it's okay for us to include
files under Apache 2.0 in our work.

https://codereview.chromium.org/13892003/diff/1/static/css/geobootstrap.min.css
File static/css/geobootstrap.min.css (right):

https://codereview.chromium.org/13892003/diff/1/static/css/geobootstrap.min.c...
static/css/geobootstrap.min.css:1: /*!
Doesn't look like anything actually uses this file. Please clean up the
bootstrap files to only include those files we're actually using.

https://codereview.chromium.org/13892003/diff/1/templates/base.html
File templates/base.html (right):

https://codereview.chromium.org/13892003/diff/1/templates/base.html#newcode1
templates/base.html:1: <!DOCTYPE html>
Don't want to clobber the old base.html. Maybe put these three templates in a
templates/logparse/ subdirectory?

[Ryan Tseng, 2013-04-17 22:53:48]

Overview of changelist:
App.py:
* Substituted links in the original build-status app to point
build.chromium.org/p/* links over to /buildbot/*

App.yaml:
* Added endpoints to point at the new builtbot viewer proxy

buildlogparse.py (named when the scope of the script was only to parse the
buildbot logs):
* Defines REPLACEMENT pairs that are from->to regex strings.
* Endpoints for Builders, steps, slaves, logs.  The slaves endpoint is yet to be
finished (Missing "recently finished" builds because its not in the json).
* MainPage takes a "url" argument, and redirects to the appropriate page.
* Decorators (saved in utils.py) are used for readability and code cleaniness. 
They should be self explanatory.  If they're not self explanatory, then I'll
need to make them more self explanatory.
* BuildLog does a log of parsing, and a bit of caching.

static files:
* Bootstrap files are added in.

default/default-old.css:
* These are the css files pulled from buildbot.  This is because the buildbot
html is pulled from build.chromium.org, slightly modified, and rendered from
this site.  The CSS file is also served from this site.

buildbot.html:
* Landing page.  Just a webpage that says "enter buildbot url here, and I'll
redirect you to the correct page (paraphrased)"

logs.html:
* Displays buildbot logs

slave.html:
* Display a buildbot slave.  Currently incomplete because the json for the
slaves page doesn't contain "recently finished builds".

step.html:
* Shows a build.

viewer_base.html:
* Bootstrap base file

utils.py:
* Contains filters and decorators.  Names should be self explanatory, make a
note if any of filter/decorator names don't make sense.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py
File buildlogparse.py (right):

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode25
buildlogparse.py:25: from google.appengine.api import mail
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Please cleanup imports to be only what you actually use, and alphabetize them.

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode25
buildlogparse.py:25: from google.appengine.api import mail
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Please cleanup imports to be only what you actually use, and alphabetize them.

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode107
buildlogparse.py:107: result += '%d hrs ' % hours
On 2013/04/15 19:33:30, Aaron Gable wrote:
> nit: add commas after hr/hrs, min/mins, and a period after sec/secs.

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode160
buildlogparse.py:160: def render(template_filename):
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Docstring, similar to the one for render_json below.

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode170
buildlogparse.py:170: # The function is expected to return a dict, and we want
to render json.
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Make this a real docstring.

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode176
buildlogparse.py:176: def
return_json_if_flag_is_set_else_render(template_filename):
On 2013/04/15 19:33:30, Aaron Gable wrote:
> maybe_return_json?

Works

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode218
buildlogparse.py:218: """Return 403 unless an admin is logged in"""
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Give all of these docstrings periods -- they're sentences after all :)

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode231
buildlogparse.py:231: def expect_request(*request_args):
On 2013/04/15 19:33:30, Aaron Gable wrote:
> expect_request_param? expect_request sounds like it expects a request object,
> rather than expecting that object to contain a specific url parameter.

Works for me.  Done

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode254
buildlogparse.py:254: return _decorator
On 2013/04/15 19:33:30, Aaron Gable wrote:
> All these wrappers are really nice and general. Once this first check-in has
> happened, let's refactor these out into utils.py

Or I can do that now :)

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode261
buildlogparse.py:261: title = source
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Remove this, title is never used.

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode270
buildlogparse.py:270: test_match = re.search(r'\[ RUN      \]\s*([^() ]*)\s*',
line)
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Here you're searching for [ RUN     ], while earlier in the REPLACEMENTS list
> you were using &nbsp. Why?
> 
> Additionally, why not make REPLACEMENTS a dict of replacement type (e.g. RUN)
->
> regex tuple. Then you could just reuse regexes from that dict here, instead of
> having to define them again here just for the sake of searching.

This set of regex is a bit special in that its not purely line by line, but
requires some state to be held between lines to know if a line is part of a
running test or not.  Theres also the benefit that these sets of regexs are ran
before escaping, and don't require inserting html tags directly, which is why
they don't have  

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode298
buildlogparse.py:298: elif re.search(r'\[\s*\d+/\d+\]\s*\d+\.\d+s\s+[\w/]+\.'
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Document your regexes :)

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode307
buildlogparse.py:307: if len(line) > 160:
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Why 160?

That was arbitrary.  I think I'll remove this and do it in javascript later.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode321
buildlogparse.py:321: lines.append((None, line, line_attr))
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Can pull this duplicated code (line.replace; for foo in REPLACEMENTS;
> lines.append) out of both branches of the if-else with line_abbr initialized
to
> None before the if.

Removed line_abbr anyways.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode322
buildlogparse.py:322: return (title, lines)
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Remove 'return title', it is identical to the input |source|.

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode326
buildlogparse.py:326: """Prases a build step page."""
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Parses.
> 
> He how prases the build step page.

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode331
buildlogparse.py:331: self.redirect('/buildbot/')
On 2013/04/15 19:33:30, Aaron Gable wrote:
> See comment below about having url be a required url param.

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode335
buildlogparse.py:335: url_m =
re.match(r'^/((p/)?)(.*)/builders/(.*)/builds/(\d+)$', path)
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Offline comment about this (p/)? to follow.

?

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode365
buildlogparse.py:365: @expect_request('url')
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Having a *required* url parameter is kinda weird. You could make it slightly
> more RESTful by changing the application handler to accept all urls of the
form
> "/buildbot/(*)" (placed after /buildbot/step/(*) for proper prioritizing) and
> then whatever is contained in the (*) will be captured and passed to this get
> method as an extra argument.

Done.  MainPage now just parses the url and redirects to the correct page.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode366
buildlogparse.py:366: def get(self, url):
On 2013/04/15 19:33:30, Aaron Gable wrote:
> I'd reorder the steps this method performs for better readability and
debugging:
> 1) redirect if necessary
> 2) check for cached result. If we have one, return it.
> 3) check for cached log. If we have it, use it. Else, fetch it and cache it.
> 4) Process log.
> 5) Cache result and return it.
> WDYT?

Refactored to just do #1.  The rest has also been refactored to that order :)

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode378
buildlogparse.py:378: buildlog = buildlog_query.get()
On 2013/04/15 19:33:30, Aaron Gable wrote:
> 377 and 378 can be one line.

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode385
buildlogparse.py:385: all_output = re.findall(r'<span
class="(header|stdout)">(.*?)</span>',
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Don't bother performing this regex unless the cached lookup failed.

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode411
buildlogparse.py:411: if len(compressed_result) < 1000 * 1000:
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Use 10**6

Done.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode463
buildlogparse.py:463: }
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Could cache the compressed version of this whole json dict rather than having
to
> do the same post-processing every time, even if the log is cached.

done :) (That's what line 388/412 is)
Well, it caches the line-by-line processing, which is the heavyweight stuff. 
The cheaper stuff is still done on each request.

https://codereview.chromium.org/13892003/diff/1/buildlogparse.py#newcode474
buildlogparse.py:474: ('/buildbot/step/?', BuildStep),
On 2013/04/15 19:33:30, Aaron Gable wrote:
> See comments on MainPage and BuildStep get methods regarding the use of url
> capture groups in this wsgi handler.

Done.

https://codereview.chromium.org/13892003/diff/1/static/css/bootstrap-responsi...
File static/css/bootstrap-responsive.css (right):

https://codereview.chromium.org/13892003/diff/1/static/css/bootstrap-responsi...
static/css/bootstrap-responsive.css:1: /*!
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Do we need both the regular and .min versions of all the CSS?

Prolly not, I can remove this.

https://codereview.chromium.org/13892003/diff/1/static/css/bootstrap-responsi...
static/css/bootstrap-responsive.css:9: */
On 2013/04/15 19:33:30, Aaron Gable wrote:
> I'm not up on all my licensing stuff. Make sure it's okay for us to include
> files under Apache 2.0 in our work.

I think our only restriction is no GPL work, Apache/BSD/MIT are okay.

https://codereview.chromium.org/13892003/diff/1/static/css/geobootstrap.min.css
File static/css/geobootstrap.min.css (right):

https://codereview.chromium.org/13892003/diff/1/static/css/geobootstrap.min.c...
static/css/geobootstrap.min.css:1: /*!
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Doesn't look like anything actually uses this file. Please clean up the
> bootstrap files to only include those files we're actually using.

Oh yeah, I left the geocities skin in here.  I can remove it.... for now.

https://codereview.chromium.org/13892003/diff/1/templates/base.html
File templates/base.html (right):

https://codereview.chromium.org/13892003/diff/1/templates/base.html#newcode1
templates/base.html:1: <!DOCTYPE html>
On 2013/04/15 19:33:30, Aaron Gable wrote:
> Don't want to clobber the old base.html. Maybe put these three templates in a
> templates/logparse/ subdirectory?

Oops, I'll move it to viewer_base.html

[agable, 2013-04-23 18:18:17]

LGTM

https://codereview.chromium.org/13892003/diff/14001/templates/step.html
File templates/step.html (right):

https://codereview.chromium.org/13892003/diff/14001/templates/step.html#newco...
templates/step.html:20: This build <blink>failed</blink> on
Blink? Really?

[commit-bot: I haz the power, 2013-04-23 18:21:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hinoka@google.com/13892003/14001

[commit-bot: I haz the power, 2013-04-23 18:22:09]

Presubmit check for 13892003-14001 failed and returned exit status 1.

INFO:root:Found 19 file(s).
INFO:PRESUBMIT:Running pylint on 9 files
Running presubmit commit checks ...
Running /b/commit-queue/workdir/tools/chromium-build/PRESUBMIT.py

** Presubmit ERRORS **
nosetests failed!
Command nosetests --with-gae --gae-lib-root=/b/google_appengine returned
non-zero exit status 1
Traceback (most recent call last):
  File "/usr/local/bin/nosetests", line 5, in <module>
    from pkg_resources import load_entry_point
  File "/usr/lib/python2.7/dist-packages/pkg_resources.py", line 2707, in
<module>
    working_set.require(__requires__)
  File "/usr/lib/python2.7/dist-packages/pkg_resources.py", line 686, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/usr/lib/python2.7/dist-packages/pkg_resources.py", line 584, in resolve
    raise DistributionNotFound(req)
pkg_resources.DistributionNotFound: nose==1.1.2


Pylint (9 files) failed
************* Module app_test
F0401: 73,4:MainTestCase.test_main_page_redirect: Unable to import 'webtest'
F0401: 93,4:AppTestCase.test_app: Unable to import 'webtest'
F0401:104,4:AppTestCase.test_app_blob: Unable to import 'webtest'
F0401:115,4:AppTestCase.test_app_unicode: Unable to import 'webtest'
F0401:127,4:AppTestCase.test_app_unicode_blob: Unable to import 'webtest'
F0401:140,4:AppTestCase.test_app_cp1252: Unable to import 'webtest'
F0401:152,4:AppTestCase.test_app_cp1252_blob: Unable to import 'webtest'
F0401:166,4:AppTestCase.test_app_bogus_query_string: Unable to import 'webtest'
F0401:167,4:AppTestCase.test_app_bogus_query_string: Unable to import 'webtest'


Presubmit checks took 7.8s to calculate.

[Ryan Tseng, 2013-04-23 18:26:47]

Committed patchset #2 manually as r195849.

[Ryan Tseng, 2013-04-23 18:32:38]

wyk deploy chromium-build? :)

[agable, 2013-04-23 18:36:37]

Will do, probably this evening when it's not under as high load and I can
watch to make sure everything is shiny.


On Tue, Apr 23, 2013 at 11:32 AM, <hinoka@google.com> wrote:

> wyk deploy chromium-build? :)
>
>
https://chromiumcodereview.**appspot.com/13892003/<https://chromiumcodereview...
>