Do not copy expectations file to itself.

Do not copy expectations file to itself.

r10969 had to be reverted because it was failing on Windows, Linux,
and Mac (but was succeeding on Android). Only call PushFileToDevice
when there is an actual device to push to.

R=epoger@google.com

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

[borenet, 2013-08-28 13:00:46]

https://codereview.chromium.org/23495007/diff/1/slave/skia_slave_scripts/prer...
File slave/skia_slave_scripts/prerender.py (right):

https://codereview.chromium.org/23495007/diff/1/slave/skia_slave_scripts/prer...
slave/skia_slave_scripts/prerender.py:140: if not skimage_device_expectations ==
skimage_host_expectations:
I would prefer "skimage_device_expectations != skimage_host_expectations"

[scroggo, 2013-08-28 13:01:57]

https://codereview.chromium.org/23495007/diff/1/slave/skia_slave_scripts/prer...
File slave/skia_slave_scripts/prerender.py (right):

https://codereview.chromium.org/23495007/diff/1/slave/skia_slave_scripts/prer...
slave/skia_slave_scripts/prerender.py:140: if not skimage_device_expectations ==
skimage_host_expectations:
On 2013/08/28 13:00:46, borenet wrote:
> I would prefer "skimage_device_expectations != skimage_host_expectations"

Done.

[scroggo, 2013-08-28 13:03:39]

https://codereview.chromium.org/23495007/diff/1/slave/skia_slave_scripts/prer...
File slave/skia_slave_scripts/prerender.py (right):

https://codereview.chromium.org/23495007/diff/1/slave/skia_slave_scripts/prer...
slave/skia_slave_scripts/prerender.py:140: if not skimage_device_expectations ==
skimage_host_expectations:
On 2013/08/28 13:01:57, scroggo wrote:
> On 2013/08/28 13:00:46, borenet wrote:
> > I would prefer "skimage_device_expectations != skimage_host_expectations"
> 
> Done.

I could actually compare self._device_dirs.SKImageExpectedDir against
self._skimage_expected_dir, which would skip the DevicePathJoin on Win/Lin/Mac.
Better?

[borenet, 2013-08-28 13:12:40]

https://codereview.chromium.org/23495007/diff/1/slave/skia_slave_scripts/prer...
File slave/skia_slave_scripts/prerender.py (right):

https://codereview.chromium.org/23495007/diff/1/slave/skia_slave_scripts/prer...
slave/skia_slave_scripts/prerender.py:140: if not skimage_device_expectations ==
skimage_host_expectations:
On 2013/08/28 13:03:39, scroggo wrote:
> On 2013/08/28 13:01:57, scroggo wrote:
> > On 2013/08/28 13:00:46, borenet wrote:
> > > I would prefer "skimage_device_expectations != skimage_host_expectations"
> > 
> > Done.
> 
> I could actually compare self._device_dirs.SKImageExpectedDir against
> self._skimage_expected_dir, which would skip the DevicePathJoin on
Win/Lin/Mac.
> Better?

I think maybe the best thing is to modify DefaultBuildStepUtils.PushFileToDevice
to be a no-op which raises an exception if src and dst are different (see
DefaultBuildStepUtils.CopyDirectoryContentsTo*)

[scroggo, 2013-08-28 13:45:00]

That won't quite work; see inline comments.

cc'ing Elliot in case he has some insight for the right thing to do with respect
to GM expectations.

https://codereview.chromium.org/23495007/diff/1/slave/skia_slave_scripts/prer...
File slave/skia_slave_scripts/prerender.py (right):

https://codereview.chromium.org/23495007/diff/1/slave/skia_slave_scripts/prer...
slave/skia_slave_scripts/prerender.py:140: if not skimage_device_expectations ==
skimage_host_expectations:
On 2013/08/28 13:12:40, borenet wrote:
> On 2013/08/28 13:03:39, scroggo wrote:
> > On 2013/08/28 13:01:57, scroggo wrote:
> > > On 2013/08/28 13:00:46, borenet wrote:
> > > > I would prefer "skimage_device_expectations !=
skimage_host_expectations"
> > > 
> > > Done.
> > 
> > I could actually compare self._device_dirs.SKImageExpectedDir against
> > self._skimage_expected_dir, which would skip the DevicePathJoin on
> Win/Lin/Mac.
> > Better?
> 
> I think maybe the best thing is to modify
DefaultBuildStepUtils.PushFileToDevice
> to be a no-op which raises an exception if src and dst are different (see
> DefaultBuildStepUtils.CopyDirectoryContentsTo*)

Currently, GM uses PushFileToDevice (up above, in this same file;
PushFileToDevice is called by no one else except by other android and chromeos
flavor_utils) to copy an expectations file from one of two places to the right
place, and modifying PushFileToDevice to behave like
CopyDirectoryContentsToDevice would break that copy. I can think of a few
possible solutions:
- depending on where GM is in the transition to checksums, it may be ready to
grab the expectations from one place, in which case that could always be the
same as device_gm_expectations_path (except of course, when there is a device,
which will call the custom version of PushFileToDevice).
- instead of changing PushFileToDevice to match the behavior of
CopyDirectoryContentsToDevice, change it to only do the copy if the two
locations are different.
- it seems that GM is moving locally because the expectations file is either
downloaded from SVN, or it's created in another place. Either way it's moved to
the right place. If it was not downloaded by SVN, what if we create it where it
would have been (repo_gm_expectations_path), set device_gm_expectations_path to
be repo_gm_expectations_path, and then the copy would be a no op (except for
attached devices).

