Added support for properly building and running isolates in recipes.

Added support for properly building and running isolates in recipes.

Switched the GPU recipe to run gl_tests via an isolate. Will convert
remaining tests to run via isolates once this is working well on the bots.

In addition to recipes_test, tested by running gpu/build_and_test recipe
locally on Linux. Isolate is properly uploaded to the server during the
build steps and executed via run_isolated.py during the test steps.

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

[Ken Russell (switch to Gerrit), 2014-01-10 23:29:33]

Robbie: please review. Thanks.

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/gpu/api.py (left):

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/gpu/api.py:125: yield
self.m.chromium.runtests(test, spawn_dbus=True)
Note that the script now spawns dbus by default, so this was useless.

[iannucci, 2014-01-10 23:50:25]

looks pretty good, just a few comments.

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/isolate/api.py (right):

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:7: ISOLATE_SERVER =
'https://isolateserver.appspot.com'
theoretically, this (and the test_isolation_mode->hashtable) should go in a
config for this module.
practically, this seems like a perfect thing to refactor later when we have more
users and other values for these.

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:47: return
self.m.properties.get('swarm_hashes', self._manifest_hashes)
Nice :)

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:54: parallel=False, **kwargs):
You probably should let all of these be taken care of by **kwargs.

def ...(self, test, args=(), **runtest_kwargs):
  ... runtests(..., **runtest_kwargs)

Otherwise you have a potentially conflicting set of default values, and a
duplication of arg names. Also verbose :).

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:61: name = test
idiomatically, I tend to prefer:
  name = name or test

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:70: args = []
If you make args default to an empty tuple `()`, you don't need this.

If you tried to set the default to [] and got pylint yelling at you, the reason
is that default arguments are evaluated at function definition time, which means
the args default value would be a mutable global across all invocations of this
method (icky).

Unfortunately there are no frozen mapping types in python, so if args were
supposed to be a dict, you'd want to use this idiom:

def ...(..., args=None, ...):
  args = args or {}

[Ken Russell (switch to Gerrit), 2014-01-11 00:35:38]

Thanks for your review and feedback. Addressed it.

Unfortunately, as we've just discussed offline, this CL won't work on the GPU
try servers, because they use the component build. Please hold off giving a
thumbs-up until I submit another version fixing that issue.

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/isolate/api.py (right):

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:7: ISOLATE_SERVER =
'https://isolateserver.appspot.com'
On 2014/01/10 23:50:25, iannucci wrote:
> theoretically, this (and the test_isolation_mode->hashtable) should go in a
> config for this module.
> practically, this seems like a perfect thing to refactor later when we have
more
> users and other values for these.

Yes, let's keep this simple for now and refactor it later.

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:54: parallel=False, **kwargs):
On 2014/01/10 23:50:25, iannucci wrote:
> You probably should let all of these be taken care of by **kwargs.
> 
> def ...(self, test, args=(), **runtest_kwargs):
>   ... runtests(..., **runtest_kwargs)
> 
> Otherwise you have a potentially conflicting set of default values, and a
> duplication of arg names. Also verbose :).

Thanks. Fixed this.

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:61: name = test
On 2014/01/10 23:50:25, iannucci wrote:
> idiomatically, I tend to prefer:
>   name = name or test

Done.

https://codereview.chromium.org/132853003/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/isolate/api.py:70: args = []
On 2014/01/10 23:50:25, iannucci wrote:
> If you make args default to an empty tuple `()`, you don't need this.
> 
> If you tried to set the default to [] and got pylint yelling at you, the
reason
> is that default arguments are evaluated at function definition time, which
means
> the args default value would be a mutable global across all invocations of
this
> method (icky).
> 
> Unfortunately there are no frozen mapping types in python, so if args were
> supposed to be a dict, you'd want to use this idiom:
> 
> def ...(..., args=None, ...):
>   args = args or {}

Unfortunately, [] + () does not work. Leaving this as is for now.

[Ken Russell (switch to Gerrit), 2014-01-11 03:07:11]

Please re-review. It turns out all of the Debug bots use the component build, so
it was easy to determine whether to attempt to use isolates.

I'd like to land this, and proceed with switching the Release bots to use
isolates for all the tests, while the work is in progress to make isolates work
with the component build.

Local re-testing of this recipe is still in progress.

[Ken Russell (switch to Gerrit), 2014-01-13 19:46:39]

On 2014/01/11 03:07:11, Ken Russell wrote:
> Please re-review. It turns out all of the Debug bots use the component build,
so
> it was easy to determine whether to attempt to use isolates.
> 
> I'd like to land this, and proceed with switching the Release bots to use
> isolates for all the tests, while the work is in progress to make isolates
work
> with the component build.
> 
> Local re-testing of this recipe is still in progress.

