CT Skia recipe to run DM on the top 10k SKPs using swarming

CT Skia recipe to run DM on the top 10k SKPs using swarming.

Uses the isolate file and script in https://codereview.chromium.org/1469823003/

This recipe is similar to the one added for Chromium Perf: https://codereview.chromium.org/1423993007/

BUG=skia:2545
TBR=iannucci

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=297734

[rmistry, 2015-11-24 18:22:48]

Description was changed from

==========
Checkpoint for running DM on 10k SKPs

BUG=
==========

to

==========
Checkpoint for running DM on 10k SKPs

BUG=skia:2545
==========

[rmistry, 2015-11-24 18:25:50]

Description was changed from

==========
Checkpoint for running DM on 10k SKPs

BUG=skia:2545
==========

to

==========
CT Skia recipe to run DM on the top 10k SKPs using swarming.

This recipe is similar to the one added for Chromium Perf:
https://codereview.chromium.org/1423993007/

BUG=skia:2545
==========

[rmistry, 2015-11-24 18:27:51]

Description was changed from

==========
CT Skia recipe to run DM on the top 10k SKPs using swarming.

This recipe is similar to the one added for Chromium Perf:
https://codereview.chromium.org/1423993007/

BUG=skia:2545
==========

to

==========
CT Skia recipe to run DM on the top 10k SKPs using swarming.

Uses the isolate file and script in https://codereview.chromium.org/1469823003/

This recipe is similar to the one added for Chromium Perf:
https://codereview.chromium.org/1423993007/

BUG=skia:2545
==========

[rmistry, 2015-11-24 18:38:04]

rmistry@google.com changed reviewers:
+ borenet@google.com

[rmistry, 2015-11-24 18:38:13]

The CQ bit was checked by rmistry@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-24 18:38:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441173003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441173003/200001

[commit-bot: I haz the power, 2015-11-24 18:41:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-24 18:41:08]

Dry run: This issue passed the CQ dry run.

[borenet, 2015-11-24 19:34:28]

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/ct_swarming/api.py (right):

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/ct_swarming/api.py:100:
self.m.file.makedirs('Create SKPs dir', skps_dir)
Should you delete the directory before creating it, in case there are leftovers
from a previous build?

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
File scripts/slave/recipes/skia/ct_10k_skps_dm.py (right):

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.py:52: api.path['checkout'] =
skia_checkout
This seems dangerous, but I can't think of an example where it would cause
problems.  Why is this needed?  I think if you specify the Skia checkout first,
api.path['checkout'] may be set to the Skia checkout without needing to override
it.

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.py:56: flavor.compile('dm')
It's probably better to just run the compile command directly, rather than
trying to use what the Skia recipe module does.

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.py:63: for f in ['run_ct_dm.py']:
Why is the loop needed?

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.py:73: 'skps_chromium_build',
DEFAULT_SKPS_CHROMIUM_BUILD)
How will this be specified?

[rmistry, 2015-11-24 19:58:41]

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/ct_swarming/api.py (right):

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/ct_swarming/api.py:100:
self.m.file.makedirs('Create SKPs dir', skps_dir)
On 2015/11/24 19:34:28, borenet wrote:
> Should you delete the directory before creating it, in case there are
leftovers
> from a previous build?

Sure. Done.

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
File scripts/slave/recipes/skia/ct_10k_skps_dm.py (right):

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.py:52: api.path['checkout'] =
skia_checkout
On 2015/11/24 19:34:28, borenet wrote:
> This seems dangerous, but I can't think of an example where it would cause
> problems.  Why is this needed?  I think if you specify the Skia checkout
first,
> api.path['checkout'] may be set to the Skia checkout without needing to
override
> it.

Specified Skia checkout first and removed this.

Why is this needed is more complicated, here are some words:
I ran into a lot of problems trying to make this work. This because both skia
and different chromium recipes rely to api.path['checkout'] to point to
different places. Chromium recipes assume that checkout points to Chromium dir,
Skia assumes that checkout points to Skia. The ideal solution for me would be
that skia/api.py created a api.path['skia'] similar to what some other recipes
do. But that will likely need changes in many places.

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.py:56: flavor.compile('dm')
On 2015/11/24 19:34:28, borenet wrote:
> It's probably better to just run the compile command directly, rather than
> trying to use what the Skia recipe module does.

