rebaseline_server: make --reload work in git checkout

rebaseline_server: make --reload work in git checkout
(SkipBuildbotRuns)

R=rmistry@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12397

[epoger, 2013-11-25 19:43:47]

Ready for review at patchset 1.

https://codereview.chromium.org/86343002/diff/1/gm/rebaseline_server/server.py
File gm/rebaseline_server/server.py (right):

https://codereview.chromium.org/86343002/diff/1/gm/rebaseline_server/server.p...
gm/rebaseline_server/server.py:144: # Reentrant lock that must be held whenever
updating EITHER of:
We didn't need this lock before, because the self._expectations_repo object had
its own lock.  But now that we're calling "gclient sync", we need to make sure
we don't have two updates running simultaneously.

[epoger, 2013-11-25 19:48:22]

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
File gm/rebaseline_server/server.py (right):

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:477: 'Skia checkout will be updated, by running '
Patchset 2: just added a bit more to the help message.  Still ready for review.

[rmistry, 2013-11-26 12:25:17]

Overall it looks good, just a few comments.

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
File gm/rebaseline_server/server.py (right):

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:1: #!/usr/bin/python
The CL description says make --update work in git checkout. Did you mean make
--reload work?

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:90: 
Nit: There should be two newlines between top level functions and classes.

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:158: def reload_seconds(self):
Make this a property so that you can refer to it with SERVER.reload_seconds

@property
def reload_seconds(self):
  ...

Could make is_exported and is_editable properties as well.

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:197: self.results = results.Results(
I recommend making self._results that is None in the constructor with a comment
saying it will be set when update_results is called successfully.

Then making a
@property
def results(self):
  return self._results

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:206: while self._reload_seconds:
If we only start the thread when reload_seconds is specified this can be while
True.
while self._reload_seconds was a little confusing to me because it makes it seem
like it is getting decremented somewhere.

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:212:
thread.start_new_thread(self._result_reloader, ())
Maybe we should not even bother starting a new thread if self._reload_seconds is
0.

if self._result_reloader:
  thread.start_new_thread(self._result_reloader, ())

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:476: 'results will be updated.  In fact, your
entire '
Could reword this by saying something like:
If specified, both expected and actual results will be updated by running
gclient sync on the Skia checkout. By default, ...

[epoger, 2013-11-26 17:04:35]

Thanks, Ravi!  Please see patchset 3.

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
File gm/rebaseline_server/server.py (right):

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:1: #!/usr/bin/python
On 2013/11/26 12:25:17, rmistry wrote:
> The CL description says make --update work in git checkout. Did you mean make
> --reload work?

Whoops, yes. Thanks for catching that.

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:90: 
On 2013/11/26 12:25:17, rmistry wrote:
> Nit: There should be two newlines between top level functions and classes.

Done.

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:158: def reload_seconds(self):
On 2013/11/26 12:25:17, rmistry wrote:
> Make this a property so that you can refer to it with SERVER.reload_seconds
> 
> @property
> def reload_seconds(self):
>   ...
> 
> Could make is_exported and is_editable properties as well.

Done.

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:197: self.results = results.Results(
On 2013/11/26 12:25:17, rmistry wrote:
> I recommend making self._results that is None in the constructor with a
comment
> saying it will be set when update_results is called successfully.
> 
> Then making a
> @property
> def results(self):
>   return self._results

Done.

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:206: while self._reload_seconds:
On 2013/11/26 12:25:17, rmistry wrote:
> If we only start the thread when reload_seconds is specified this can be while
> True.
> while self._reload_seconds was a little confusing to me because it makes it
seem
> like it is getting decremented somewhere.

Done.

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:212:
thread.start_new_thread(self._result_reloader, ())
On 2013/11/26 12:25:17, rmistry wrote:
> Maybe we should not even bother starting a new thread if self._reload_seconds
is
> 0.
> 
> if self._result_reloader:
>   thread.start_new_thread(self._result_reloader, ())

Done.

https://codereview.chromium.org/86343002/diff/20001/gm/rebaseline_server/serv...
gm/rebaseline_server/server.py:476: 'results will be updated.  In fact, your
entire '
On 2013/11/26 12:25:17, rmistry wrote:
> Could reword this by saying something like:
> If specified, both expected and actual results will be updated by running
> gclient sync on the Skia checkout. By default, ...

Done.

[rmistry, 2013-11-26 17:39:08]

LGTM

[epoger, 2013-11-26 17:59:30]

Committed patchset #3 manually as r12397.