rebaseline_server: download actual-results.json files from GCS instead of SVN

rebaseline_server: download actual-results.json files from GCS instead of SVN

BUG=skia:553
Committed: https://skia.googlesource.com/skia/+/f4666045f378dee22ce40108e90bf5195634ebfc

[epoger, 2014-06-03 20:11:03]

Eric (or Ravi)- PTAL

https://codereview.chromium.org/310093003/diff/40001/gm/rebaseline_server/dow...
File gm/rebaseline_server/download_actuals.py (left):

https://codereview.chromium.org/310093003/diff/40001/gm/rebaseline_server/dow...
gm/rebaseline_server/download_actuals.py:109: # we don't already have on local
disk.
This comment turns out to be misleading... download_actuals.py writes out files
with NON-checksum-based filenames, so we can't easily tell whether the images we
have already downloaded need to be overwritten.

https://codereview.chromium.org/310093003/diff/40001/gm/rebaseline_server/dow...
File gm/rebaseline_server/download_actuals.py (right):

https://codereview.chromium.org/310093003/diff/40001/gm/rebaseline_server/dow...
gm/rebaseline_server/download_actuals.py:167: EPOGER: move into a gcs utility
module?
I think I should move gcs_download_file() and gcs_list_bucket_contents() into a
new gcs_utils.py module before committing this.  Does that sound good to you?

[borenet, 2014-06-03 20:27:47]

Looks mostly good, just a couple of comments/questions.

https://codereview.chromium.org/310093003/diff/40001/gm/rebaseline_server/dow...
File gm/rebaseline_server/download_actuals.py (right):

https://codereview.chromium.org/310093003/diff/40001/gm/rebaseline_server/dow...
gm/rebaseline_server/download_actuals.py:167: EPOGER: move into a gcs utility
module?
On 2014/06/03 20:11:03, epoger wrote:
> I think I should move gcs_download_file() and gcs_list_bucket_contents() into
a
> new gcs_utils.py module before committing this.  Does that sound good to you?

Yes, that SGTM, but what's the difference then between "gcs_utils" and
"gs_utils"?  Should one supercede the other?  This is another module which would
be nice to share between buildbot and skia...

https://codereview.chromium.org/310093003/diff/40001/gm/rebaseline_server/ser...
File gm/rebaseline_server/server.py (right):

https://codereview.chromium.org/310093003/diff/40001/gm/rebaseline_server/ser...
gm/rebaseline_server/server.py:321: # instead of raw git commands.
This is not really true, and the below will not have the intended effect for
unmanaged checkouts.  Would we be better off doing this?

$ git reset --hard HEAD
$ git checkout master -f
$ git fetch
$ git reset --hard origin/master
$ gclient sync

That should ensure that we get exactly where we want to go.  It's destructive
though...

[epoger, 2014-06-03 20:39:04]

Thanks, Eric. Please grab me for live discussion of these issues when you have a
few minutes.

https://codereview.chromium.org/310093003/diff/40001/gm/rebaseline_server/dow...
File gm/rebaseline_server/download_actuals.py (right):

https://codereview.chromium.org/310093003/diff/40001/gm/rebaseline_server/dow...
gm/rebaseline_server/download_actuals.py:167: EPOGER: move into a gcs utility
module?
On 2014/06/03 20:27:47, borenet wrote:
> On 2014/06/03 20:11:03, epoger wrote:
> > I think I should move gcs_download_file() and gcs_list_bucket_contents()
into
> a
> > new gcs_utils.py module before committing this.  Does that sound good to
you?
> 
> Yes, that SGTM, but what's the difference then between "gcs_utils" and
> "gs_utils"?  Should one supercede the other?  This is another module which
would
> be nice to share between buildbot and skia...

In this case I went with "gcs_utils.py" because (a) the public API documentation
refers to Google Cloud Storage, and (b) it seemed helpful to be clear that this
is distinct from the gs_utils.py file in buildbot and not using the gsutil
binary.  But I'm happy calling it just about anything.

In buildbot-land, I think we should try to update gs_utils.py to use these API
calls instead of the gsutil binary.  (Ideally, the callers to it won't have to
change at all, unless they want to.)

I agree that it would be good for the two modules to be shared somehow.  I'll
add a TODO when I create the new module.