[borenet, 2013-08-28 13:54:03]

On 2013/08/28 13:45:00, scroggo wrote:
> That won't quite work; see inline comments.
> 
> cc'ing Elliot in case he has some insight for the right thing to do with
respect
> to GM expectations.
> 
>
https://codereview.chromium.org/23495007/diff/1/slave/skia_slave_scripts/prer...
> File slave/skia_slave_scripts/prerender.py (right):
> 
>
https://codereview.chromium.org/23495007/diff/1/slave/skia_slave_scripts/prer...
> slave/skia_slave_scripts/prerender.py:140: if not skimage_device_expectations
==
> skimage_host_expectations:
> On 2013/08/28 13:12:40, borenet wrote:
> > On 2013/08/28 13:03:39, scroggo wrote:
> > > On 2013/08/28 13:01:57, scroggo wrote:
> > > > On 2013/08/28 13:00:46, borenet wrote:
> > > > > I would prefer "skimage_device_expectations !=
> skimage_host_expectations"
> > > > 
> > > > Done.
> > > 
> > > I could actually compare self._device_dirs.SKImageExpectedDir against
> > > self._skimage_expected_dir, which would skip the DevicePathJoin on
> > Win/Lin/Mac.
> > > Better?
> > 
> > I think maybe the best thing is to modify
> DefaultBuildStepUtils.PushFileToDevice
> > to be a no-op which raises an exception if src and dst are different (see
> > DefaultBuildStepUtils.CopyDirectoryContentsTo*)
> 
> Currently, GM uses PushFileToDevice (up above, in this same file;
> PushFileToDevice is called by no one else except by other android and chromeos
> flavor_utils) to copy an expectations file from one of two places to the right
> place, and modifying PushFileToDevice to behave like
> CopyDirectoryContentsToDevice would break that copy. I can think of a few
> possible solutions:
> - depending on where GM is in the transition to checksums, it may be ready to
> grab the expectations from one place, in which case that could always be the
> same as device_gm_expectations_path (except of course, when there is a device,
> which will call the custom version of PushFileToDevice).
> - instead of changing PushFileToDevice to match the behavior of
> CopyDirectoryContentsToDevice, change it to only do the copy if the two
> locations are different.
> - it seems that GM is moving locally because the expectations file is either
> downloaded from SVN, or it's created in another place. Either way it's moved
to
> the right place. If it was not downloaded by SVN, what if we create it where
it
> would have been (repo_gm_expectations_path), set device_gm_expectations_path
to
> be repo_gm_expectations_path, and then the copy would be a no op (except for
> attached devices).

I think we're a little inconsistent, and it's causing a problem: for builders
who don't have attached devices, we need to decide whether the host-side
directories and device side directories are going to be the same (for every one)
or different (again, for every one).  The GM expectations dir seems to be the
only one where the host-side dir and device-side dir can be different.  My
opinion is that we should make them the same across the board, and then the
copy/push functions can be no-ops which throw errors if they aren't the same.

Elliot, the code which creates the not-yet-existing expectations file is left
over from the transition to checksums.  Can we get rid of
_CreateExpectationsSummaryFromImages, replacing it with some utility to create
an empty expectations file, and make sure that we put that in the gm
expectations dir for the current builder rather than some temporary dir?

[epoger, 2013-08-28 15:20:28]

On 2013/08/28 13:54:03, borenet wrote:
> Elliot, the code which creates the not-yet-existing expectations file is left
> over from the transition to checksums.  Can we get rid of
> _CreateExpectationsSummaryFromImages, replacing it with some utility to create
> an empty expectations file, and make sure that we put that in the gm
> expectations dir for the current builder rather than some temporary dir?