This CL passed local testing. Please re-review. Thanks.

[iannucci, 2014-01-13 21:24:29]

lgtm

https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/gpu/api.py (right):

https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/gpu/api.py:65: self._use_isolates =
self.m.chromium.is_release_build
I would actually make _use_isolates be an @property, so it always has the
correct value, even if you don't call setup().

https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipes/gp...
File scripts/slave/recipes/gpu/download_and_test.expected/win_release.json
(right):

https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipes/gp...
scripts/slave/recipes/gpu/download_and_test.expected/win_release.json:318:
"--builder-name=win release tester",
Hm. This seems to be quoted wrong. Probably a bug in the test output.

[Ken Russell (switch to Gerrit), 2014-01-13 22:08:19]

https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/gpu/api.py (right):

https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/gpu/api.py:65: self._use_isolates =
self.m.chromium.is_release_build
On 2014/01/13 21:24:30, iannucci wrote:
> I would actually make _use_isolates be an @property, so it always has the
> correct value, even if you don't call setup().

After thinking more about this I'm going to leave it as is. There are other
required side effects of calling GpuApi.setup(). We can revisit this in the
future.

https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipes/gp...
File scripts/slave/recipes/gpu/download_and_test.expected/win_release.json
(right):

https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipes/gp...
scripts/slave/recipes/gpu/download_and_test.expected/win_release.json:318:
"--builder-name=win release tester",
On 2014/01/13 21:24:30, iannucci wrote:
> Hm. This seems to be quoted wrong. Probably a bug in the test output.

Should I file a bug about this? It's been the case for a while. I think the real
runs on the bots are working.

[commit-bot: I haz the power, 2014-01-13 22:16:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/132853003/70001

[commit-bot: I haz the power, 2014-01-13 22:17:21]

Change committed as 244596

[iannucci, 2014-01-14 01:24:51]

https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/gpu/api.py (right):

https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/gpu/api.py:65: self._use_isolates =
self.m.chromium.is_release_build
On 2014/01/13 22:08:20, Ken Russell wrote:
> On 2014/01/13 21:24:30, iannucci wrote:
> > I would actually make _use_isolates be an @property, so it always has the
> > correct value, even if you don't call setup().
> 
> After thinking more about this I'm going to leave it as is. There are other
> required side effects of calling GpuApi.setup(). We can revisit this in the
> future.

Right, but if the chromium config changes, this value will also change... it's
complicated :p.

No big deal.

https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipes/gp...
File scripts/slave/recipes/gpu/download_and_test.expected/win_release.json
(right):

https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipes/gp...
scripts/slave/recipes/gpu/download_and_test.expected/win_release.json:318:
"--builder-name=win release tester",
On 2014/01/13 22:08:20, Ken Russell wrote:
> On 2014/01/13 21:24:30, iannucci wrote:
> > Hm. This seems to be quoted wrong. Probably a bug in the test output.
> 
> Should I file a bug about this? It's been the case for a while. I think the
real
> runs on the bots are working.

Yes, please.

[Ken Russell (switch to Gerrit), 2014-01-15 07:18:28]

On 2014/01/14 01:24:51, iannucci wrote:
>
https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipe_mod...
> File scripts/slave/recipe_modules/gpu/api.py (right):
> 
>
https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipe_mod...
> scripts/slave/recipe_modules/gpu/api.py:65: self._use_isolates =
> self.m.chromium.is_release_build
> On 2014/01/13 22:08:20, Ken Russell wrote:
> > On 2014/01/13 21:24:30, iannucci wrote:
> > > I would actually make _use_isolates be an @property, so it always has the
> > > correct value, even if you don't call setup().
> > 
> > After thinking more about this I'm going to leave it as is. There are other
> > required side effects of calling GpuApi.setup(). We can revisit this in the
> > future.
> 
> Right, but if the chromium config changes, this value will also change... it's
> complicated :p.
> 
> No big deal.
> 
>
https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipes/gp...
> File scripts/slave/recipes/gpu/download_and_test.expected/win_release.json
> (right):
> 
>
https://codereview.chromium.org/132853003/diff/70001/scripts/slave/recipes/gp...
> scripts/slave/recipes/gpu/download_and_test.expected/win_release.json:318:
> "--builder-name=win release tester",
> On 2014/01/13 22:08:20, Ken Russell wrote:
> > On 2014/01/13 21:24:30, iannucci wrote:
> > > Hm. This seems to be quoted wrong. Probably a bug in the test output.
> > 
> > Should I file a bug about this? It's been the case for a while. I think the
> real
> > runs on the bots are working.
> 
> Yes, please.

Filed http://crbug.com/334552 .