Run skimage on the bots.

Run skimage on the bots.

Run our image decoders on a set of images for regression testing.

On each run, we will download a set of expectations and download
the images from Google Storage. Both are copied to the device
if necessary so we can run the tests.

TODO: Add a script to perform the rebaseline.

BUG=https://code.google.com/p/skia/issues/detail?id=1241
R=borenet@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9771, and then reverted https://code.google.com/p/skia/source/detail?r=9772.

Reverted due to the fact that it wasn't quite ready, and it was only submitted due to a muscle memory error.

(SkipBuildbotRuns)

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

[scroggo, 2013-06-24 17:43:16]

Elliot,

Here are my in-progress changes to run skimage on the bots. Would you mind
looking over them and discussing with me tomorrow or Wednesday what is the
correct way to do things?

Thanks,
Leon

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/android_postrender.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/android_postrender.py:32:
android_utils.RunADB(self._serial, ['pull',
The android paths are untested, as I have been unable to get past the install
step: exception due to failing the following command:

adb -s 015d210a13480604 shell echo \$EXTERNAL_STORAGE

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/chromeos_postrender.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/chromeos_postrender.py:43:
ssh_utils.GetSCP(self._skimage_out_dir, self._device_dirs.SKImageOutDir(),
The chrome steps are also untested, but mimic similar code above, to copy and
remove from the device.

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/get_subdir.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/get_subdir.py:9: def
GetSubDirFromBuilderName(builder_name):
This is a hack I've used to grab the right expectations file.

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/get_subdir.py:41: return
'base-shuttle_ubuntu12_ati5770'
Double hack, because my local builder was not found...

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/update.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/update.py:67: if not solution['url'].endswith('.git')],
I assume there is some other method we'll be using once we switch everything to
git? My repository is already hosted in git, and running this Sync function on
it tries to rebase onto a commit named <current skia revision> (which doesn't
exist). Skipping it here allows downloading it.

[scroggo, 2013-06-25 15:38:10]

D'oh! Elliot's on vacation.

Eric, do you mind taking a look?

[borenet, 2013-06-25 18:19:12]

https://codereview.chromium.org/16226005/diff/50001/master/skia_master_script...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/16226005/diff/50001/master/skia_master_script...
master/skia_master_scripts/factory.py:106:
svn_url='https://skia.googlesource.com/expectations.git',
As discussed before, I think the expectations belong in the Skia repository.

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/android_build_step.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/android_build_step.py:95: skimage_expected_dir=prefix +
'skimage_expected',
Nit: 80 chars

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/build_step.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/build_step.py:249: skimage_subdir =
GetSubDirFromBuilderName(self._builder_name)
Would it be equivalent to use self._gm_image_subdir?

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/chromeos_build_step.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/chromeos_build_step.py:83: skimage_expected_dir=prefix
+ 'skimage_expected',
Nit: 80 chars

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/get_subdir.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/get_subdir.py:9: def
GetSubDirFromBuilderName(builder_name):
On 2013/06/24 17:43:16, scroggo wrote:
> This is a hack I've used to grab the right expectations file. 

I made this comment in BuildStep - it looks like we could just use
self._gm_image_subdir instead of having this function.  IMO it's better not to
spread this hack around.  The downside is that it's sometimes None (which is
perfectly okay), so you'll have to check for that.

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/update.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/update.py:67: if not solution['url'].endswith('.git')],
On 2013/06/24 17:43:16, scroggo wrote:
> I assume there is some other method we'll be using once we switch everything
to
> git? My repository is already hosted in git, and running this Sync function on
> it tries to rebase onto a commit named <current skia revision> (which doesn't
> exist). Skipping it here allows downloading it.