Compile directly from where? do you mean api.skia.flavor.compile ? If so then I
could not do that because flavor was initialized in gen_steps (which is not
called) and so was self.builder_cfg.

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.py:63: for f in ['run_ct_dm.py']:
On 2015/11/24 19:34:28, borenet wrote:
> Why is the loop needed?

This section is temporary and will be removed. The loop is not needed right now
but it makes adding new files easy if necessary.

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.py:73: 'skps_chromium_build',
DEFAULT_SKPS_CHROMIUM_BUILD)
On 2015/11/24 19:34:28, borenet wrote:
> How will this be specified?

Will always probably be hardcoded here, made it set from properties for local
testing.

[borenet, 2015-11-24 20:06:40]

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
File scripts/slave/recipes/skia/ct_10k_skps_dm.py (right):

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.py:56: flavor.compile('dm')
On 2015/11/24 19:58:41, rmistry wrote:
> On 2015/11/24 19:34:28, borenet wrote:
> > It's probably better to just run the compile command directly, rather than
> > trying to use what the Skia recipe module does.
> 
> Compile directly from where? do you mean api.skia.flavor.compile ? If so then
I
> could not do that because flavor was initialized in gen_steps (which is not
> called) and so was self.builder_cfg.

I mean just directly run "make dm" instead of trying to use the Skia recipe
module's flavor utils, which aren't needed here.  That way, you don't even need
the Skia DEP.

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.py:63: for f in ['run_ct_dm.py']:
On 2015/11/24 19:58:41, rmistry wrote:
> On 2015/11/24 19:34:28, borenet wrote:
> > Why is the loop needed?
> 
> This section is temporary and will be removed. The loop is not needed right
now
> but it makes adding new files easy if necessary.

Acknowledged.

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.py:73: 'skps_chromium_build',
DEFAULT_SKPS_CHROMIUM_BUILD)
On 2015/11/24 19:58:41, rmistry wrote:
> On 2015/11/24 19:34:28, borenet wrote:
> > How will this be specified?
> 
> Will always probably be hardcoded here, made it set from properties for local
> testing.

Acknowledged.

[rmistry, 2015-11-25 14:00:03]

Patchset #13 (id:240001) has been deleted

[rmistry, 2015-11-25 14:09:22]

Patchset #12 (id:220001) has been deleted

[rmistry, 2015-11-25 14:09:29]

Patchset #12 (id:250001) has been deleted

[rmistry, 2015-11-25 14:11:21]

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
File scripts/slave/recipes/skia/ct_10k_skps_dm.py (right):

https://codereview.chromium.org/1441173003/diff/200001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.py:56: flavor.compile('dm')
On 2015/11/24 20:06:40, borenet wrote:
> On 2015/11/24 19:58:41, rmistry wrote:
> > On 2015/11/24 19:34:28, borenet wrote:
> > > It's probably better to just run the compile command directly, rather than
> > > trying to use what the Skia recipe module does.
> > 
> > Compile directly from where? do you mean api.skia.flavor.compile ? If so
then
> I
> > could not do that because flavor was initialized in gen_steps (which is not
> > called) and so was self.builder_cfg.
> 
> I mean just directly run "make dm" instead of trying to use the Skia recipe
> module's flavor utils, which aren't needed here.  That way, you don't even
need
> the Skia DEP.

Oh, makes sense. Done. Much better now, I could remove the skia_checkout var
too.

[borenet, 2015-11-30 12:25:48]

LGTM with nit/note.

https://codereview.chromium.org/1441173003/diff/270001/scripts/slave/recipes/...
File scripts/slave/recipes/skia/ct_10k_skps_dm.expected/CT_DM_10k_SKPs.json
(right):

https://codereview.chromium.org/1441173003/diff/270001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.expected/CT_DM_10k_SKPs.json:345:
"name": "rmtree Clean SKPs dir (3)",
Just wanted to point this out: rmtree and makedirs get prepended to their step
names.  You might want to rename the step so that it ends up like, eg. "rmtree
skps dir".  Don't include the path in the name, or the server will get confused
and be unable to serve the logs for the step.

[rmistry, 2015-11-30 13:12:32]

Patchset #13 (id:290001) has been deleted

[rmistry, 2015-11-30 13:13:20]

https://codereview.chromium.org/1441173003/diff/270001/scripts/slave/recipes/...
File scripts/slave/recipes/skia/ct_10k_skps_dm.expected/CT_DM_10k_SKPs.json
(right):