https://codereview.chromium.org/310093003/diff/40001/gm/rebaseline_server/ser...
File gm/rebaseline_server/server.py (right):

https://codereview.chromium.org/310093003/diff/40001/gm/rebaseline_server/ser...
gm/rebaseline_server/server.py:321: # instead of raw git commands.
On 2014/06/03 20:27:47, borenet wrote:
> This is not really true, and the below will not have the intended effect for
> unmanaged checkouts.  Would we be better off doing this?
> 
> $ git reset --hard HEAD
> $ git checkout master -f
> $ git fetch
> $ git reset --hard origin/master
> $ gclient sync
> 
> That should ensure that we get exactly where we want to go.  It's destructive
> though...

Yikes.

[Insert "blah blah what is our officially supported method of checking out Skia
blah blah" here.]

I dunno if that's a good change or not, but I'd definitely like to leave it for
a future CL either way, so we can kill skia-autogen without getting bogged down
in all that.

[epoger, 2014-06-03 21:43:20]

Thanks, Eric.  Please see patchset 4.

https://codereview.chromium.org/310093003/diff/60001/gm/rebaseline_server/gs_...
File gm/rebaseline_server/gs_utils.py (right):

https://codereview.chromium.org/310093003/diff/60001/gm/rebaseline_server/gs_...
gm/rebaseline_server/gs_utils.py:11: TODO(epoger): move this into tools/utils
for broader use?
Any thoughts on where gs_utils.py and url_utils.py should live?

My guess is that somewhere inside tools/ is the right place, probably a new
tools/utils/ dir, but I didn't want to go to the trouble until you tell me where
you think is a good place for them.

[borenet, 2014-06-04 13:10:15]

Looks good except I'm confused about copy_contents.

https://codereview.chromium.org/310093003/diff/60001/gm/rebaseline_server/gs_...
File gm/rebaseline_server/gs_utils.py (right):

https://codereview.chromium.org/310093003/diff/60001/gm/rebaseline_server/gs_...
gm/rebaseline_server/gs_utils.py:11: TODO(epoger): move this into tools/utils
for broader use?
On 2014/06/03 21:43:20, epoger wrote:
> Any thoughts on where gs_utils.py and url_utils.py should live?
> 
> My guess is that somewhere inside tools/ is the right place, probably a new
> tools/utils/ dir, but I didn't want to go to the trouble until you tell me
where
> you think is a good place for them.

Yeah, tools/utils or tools/pyutils (to be explicit that it only contains Python
utility modules) or something sounds good.

https://codereview.chromium.org/310093003/diff/60001/gm/rebaseline_server/url...
File gm/rebaseline_server/url_utils.py (right):

https://codereview.chromium.org/310093003/diff/60001/gm/rebaseline_server/url...
gm/rebaseline_server/url_utils.py:58: dest_dir = os.path.dirname(dest_path)
I'm confused about why you're using dirname here.  It seems to behave strangely:

> os.path.dirname('/home/borenet')
/home

So the whole directory path doesn't get created?

[epoger, 2014-06-04 14:39:35]

Thanks, Eric... please see patchset 5.

https://codereview.chromium.org/310093003/diff/60001/gm/rebaseline_server/gs_...
File gm/rebaseline_server/gs_utils.py (right):

https://codereview.chromium.org/310093003/diff/60001/gm/rebaseline_server/gs_...
gm/rebaseline_server/gs_utils.py:11: TODO(epoger): move this into tools/utils
for broader use?
On 2014/06/04 13:10:15, borenet wrote:
> On 2014/06/03 21:43:20, epoger wrote:
> > Any thoughts on where gs_utils.py and url_utils.py should live?
> > 
> > My guess is that somewhere inside tools/ is the right place, probably a new
> > tools/utils/ dir, but I didn't want to go to the trouble until you tell me
> where
> > you think is a good place for them.
> 
> Yeah, tools/utils or tools/pyutils (to be explicit that it only contains
Python
> utility modules) or something sounds good.

Moved into tools/pyutils.  To get that working, I figured out a simpler way for
the rebaseline_server code to import modules from tools/ and gm/.

https://codereview.chromium.org/310093003/diff/60001/gm/rebaseline_server/url...
File gm/rebaseline_server/url_utils.py (right):

https://codereview.chromium.org/310093003/diff/60001/gm/rebaseline_server/url...
gm/rebaseline_server/url_utils.py:58: dest_dir = os.path.dirname(dest_path)
On 2014/06/04 13:10:15, borenet wrote:
> I'm confused about why you're using dirname here.  It seems to behave
strangely:
> 
> > os.path.dirname('/home/borenet')
> /home
> 
> So the whole directory path doesn't get created?

I think the behavior is correct (and it does work in practice)... dest_path is a
file, not a directory.

[borenet, 2014-06-04 15:04:52]

LGTM with optional nit about the import_* modules.

https://codereview.chromium.org/310093003/diff/60001/gm/rebaseline_server/url...
File gm/rebaseline_server/url_utils.py (right):

https://codereview.chromium.org/310093003/diff/60001/gm/rebaseline_server/url...
gm/rebaseline_server/url_utils.py:58: dest_dir = os.path.dirname(dest_path)
On 2014/06/04 14:39:36, epoger wrote:
> On 2014/06/04 13:10:15, borenet wrote:
> > I'm confused about why you're using dirname here.  It seems to behave
> strangely:
> > 
> > > os.path.dirname('/home/borenet')
> > /home
> > 
> > So the whole directory path doesn't get created?
> 
> I think the behavior is correct (and it does work in practice)... dest_path is
a
> file, not a directory.

Got it. For some reason, I interpreted this as copying a directory, which makes
no sense.

https://codereview.chromium.org/310093003/diff/80001/gm/rebaseline_server/imp...
File gm/rebaseline_server/import_gm.py (right):

https://codereview.chromium.org/310093003/diff/80001/gm/rebaseline_server/imp...
gm/rebaseline_server/import_gm.py:1: #!/usr/bin/python
I think the name is a little misleading, since it doesn't actually import
anything.  Why not combine this and import_tools.py into a fix_pythonpath
module?

[epoger, 2014-06-04 15:20:36]

I think we're there... please see patchsets 6+7.

https://codereview.chromium.org/310093003/diff/80001/gm/rebaseline_server/imp...
File gm/rebaseline_server/import_gm.py (right):

https://codereview.chromium.org/310093003/diff/80001/gm/rebaseline_server/imp...
gm/rebaseline_server/import_gm.py:1: #!/usr/bin/python
On 2014/06/04 15:04:52, borenet wrote:
> I think the name is a little misleading, since it doesn't actually import
> anything.  Why not combine this and import_tools.py into a fix_pythonpath
> module?

Done in patchset 7.

https://codereview.chromium.org/310093003/diff/120001/tools/pyutils/url_utils...
File tools/pyutils/url_utils_test.py (right):

https://codereview.chromium.org/310093003/diff/120001/tools/pyutils/url_utils...
tools/pyutils/url_utils_test.py:23: class UrlUtilsTest(unittest.TestCase):
Whoops... as of patchset 5, this unittest wasn't getting run anymore (by
tools/tests/run.sh).  Fixed in patchset 6, and the Python unittest setup within
tools/ is better than it was before (supports automatic discovery of new test
modules).

[borenet, 2014-06-04 15:43:23]

LGTM at patch set 7.

https://codereview.chromium.org/310093003/diff/120001/tools/pyutils/url_utils...
File tools/pyutils/url_utils_test.py (right):

https://codereview.chromium.org/310093003/diff/120001/tools/pyutils/url_utils...
tools/pyutils/url_utils_test.py:23: class UrlUtilsTest(unittest.TestCase):
On 2014/06/04 15:20:59, epoger wrote:
> Whoops... as of patchset 5, this unittest wasn't getting run anymore (by
> tools/tests/run.sh).  Fixed in patchset 6, and the Python unittest setup
within
> tools/ is better than it was before (supports automatic discovery of new test
> modules).

Good catch!

[epoger, 2014-06-04 16:51:14]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-06-04 16:53:02]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/epoger@google.com/310093003/120001

[commit-bot: I haz the power, 2014-06-04 17:03:03]

Change committed as f4666045f378dee22ce40108e90bf5195634ebfc

[epoger, 2014-06-05 17:03:11]

Reverted in https://codereview.chromium.org/317783004 , as a result of
http://skbug.com/2641 ('rebaseline_server: google-api-python-client requires
manually installed dependencies')