Does this actually cause the git repository to be updated?  As I mentioned, I
think we need the Skia code and the corresponding expectations to be locked at
the same commit.  The sensible way to do that is to have them in the same
repository.  If we do that, this problem goes away.

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/upload_skimage_results.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/upload_skimage_results.py:50:
gs_utils.UploadDirectoryContentsIfChanged(
Note that this mechanism will change very soon, once we have the checksum-based
syncing in place.

[scroggo, 2013-06-25 19:28:56]

https://codereview.chromium.org/16226005/diff/50001/master/skia_master_script...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/16226005/diff/50001/master/skia_master_script...
master/skia_master_scripts/factory.py:106:
svn_url='https://skia.googlesource.com/expectations.git',
On 2013/06/25 18:19:12, borenet wrote:
> As discussed before, I think the expectations belong in the Skia repository.

Sorry, I added the expectations repository before we got into the depths of
discussing where everything should go. Is the structure in
https://docs.google.com/a/google.com/document/d/1FSKX0u6Q7W_pQby000fV93y7Gndf...
final? Should I check this directory into our SVN repo alongside gm-expected for
now?

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/android_build_step.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/android_build_step.py:95: skimage_expected_dir=prefix +
'skimage_expected',
On 2013/06/25 18:19:12, borenet wrote:
> Nit: 80 chars

Done.

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/build_step.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/build_step.py:249: skimage_subdir =
GetSubDirFromBuilderName(self._builder_name)
On 2013/06/25 18:19:12, borenet wrote:
> Would it be equivalent to use self._gm_image_subdir?

Yes. Thanks!

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/chromeos_build_step.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/chromeos_build_step.py:83: skimage_expected_dir=prefix
+ 'skimage_expected',
On 2013/06/25 18:19:12, borenet wrote:
> Nit: 80 chars

Done.

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/get_subdir.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/get_subdir.py:9: def
GetSubDirFromBuilderName(builder_name):
On 2013/06/25 18:19:12, borenet wrote:
> On 2013/06/24 17:43:16, scroggo wrote:
> > This is a hack I've used to grab the right expectations file. 
> 
> I made this comment in BuildStep - it looks like we could just use
> self._gm_image_subdir instead of having this function.  IMO it's better not to
> spread this hack around.  The downside is that it's sometimes None (which is
> perfectly okay), so you'll have to check for that.

Done. What is the correct way to handle None? It seems like we could skip the
test, since we won't have anything to compare against. Do we still run gm in
that case?

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/update.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/update.py:67: if not solution['url'].endswith('.git')],
On 2013/06/25 18:19:12, borenet wrote:
> On 2013/06/24 17:43:16, scroggo wrote:
> > I assume there is some other method we'll be using once we switch everything
> to
> > git? My repository is already hosted in git, and running this Sync function
on
> > it tries to rebase onto a commit named <current skia revision> (which
doesn't
> > exist). Skipping it here allows downloading it.
> 
> Does this actually cause the git repository to be updated?  As I mentioned, I
> think we need the Skia code and the corresponding expectations to be locked at
> the same commit.  The sensible way to do that is to have them in the same
> repository.  If we do that, this problem goes away.

It does, but I agree with your concern about keeping everything in sync.

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/upload_skimage_results.py (right):

https://codereview.chromium.org/16226005/diff/50001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/upload_skimage_results.py:50:
gs_utils.UploadDirectoryContentsIfChanged(
On 2013/06/25 18:19:12, borenet wrote:
> Note that this mechanism will change very soon, once we have the
checksum-based
> syncing in place.

Noted. I'll keep watch on Elliot's progress.

[borenet, 2013-06-25 19:55:40]

Patch set 12 LGTM except for my concern about the home of the expectations.

https://codereview.chromium.org/16226005/diff/50001/master/skia_master_script...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/16226005/diff/50001/master/skia_master_script...
master/skia_master_scripts/factory.py:106:
svn_url='https://skia.googlesource.com/expectations.git',
On 2013/06/25 19:28:56, scroggo wrote:
> On 2013/06/25 18:19:12, borenet wrote:
> > As discussed before, I think the expectations belong in the Skia repository.
> 
> Sorry, I added the expectations repository before we got into the depths of
> discussing where everything should go. Is the structure in
>
https://docs.google.com/a/google.com/document/d/1FSKX0u6Q7W_pQby000fV93y7Gndf...
> final? Should I check this directory into our SVN repo alongside gm-expected
for
> now?

I think we came to the agreement that the expectations needed to go in the
primary repository.  I got the impression that the other aspects of the new
structure were agreeable, but I don't think we're in a hurry to implement them.

For now, placing them alongside gm-expected should be okay, but they'll need to
be moved before we can switch to git.  Alternatively, we could go ahead and
create an expectations directory inside of trunk.

[borenet, 2013-06-25 19:57:04]

We'll need to coordinate submitting this with a master restart.

[scroggo, 2013-06-25 22:23:16]

New directory in trunk created by https://codereview.chromium.org/17737002/

https://codereview.chromium.org/16226005/diff/55002/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/install.py (right):

https://codereview.chromium.org/16226005/diff/55002/slave/skia_slave_scripts/...
slave/skia_slave_scripts/install.py:54:
self.CreateCleanDirectory(self._skimage_out_dir)
Oops. I wanted to create a clean local directory on the host, not the device.

[scroggo, 2013-06-25 22:23:39]

On 2013/06/25 22:23:16, scroggo wrote:
> New directory in trunk created by https://codereview.chromium.org/17737002/
> 
>
https://codereview.chromium.org/16226005/diff/55002/slave/skia_slave_scripts/...
> File slave/skia_slave_scripts/install.py (right):
> 
>
https://codereview.chromium.org/16226005/diff/55002/slave/skia_slave_scripts/...
> slave/skia_slave_scripts/install.py:54:
> self.CreateCleanDirectory(self._skimage_out_dir)
> Oops. I wanted to create a clean local directory on the host, not the device.

And latest patch looks in the new location for expectations.

[borenet, 2013-06-26 12:54:44]

Just a few more comments.

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/build_step.py (right):

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/build_step.py:250: self._skimage_expected_dir =
os.path.join('expectations', 'skimage')
Not using gm_image_subdir, since we're using a flat directory, right?

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/download_skimage_files.py (right):

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/download_skimage_files.py:18: **kwargs)
Nit: spacing.  If the parameters won't line up without fitting in 80 chars,
maybe this would be better?