https://codereview.chromium.org/1441173003/diff/270001/scripts/slave/recipes/...
scripts/slave/recipes/skia/ct_10k_skps_dm.expected/CT_DM_10k_SKPs.json:345:
"name": "rmtree Clean SKPs dir (3)",
On 2015/11/30 12:25:48, borenet wrote:
> Just wanted to point this out: rmtree and makedirs get prepended to their step
> names.  You might want to rename the step so that it ends up like, eg. "rmtree
> skps dir".  Don't include the path in the name, or the server will get
confused
> and be unable to serve the logs for the step.

Done.

[rmistry, 2015-11-30 13:17:06]

The CQ bit was checked by rmistry@google.com

[rmistry, 2015-11-30 13:17:06]

The patchset sent to the CQ was uploaded after l-g-t-m from borenet@google.com
Link to the patchset: https://codereview.chromium.org/1441173003/#ps310001
(title: "Improve step names")

[commit-bot: I haz the power, 2015-11-30 13:17:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441173003/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441173003/310001

[commit-bot: I haz the power, 2015-11-30 13:28:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-30 13:28:29]

Try jobs failed on following builders:
  Build Presubmit on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)

[rmistry, 2015-11-30 13:35:29]

The CQ bit was checked by rmistry@google.com

[rmistry, 2015-11-30 13:35:30]

The patchset sent to the CQ was uploaded after l-g-t-m from borenet@google.com
Link to the patchset: https://codereview.chromium.org/1441173003/#ps330001
(title: "Rebase expectations due to step name change")

[commit-bot: I haz the power, 2015-11-30 13:35:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441173003/330001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441173003/330001

[rmistry1, 2015-11-30 13:42:42]

The CQ bit was unchecked by rmistry@chromium.org

[rmistry1, 2015-11-30 13:42:52]

rmistry@chromium.org changed reviewers:
+ rmistry@chromium.org

[rmistry1, 2015-11-30 13:42:53]

lgtm

[rmistry1, 2015-11-30 13:42:58]

The CQ bit was checked by rmistry@chromium.org

[commit-bot: I haz the power, 2015-11-30 13:43:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441173003/330001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441173003/330001

[rmistry, 2015-11-30 13:51:46]

The CQ bit was unchecked by rmistry@google.com

[rmistry, 2015-11-30 13:52:02]

Description was changed from

==========
CT Skia recipe to run DM on the top 10k SKPs using swarming.

Uses the isolate file and script in https://codereview.chromium.org/1469823003/

This recipe is similar to the one added for Chromium Perf:
https://codereview.chromium.org/1423993007/

BUG=skia:2545
==========

to

==========
CT Skia recipe to run DM on the top 10k SKPs using swarming.

Uses the isolate file and script in https://codereview.chromium.org/1469823003/

This recipe is similar to the one added for Chromium Perf:
https://codereview.chromium.org/1423993007/

BUG=skia:2545
TBR=iannucci
==========

[rmistry, 2015-11-30 13:52:30]

rmistry@google.com changed reviewers:
+ iannucci@chromium.org

[rmistry, 2015-11-30 13:52:31]

Trybot failing due to-

Missing LGTM from an OWNER for these files:
    scripts/slave/recipes/perf/ct_top1k_perf.expected/CT_Top1k_RR.json
    scripts/slave/recipes/perf/ct_top1k_perf.expected/CT_Top1k_Repaint.json
   
scripts/slave/recipes/perf/ct_top1k_perf.expected/CT_Top1k_slave3_failure.json

These are the expectations I originally added in
https://codereview.chromium.org/1423993007/. Adding iannucci as TBRer.

[rmistry, 2015-11-30 13:52:40]

The CQ bit was checked by rmistry@google.com

[commit-bot: I haz the power, 2015-11-30 13:52:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441173003/330001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441173003/330001

[commit-bot: I haz the power, 2015-11-30 13:55:55]

Description was changed from

==========
CT Skia recipe to run DM on the top 10k SKPs using swarming.

Uses the isolate file and script in https://codereview.chromium.org/1469823003/

This recipe is similar to the one added for Chromium Perf:
https://codereview.chromium.org/1423993007/

BUG=skia:2545
TBR=iannucci
==========

to

==========
CT Skia recipe to run DM on the top 10k SKPs using swarming.

Uses the isolate file and script in https://codereview.chromium.org/1469823003/

This recipe is similar to the one added for Chromium Perf:
https://codereview.chromium.org/1423993007/

BUG=skia:2545
TBR=iannucci

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=297734
==========

[commit-bot: I haz the power, 2015-11-30 13:55:55]

Committed patchset #14 (id:330001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=297734