We can't do it exactly like that, because the expectations file is typically
checked out of SVN.  If we have to create it locally, we don't want to create
that file within an SVN-managed directory, because that can cause merge
conflicts later.  (This is what I was getting at with the comment at line 89,
but I wasn't explicit enough about it.)

We can go ahead and delete the code that creates JSON expectations from images
(because all of our checked-in expectations are now in JSON form), but sadly the
PreRender step still needs to generate an empty expected-results.json file for
platforms where we have no expectations at all.  (e.g.
http://skiabot-master.pogerlabs.com:10117/builders/Test-ChromeOS-Alex-GMA3150...
)  It may be useful to find some other way to handle that case (like, run gm
without pointing at any expectations file in that case)

> I think we're a little inconsistent, and it's causing a problem: for builders
> who don't have attached devices, we need to decide whether the host-side
> directories and device side directories are going to be the same (for every
one)
> or different (again, for every one).  The GM expectations dir seems to be the
> only one where the host-side dir and device-side dir can be different.  My
> opinion is that we should make them the same across the board, and then the
> copy/push functions can be no-ops which throw errors if they aren't the same.

I think this is worth discussing.  Why do we need to mandate ANY relationship
between host-side and device-side directories?  Wouldn't this work just fine? 
PLEASE SEE ME.

class DefaultBuildStepUtils:
  def CopyDirectoryContentsToDevice(self, host_dir, device_dir):
    # There is no device, so it's just a local filecopy.
    if host_dir != device_dir:
      _LocalDirectoryCopy(host_dir, device_dir)

class AndroidBuildStepUtils(DefaultBuildStepUtils):
  def CopyDirectoryContentsToDevice(self, host_dir, device_dir):
    # Exactly as it is already implemented

[borenet, 2013-08-28 15:33:48]

On 2013/08/28 15:20:28, epoger wrote:
> On 2013/08/28 13:54:03, borenet wrote:
> > Elliot, the code which creates the not-yet-existing expectations file is
left
> > over from the transition to checksums.  Can we get rid of
> > _CreateExpectationsSummaryFromImages, replacing it with some utility to
create
> > an empty expectations file, and make sure that we put that in the gm
> > expectations dir for the current builder rather than some temporary dir?
> 
> We can't do it exactly like that, because the expectations file is typically
> checked out of SVN.  If we have to create it locally, we don't want to create
> that file within an SVN-managed directory, because that can cause merge
> conflicts later.  (This is what I was getting at with the comment at line 89,
> but I wasn't explicit enough about it.)
> 

What about just deleting the file after it's used, in PostRender?

> We can go ahead and delete the code that creates JSON expectations from images
> (because all of our checked-in expectations are now in JSON form), but sadly
the
> PreRender step still needs to generate an empty expected-results.json file for
> platforms where we have no expectations at all.  (e.g.
>
http://skiabot-master.pogerlabs.com:10117/builders/Test-ChromeOS-Alex-GMA3150...
> )  It may be useful to find some other way to handle that case (like, run gm
> without pointing at any expectations file in that case)
> 
> > I think we're a little inconsistent, and it's causing a problem: for
builders
> > who don't have attached devices, we need to decide whether the host-side
> > directories and device side directories are going to be the same (for every
> one)
> > or different (again, for every one).  The GM expectations dir seems to be
the
> > only one where the host-side dir and device-side dir can be different.  My
> > opinion is that we should make them the same across the board, and then the
> > copy/push functions can be no-ops which throw errors if they aren't the
same.
> 
> I think this is worth discussing.  Why do we need to mandate ANY relationship
> between host-side and device-side directories?  Wouldn't this work just fine? 
> PLEASE SEE ME.
> 
> class DefaultBuildStepUtils:
>   def CopyDirectoryContentsToDevice(self, host_dir, device_dir):
>     # There is no device, so it's just a local filecopy.
>     if host_dir != device_dir:
>       _LocalDirectoryCopy(host_dir, device_dir)
> 
> class AndroidBuildStepUtils(DefaultBuildStepUtils):
>   def CopyDirectoryContentsToDevice(self, host_dir, device_dir):
>     # Exactly as it is already implemented

Confusion caused by the inconsistency results in the need for CLs like this,
which shouldn't be at all necessary if we enforce that we have either:
1. a separate copy of everything to act as the device-side data
2. the device-side data IS the host-side data. (I think this is better, since it
saves us a lot of copying)

The whole point of adding the flavored build step utils was to avoid having
special checks like this in the build steps.

[epoger, 2013-08-28 18:21:54]

LGTM if you add the TODO described below

https://codereview.chromium.org/23495007/diff/6001/slave/skia_slave_scripts/p...
File slave/skia_slave_scripts/prerender.py (right):

https://codereview.chromium.org/23495007/diff/6001/slave/skia_slave_scripts/p...
slave/skia_slave_scripts/prerender.py:136: # For builders without an attached
device, PushFileToDevice will fail
Leon: please add a TODO referring to
https://code.google.com/p/skia/issues/detail?id=1571 ('build steps: make
PushFileToDevice() consistent with CopyDirectoryContentsToDevice()')... once I
remedy that bug, this new check can go away.

[scroggo, 2013-08-28 18:32:40]

https://codereview.chromium.org/23495007/diff/6001/slave/skia_slave_scripts/p...
File slave/skia_slave_scripts/prerender.py (right):

https://codereview.chromium.org/23495007/diff/6001/slave/skia_slave_scripts/p...
slave/skia_slave_scripts/prerender.py:136: # For builders without an attached
device, PushFileToDevice will fail
On 2013/08/28 18:21:54, epoger wrote:
> Leon: please add a TODO referring to
> https://code.google.com/p/skia/issues/detail?id=1571 ('build steps: make
> PushFileToDevice() consistent with CopyDirectoryContentsToDevice()')... once I
> remedy that bug, this new check can go away.

Done.

[epoger, 2013-08-28 18:37:26]

lgtm

[scroggo, 2013-08-28 18:40:59]

Committed patchset #3 manually as r10986 (presubmit successful).