super(DownloadSKImageFiles, self).__init__(
    timeout=timeout,
    no_output_timeout=no_output_timeout,
    **kwargs)

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/install.py (right):

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/install.py:58:
self.CreateCleanDirectory(self._device_dirs.SKImageOutDir())
Can we put these additions in PreRender since they aren't used by the Perf bots?

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/run_decoding_tests.py (right):

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/run_decoding_tests.py:31:
cmd.extend(['--readExpectationsPath', expectations_file])
I think we should verify that the file exists before passing this flag.

[scroggo, 2013-06-26 15:06:39]

New patch ready for review.

When I do a git cl upload, I fail the factory_configuration_test. How do I
update the expectations to pass this test?

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/build_step.py (right):

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/build_step.py:250: self._skimage_expected_dir =
os.path.join('expectations', 'skimage')
On 2013/06/26 12:54:45, borenet wrote:
> Not using gm_image_subdir, since we're using a flat directory, right?

That is correct. We're actually using gm_image_subdir to determine the name of
the expectations file. (See run_decoding_tests.py)

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/download_skimage_files.py (right):

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/download_skimage_files.py:18: **kwargs)
On 2013/06/26 12:54:45, borenet wrote:
> Nit: spacing.  If the parameters won't line up without fitting in 80 chars,
> maybe this would be better?
> 
> super(DownloadSKImageFiles, self).__init__(
>     timeout=timeout,
>     no_output_timeout=no_output_timeout,
>     **kwargs)

This is copied from download_skps.py. Agreed that your way is better and fits
with the style guide. Changing in both places.

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/install.py (right):

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/install.py:58:
self.CreateCleanDirectory(self._device_dirs.SKImageOutDir())
On 2013/06/26 12:54:45, borenet wrote:
> Can we put these additions in PreRender since they aren't used by the Perf
bots?

Done.

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
File slave/skia_slave_scripts/run_decoding_tests.py (right):

https://codereview.chromium.org/16226005/diff/76001/slave/skia_slave_scripts/...
slave/skia_slave_scripts/run_decoding_tests.py:31:
cmd.extend(['--readExpectationsPath', expectations_file])
On 2013/06/26 12:54:45, borenet wrote:
> I think we should verify that the file exists before passing this flag.

Done.

[scroggo, 2013-06-26 16:55:51]

Latest patch includes the results from running

factory_configuration_test.py --rebaseline

Build-Ubuntu12-GCC-x86-Debug-ChromeOS and
Build-Ubuntu12-GCC-x86-Debug-ChromeOS-Trybot are new, as opposed to modifying
old expectations. Should I leave those out?

[scroggo, 2013-06-26 17:15:03]

https://codereview.chromium.org/16226005/diff/105001/tools/tests/factory_conf...
File
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Release-ChromeOS
(right):

https://codereview.chromium.org/16226005/diff/105001/tools/tests/factory_conf...
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Release-ChromeOS:22:
'False',
Any idea why this changed from False to True? Did someone forget a rebaseline?
It only seems to have changed in the ChromeOS files.

[scroggo, 2013-06-26 17:16:27]

Committed patchset #15 manually as r9771 (presubmit successful).

[borenet, 2013-06-26 17:37:54]

In the future, you don't need to include all of the factory config files as part
of the review.  If there's nothing too surprising, just one or two is fine.

https://codereview.chromium.org/16226005/diff/113001/slave/skia_slave_scripts...
File slave/skia_slave_scripts/run_decoding_tests.py (right):

https://codereview.chromium.org/16226005/diff/113001/slave/skia_slave_scripts...
slave/skia_slave_scripts/run_decoding_tests.py:24: expectations_name =
self._gm_image_subdir + '.json'
I think the better way is to check whether self._gm_image_subdir is None here.

https://codereview.chromium.org/16226005/diff/113001/slave/skia_slave_scripts...
slave/skia_slave_scripts/run_decoding_tests.py:30: if
os.path.exists(expectations_file):
This won't work for Android or ChromeOS.

https://codereview.chromium.org/16226005/diff/113001/tools/tests/factory_conf...
File
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Debug-ChromeOS
(right):

https://codereview.chromium.org/16226005/diff/113001/tools/tests/factory_conf...
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Debug-ChromeOS:2:
'TargetPathJoin': <Object>,
Please "svn rm" this file, as this bot no longer exists.

https://codereview.chromium.org/16226005/diff/113001/tools/tests/factory_conf...
File
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Debug-ChromeOS-Trybot
(right):

https://codereview.chromium.org/16226005/diff/113001/tools/tests/factory_conf...
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Debug-ChromeOS-Trybot:2:
'TargetPathJoin': <Object>,
Please "svn rm" this file, as this bot no longer exists.

https://codereview.chromium.org/16226005/diff/113001/tools/tests/factory_conf...
File
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Release-ChromeOS
(right):

https://codereview.chromium.org/16226005/diff/113001/tools/tests/factory_conf...
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Release-ChromeOS:1:
{
Please "svn rm" this file, as this bot no longer exists.

https://codereview.chromium.org/16226005/diff/113001/tools/tests/factory_conf...
File
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Release-ChromeOS-Trybot
(right):

https://codereview.chromium.org/16226005/diff/113001/tools/tests/factory_conf...
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Release-ChromeOS-Trybot:2:
'TargetPathJoin': <Object>,
Please "svn rm" this file, as this bot no longer exists.

[scroggo, 2013-06-26 18:30:15]

https://codereview.chromium.org/16226005/diff/113001/slave/skia_slave_scripts...
File slave/skia_slave_scripts/run_decoding_tests.py (right):

https://codereview.chromium.org/16226005/diff/113001/slave/skia_slave_scripts...
slave/skia_slave_scripts/run_decoding_tests.py:24: expectations_name =
self._gm_image_subdir + '.json'
On 2013/06/26 17:37:54, borenet wrote:
> I think the better way is to check whether self._gm_image_subdir is None here.

Done.

https://codereview.chromium.org/16226005/diff/113001/slave/skia_slave_scripts...
slave/skia_slave_scripts/run_decoding_tests.py:30: if
os.path.exists(expectations_file):
On 2013/06/26 17:37:54, borenet wrote:
> This won't work for Android or ChromeOS.

Added DevicePathExists.

I also switched from os.path.join to DevicePathJoin.

https://codereview.chromium.org/16226005/diff/113001/tools/tests/factory_conf...
File
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Debug-ChromeOS
(right):

https://codereview.chromium.org/16226005/diff/113001/tools/tests/factory_conf...
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Debug-ChromeOS:2:
'TargetPathJoin': <Object>,
On 2013/06/26 17:37:54, borenet wrote:
> Please "svn rm" this file, as this bot no longer exists.

All four of them have been removed. Should we change factory_configuration_test
to not generate them?

[borenet, 2013-06-26 18:47:31]

https://codereview.chromium.org/16226005/diff/113001/tools/tests/factory_conf...
File
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Debug-ChromeOS
(right):

https://codereview.chromium.org/16226005/diff/113001/tools/tests/factory_conf...
tools/tests/factory_configuration/expected/Build-Ubuntu12-GCC-x86-Debug-ChromeOS:2:
'TargetPathJoin': <Object>,
On 2013/06/26 18:30:15, scroggo wrote:
> On 2013/06/26 17:37:54, borenet wrote:
> > Please "svn rm" this file, as this bot no longer exists.
> 
> All four of them have been removed. Should we change
factory_configuration_test
> to not generate them?

It looks like I forgot to remove them when I renamed the builder last week, so
this is my mistake.  They shouldn't be generated any more.

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
File slave/skia_slave_scripts/android_build_step.py (right):

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
slave/skia_slave_scripts/android_build_step.py:57: return posixpath.exists(path)
You'll need to run something in the ADB shell to do this, as posixpath is still
looking on the host side.  I think you can just move the implementation from
_RemoveDirectoryOnDevice.

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
File slave/skia_slave_scripts/chromeos_build_step.py (right):

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
slave/skia_slave_scripts/chromeos_build_step.py:51: return
posixpath.exists(path)
Again, this won't work because it's on the host side.  I think you can copy the
implementation from _RemoveDirectoryOnDevice.

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
File slave/skia_slave_scripts/run_decoding_tests.py (right):

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
slave/skia_slave_scripts/run_decoding_tests.py:28: expectations_file =
self.DevicePathJoin(
Nice!

[borenet, 2013-06-26 18:51:55]

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
File slave/skia_slave_scripts/run_decoding_tests.py (right):

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
slave/skia_slave_scripts/run_decoding_tests.py:1: #!/usr/bin/env python
I just realized - You'll need to create "android_run_decoding_tests.py" and
"chromeos_run_decoding_tests.py".  This is an unfortunate side effect of the way
this framework is set up.  They'll just be empty classes which inherit from both
Android/ChromeOSBuildStep and RunDecodingTests.  Take a look at
chromeos_run_gm.py for an example.

[scroggo, 2013-06-26 20:07:08]

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
File slave/skia_slave_scripts/android_build_step.py (right):

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
slave/skia_slave_scripts/android_build_step.py:57: return posixpath.exists(path)
On 2013/06/26 18:47:31, borenet wrote:
> You'll need to run something in the ADB shell to do this, as posixpath is
still
> looking on the host side.  I think you can just move the implementation from
> _RemoveDirectoryOnDevice.

Done.

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
File slave/skia_slave_scripts/chromeos_build_step.py (right):

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
slave/skia_slave_scripts/chromeos_build_step.py:51: return
posixpath.exists(path)
On 2013/06/26 18:47:31, borenet wrote:
> Again, this won't work because it's on the host side.  I think you can copy
the
> implementation from _RemoveDirectoryOnDevice.

Done.

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
File slave/skia_slave_scripts/run_decoding_tests.py (right):

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
slave/skia_slave_scripts/run_decoding_tests.py:1: #!/usr/bin/env python
On 2013/06/26 18:51:56, borenet wrote:
> I just realized - You'll need to create "android_run_decoding_tests.py" and
> "chromeos_run_decoding_tests.py".  This is an unfortunate side effect of the
way
> this framework is set up.  They'll just be empty classes which inherit from
both
> Android/ChromeOSBuildStep and RunDecodingTests.  Take a look at
> chromeos_run_gm.py for an example.

Done.

How does the slave know where to find the right script? In factory.py
RunDecodingTests() adds the slave script "run_decoding_tests.py". Somewhere in
android_factory?

https://codereview.chromium.org/16226005/diff/126001/slave/skia_slave_scripts...
File slave/skia_slave_scripts/run_decoding_tests.py (right):

https://codereview.chromium.org/16226005/diff/126001/slave/skia_slave_scripts...
slave/skia_slave_scripts/run_decoding_tests.py:18: for filename in
self.DeviceListDir(image_dir)
This CL also adds DeviceListDir.

[borenet, 2013-06-26 20:23:14]

LGTM.  I plan to restart the master tomorrow morning around 8.  IMO the best way
to commit these kinds of changes is to add "(SkipBuildbotRuns) to the CL
description so that the bots don't run, commit shortly before a restart, and
close (or caution) the tree.

Also, you'll need to rebaseline the factory configs again since I just committed
https://codereview.chromium.org/17931002/ which adds a few more bots. No need to
upload a new patch set for that.

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
File slave/skia_slave_scripts/run_decoding_tests.py (right):

https://codereview.chromium.org/16226005/diff/105002/slave/skia_slave_scripts...
slave/skia_slave_scripts/run_decoding_tests.py:1: #!/usr/bin/env python
On 2013/06/26 20:07:08, scroggo wrote:
> On 2013/06/26 18:51:56, borenet wrote:
> > I just realized - You'll need to create "android_run_decoding_tests.py" and
> > "chromeos_run_decoding_tests.py".  This is an unfortunate side effect of the
> way
> > this framework is set up.  They'll just be empty classes which inherit from
> both
> > Android/ChromeOSBuildStep and RunDecodingTests.  Take a look at
> > chromeos_run_gm.py for an example.
> 
> Done.
> 
> How does the slave know where to find the right script? In factory.py
> RunDecodingTests() adds the slave script "run_decoding_tests.py". Somewhere in
> android_factory?

There's a "flavor" property for each build factory.  If it's not defined, the
vanilla script is used.  If it is defined, it's prepended to the name of the
script, eg for flavor "android", "android_run_decoding_tests.py" gets run.  This
is only for slave scripts which are run with AddFlavoredSlaveScript.

[scroggo, 2013-06-26 20:46:10]

Committed patchset #18 manually as r9779 (presubmit successful).

[epoger, 2013-07-08 16:03:05]

LGTM (after the fact) with one question...

https://codereview.chromium.org/16226005/diff/126001/slave/skia_slave_scripts...
File slave/skia_slave_scripts/download_skimage_files.py (right):

https://codereview.chromium.org/16226005/diff/126001/slave/skia_slave_scripts...
slave/skia_slave_scripts/download_skimage_files.py:6: """ Download the image
files needed to run skimage tool. """
Wouldn't it be simpler to keep the source image files in source control (e.g.
https://code.google.com/p/skia/source/browse/#svn%2Ftrunk%2Fresources ), rather
than downloading them from Google Storage?

Or are there a very large number of these images, or maybe they change often? 
(Those are reasons I could imagine not wanting to have them in the repo)

One thing that's nice about keeping files in our repo is that it makes it easier
for individual developers to run the test locally...

[scroggo, 2013-07-08 22:36:44]

> LGTM (after the fact) with one question...

Thanks for looking over it, Elliot.

https://codereview.chromium.org/16226005/diff/126001/slave/skia_slave_scripts...
File slave/skia_slave_scripts/download_skimage_files.py (right):

https://codereview.chromium.org/16226005/diff/126001/slave/skia_slave_scripts...
slave/skia_slave_scripts/download_skimage_files.py:6: """ Download the image
files needed to run skimage tool. """
On 2013/07/08 16:03:05, epoger wrote:
> Wouldn't it be simpler to keep the source image files in source control (e.g.
> https://code.google.com/p/skia/source/browse/#svn%252Ftrunk%252Fresources ),
rather
> than downloading them from Google Storage?

Yes, it would be easier to keep them in resources.
> 
> Or are there a very large number of these images, or maybe they change often? 
> (Those are reasons I could imagine not wanting to have them in the repo)

There are currently a "small" number of images. My understanding was that we
wanted NO images under git source control, but perhaps I took the warning too
far. My plan is to check in more images when we find and fix bugs in our image
decoders, and continue to have them for regression testing. The images
themselves will not change (i.e. '<image1>.png' will always be the same encoded
image). I don't know where the line into "very large" is, or when we will cross
it.

[epoger, 2013-07-12 15:09:01]

https://codereview.chromium.org/16226005/diff/126001/slave/skia_slave_scripts...
File slave/skia_slave_scripts/download_skimage_files.py (right):

https://codereview.chromium.org/16226005/diff/126001/slave/skia_slave_scripts...
slave/skia_slave_scripts/download_skimage_files.py:6: """ Download the image
files needed to run skimage tool. """
On 2013/07/08 22:36:44, scroggo wrote:
> On 2013/07/08 16:03:05, epoger wrote:
> > Wouldn't it be simpler to keep the source image files in source control
(e.g.
> >
https://code.google.com/p/skia/source/browse/#svn%25252Ftrunk%25252Fresources ),
> rather
> > than downloading them from Google Storage?
> 
> Yes, it would be easier to keep them in resources.
> > 
> > Or are there a very large number of these images, or maybe they change
often? 
> > (Those are reasons I could imagine not wanting to have them in the repo)
> 
> There are currently a "small" number of images. My understanding was that we
> wanted NO images under git source control, but perhaps I took the warning too
> far. My plan is to check in more images when we find and fix bugs in our image
> decoders, and continue to have them for regression testing. The images
> themselves will not change (i.e. '<image1>.png' will always be the same
encoded
> image). I don't know where the line into "very large" is, or when we will
cross
> it.

I don't know where the line is either. :-)  You get to decide.

Since you've already got this working pulling images from Google Storage, that's
fine.  If it ends up being trouble for a developer who wants to run locally (or
for some other reason), and thus you decide to move the images into our repo,
that's fine too.