WebRTC Android APK trybot recipe

WebRTC Android APK trybot recipe

This is now finally tested locally and seems to work.

Here's the workflow:
* Chromium src/ is synced ToT
* src/third_party/webrtc is synced to the revision property number.
* src/third_party/webrtc is patched with the patch, after the 
  webrtc/ paths have been filtered out (depends on 
  https://codereview.chromium.org/27575002)
* The custom WebRTC Android APK GYP targets are generated and compiled.
* Tests are executed on an Android device (on the same slave)

BUG=312827
TESTED=
Tested using
scripts/slave/unittests/recipe_configs_test.py --train
scripts/slave/unittests/recipes_test.py --train
and manual review of the .json files.
I've also done full builds using:
scripts/tools/run_recipe.py webrtc_android_apk_trybot BUILD_CONFIG=Release TARGET_PLATFORM=android TARGET_ARCH=arm patch_url=file:///path/to/valid/patch revision=5129

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

[kjellander_chromium, 2013-11-18 16:45:21]

I know the ANDROID_APK is really redundant right now, but as I plan to use the
same recipe for our normal build, I think it can be left in here.

I'm also considering keeping this odd build setup in a separate recipe module
too. Ideally I would be able to reuse more from a chromium_android module
instead, but all my attempts to use it in the current state ended up with a mess
and I simply gave up on that.

[iannucci, 2013-11-18 18:49:25]

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

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/gclient/api.py:101:
self.m.properties.get('revision')]
whoa! *bzzt* :p

(mentioned in chat)

why not just either:

c.solutions[0].custom_deps['src/third_party/webrtc'] = 'url@revision'

  OR

c.solutions[<whatever your new solution is>].revision = <revision>

?

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

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/api.py:37: ]
I would even just make this a class attribute NORMAL_TESTS

api.webrtc.NORMAL_TESTS

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/api.py:45: yield self.m.step('envsetup',
cmd, env=self._env)
envsetup makes me sad panda :(

It would be super cool if we could just call the NDK's envsetup directly, but
that's probably wishful thinking.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/api.py:49: self._env[key] = value
wdyt about putting this envsetup stuff in a 'base_android' module which can
contain all the sane/non-controversial android support tools? Then as we
discover more similarities/refactor craziness we can put them into base_android
until it's the only one left.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/api.py:52: kwargs['env'] = self._env
What do you think about 

kwargs.setdefault('env', self._env) 

or

old = kwargs.get('env', {})
kwargs['env'] = self._env.copy()
kwargs['env'].update(old)

?

I would at least add an

assert 'env' not in kwargs

So then callers don't get confused when they try to add env=... to the call.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/api.py:71: def runtests(self, test,
**kwargs):
This should probably be test_runner. ('runtests' instead of 'runtest' was a typo
I need to fix).

As much as possible, we try to keep the names of the functions matching the
names of the scripts that they call. Then when someone's thinking about
test_runner, they are more confident that the 'test_runner' method is what they
want.

Later when we consolidate all the testing scripts, we can pick a standard global
name, but for now there are so many different scripts which do slightly
different things that it's silly.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
File scripts/slave/recipe_modules/webrtc/chromium_config.py (right):

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/chromium_config.py:11:
@CONFIG_CTX(includes=['ninja', 'static_library'])
May want to check for / raise BadConf

TARGET_PLATFORM == 'android'
TARGET_ARCH == 'arm'
TARGET_BITS == 32

?

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/chromium_config.py:14:
c.gyp_env.GYP_CROSSCOMPILE = 1
Do you know when this is supposed to be set? We could theoretically have
chromium's BASE set this automatically. I suspect that its value is inconsistent
though.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/chromium_config.py:15:
c.gyp_env.GYP_GENERATORS.add('ninja')
This should already be set, since you're including 'ninja'

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/chromium_config.py:16:
c.gyp_env.GYP_GENERATOR_FLAGS['default_target'] = 'All'
this is redundant with compile_py.default_targets (or at least it should be)

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/chromium_config.py:19: gyp_defs['OS'] =
'android'
This gets me all the time... is 'OS' supposed to be the host OS, or the
destination OS?

Should we make the chromium module set OS == gyp_name_for(TARGET_PLATFORM)?

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
File scripts/slave/recipe_modules/webrtc/gclient_config.py (right):

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/gclient_config.py:8: ChromiumSvnSubURL
I'm 'meh' on this, but I really don't have a better way to do it :)

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/gclient_config.py:13: c.target_os =
['android']
I wonder if gclient's config.py should be informed by TARGET_PLATFORM to set
this automatically?

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipes/webrtc_...
File scripts/slave/recipes/webrtc_android_apk_trybot.py (right):

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipes/webrtc_...
scripts/slave/recipes/webrtc_android_apk_trybot.py:43: HOST_BITS=64,
In general, you should let the HOST_* be set automatically. You can 'simulate'
the host platform by adding e.g. api.platform('linux', 64) to your test (you'll
need to add platform to DEPS)

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipes/webrtc_...
scripts/slave/recipes/webrtc_android_apk_trybot.py:44: TARGET_PLATFORM='linux',
target platform should actually be 'android', right?

[kjellander_chromium, 2013-11-20 14:29:15]

refactored out some android stuff into base_android instead of the webrtc recipe
module (where it didn't make sense). 

PTAL

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

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/gclient/api.py:101:
self.m.properties.get('revision')]
On 2013/11/18 18:49:26, iannucci wrote:
> whoa! *bzzt* :p
> 
> (mentioned in chat)
> 
> why not just either:
> 
> c.solutions[0].custom_deps['src/third_party/webrtc'] = 'url@revision'
> 
>   OR
> 
> c.solutions[<whatever your new solution is>].revision = <revision>
> 
> ?

I agree. I moved it into the recipe.

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

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/api.py:37: ]
On 2013/11/18 18:49:26, iannucci wrote:
> I would even just make this a class attribute NORMAL_TESTS
> 
> api.webrtc.NORMAL_TESTS

Right, I don't remember why I put it into a method in the first place. I think I
used to pass a parameter or something...

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/api.py:45: yield self.m.step('envsetup',
cmd, env=self._env)
On 2013/11/18 18:49:26, iannucci wrote:
> envsetup makes me sad panda :(
> 
> It would be super cool if we could just call the NDK's envsetup directly, but
> that's probably wishful thinking.

I don't know the details about this. My primary target was to reproduce what's
currently running in our build waterfall for this trybot role.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/api.py:49: self._env[key] = value
On 2013/11/18 18:49:26, iannucci wrote:
> wdyt about putting this envsetup stuff in a 'base_android' module which can
> contain all the sane/non-controversial android support tools? Then as we
> discover more similarities/refactor craziness we can put them into
base_android
> until it's the only one left.

Yes, I'm doing that in the new patch set.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/api.py:52: kwargs['env'] = self._env
On 2013/11/18 18:49:26, iannucci wrote:
> What do you think about 
> 
> kwargs.setdefault('env', self._env) 
> 
> or
> 
> old = kwargs.get('env', {})
> kwargs['env'] = self._env.copy()
> kwargs['env'].update(old)
> 
> ?
> 
> I would at least add an
> 
> assert 'env' not in kwargs
> 
> So then callers don't get confused when they try to add env=... to the call.

For now, I'll rather skip kwargs and let someone add it when it's needed (if
ever).
That makes the code cleaner (I've done so in the base_android/api.py)

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/api.py:71: def runtests(self, test,
**kwargs):
On 2013/11/18 18:49:26, iannucci wrote:
> This should probably be test_runner. ('runtests' instead of 'runtest' was a
typo
> I need to fix).
> 
> As much as possible, we try to keep the names of the functions matching the
> names of the scripts that they call. Then when someone's thinking about
> test_runner, they are more confident that the 'test_runner' method is what
they
> want.
> 
> Later when we consolidate all the testing scripts, we can pick a standard
global
> name, but for now there are so many different scripts which do slightly
> different things that it's silly.

OK. I renamed it in the base_android/api.py

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
File scripts/slave/recipe_modules/webrtc/chromium_config.py (right):

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/chromium_config.py:11:
@CONFIG_CTX(includes=['ninja', 'static_library'])
On 2013/11/18 18:49:26, iannucci wrote:
> May want to check for / raise BadConf
> 
> TARGET_PLATFORM == 'android'
> TARGET_ARCH == 'arm'
> TARGET_BITS == 32
> 
> ?

I added platform and architecture checks only for the
webrtc_android_apk_try_builder. I'll leave the bits for now.
I moved the config into chromium/config.py instead.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/chromium_config.py:14:
c.gyp_env.GYP_CROSSCOMPILE = 1
On 2013/11/18 18:49:26, iannucci wrote:
> Do you know when this is supposed to be set? We could theoretically have
> chromium's BASE set this automatically. I suspect that its value is
inconsistent
> though.

I assume it's needed only when compiling for another target architecture than
the host (i.e. only Android for now)

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/chromium_config.py:15:
c.gyp_env.GYP_GENERATORS.add('ninja')
On 2013/11/18 18:49:26, iannucci wrote:
> This should already be set, since you're including 'ninja'

Done.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/chromium_config.py:16:
c.gyp_env.GYP_GENERATOR_FLAGS['default_target'] = 'All'
On 2013/11/18 18:49:26, iannucci wrote:
> this is redundant with compile_py.default_targets (or at least it should be)

Right, I guess All is the default if nothing is specified (this was copied from
chromium_android).

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/chromium_config.py:19: gyp_defs['OS'] =
'android'
On 2013/11/18 18:49:26, iannucci wrote:
> This gets me all the time... is 'OS' supposed to be the host OS, or the
> destination OS?
> 
> Should we make the chromium module set OS == gyp_name_for(TARGET_PLATFORM)?

I think it's the target OS in all cases, and the same as TARGET_PLATFORM, so
I'll give that a try.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
File scripts/slave/recipe_modules/webrtc/config.py (right):

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/config.py:14: apply_svn_patch = Single(bool,
required=False, empty_val=False),
(this wasn't used; leftover from chromium_android)
I'll leave it to the recipe to decide whether to add the apply_svn_patch step or
not, no need for a config variable for it.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
File scripts/slave/recipe_modules/webrtc/gclient_config.py (right):

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/gclient_config.py:8: ChromiumSvnSubURL
On 2013/11/18 18:49:26, iannucci wrote:
> I'm 'meh' on this, but I really don't have a better way to do it :)

Yeah, I guess such paths could be refactored into something more reusable.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/webrtc/gclient_config.py:13: c.target_os =
['android']
On 2013/11/18 18:49:26, iannucci wrote:
> I wonder if gclient's config.py should be informed by TARGET_PLATFORM to set
> this automatically?

I guess it could, but it would confuse a bit as well, since the target_os
variable is a Set, so using the TARGET_PLATFORM variable would only make it get
that value. I'll defer that change/discussion to a future CL.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipes/webrtc_...
File scripts/slave/recipes/webrtc_android_apk_trybot.py (right):

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipes/webrtc_...
scripts/slave/recipes/webrtc_android_apk_trybot.py:43: HOST_BITS=64,
On 2013/11/18 18:49:26, iannucci wrote:
> In general, you should let the HOST_* be set automatically. You can 'simulate'
> the host platform by adding e.g. api.platform('linux', 64) to your test
(you'll
> need to add platform to DEPS)

Ah, so that's how it works. I remember seeing the platform recipe module when I
started looking a recipes and I thought "great, finally moving out all that into
a single place". Then I forgot about it on the way... I'll be happy to use it
for what it's meant for.

https://codereview.chromium.org/75163006/diff/1/scripts/slave/recipes/webrtc_...
scripts/slave/recipes/webrtc_android_apk_trybot.py:44: TARGET_PLATFORM='linux',
On 2013/11/18 18:49:26, iannucci wrote:
> target platform should actually be 'android', right?

Yes.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/webrtc/gclient_config.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/webrtc/gclient_config.py:16:
'http://webrtc.googlecode.com/svn/trunk/webrtc')
I don't have a good way of getting the revision at this stage either, so I add
this and append the revision to it in the recipe. Not ideal but the best I could
come up with.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipes/web...
File scripts/slave/recipes/webrtc_android_apk_trybot.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipes/web...
scripts/slave/recipes/webrtc_android_apk_trybot.py:28:
api.gclient.c.solutions[0].revision = 'HEAD'
I figure the HEAD revision is needed to avoid it getting the properties'
revision in gclient.api.sync()?

[M-A Ruel, 2013-11-20 14:37:36]

(simple drive-by, don't plan to actually review)

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/base_android/api.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/base_android/api.py:28: for key, value in
env_diff.iteritems():
What you want is:
self._env.update((k, v) for k, v in env_diff.iteritems() if not
key.startswith('GYP_'))

it may require a wrapping dict(), I'm not 100% sure.

[iannucci, 2013-11-21 00:33:58]

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/base_android/api.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/base_android/api.py:20: if
self.m.chromium.c.TARGET_ARCH == 'intel':
I would add a @property to this class which converts TARGET_ARCH to the string
that envsetup expects (i.e. 'arm' or 'x86')

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/base_android/api.py:28: for key, value in
env_diff.iteritems():
On 2013/11/20 14:37:37, M-A Ruel wrote:
> What you want is:
> self._env.update((k, v) for k, v in env_diff.iteritems() if not
> key.startswith('GYP_'))
> 
> it may require a wrapping dict(), I'm not 100% sure.

+1

I think it just needs to be an iterable of pairs

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/chromium/config.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/chromium/config.py:225: c.gyp_env.GYP_CROSSCOMPILE
= 1
Should we just set this in BASE() any time HOST_PLATFORM != TARGET_PLATFORM?

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/chromium/config.py:233: gyp_defs['target_arch'] =
c.TARGET_ARCH
I think target_arch is now automatically handled for everything. Try rebasing on
HEAD.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/webrtc/config.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/webrtc/config.py:38: pass
You can omit this method entirely. root configs are optional.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/webrtc/gclient_config.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/webrtc/gclient_config.py:16:
'http://webrtc.googlecode.com/svn/trunk/webrtc')
On 2013/11/20 14:29:16, Henrik Kjellander wrote:
> I don't have a good way of getting the revision at this stage either, so I add
> this and append the revision to it in the recipe. Not ideal but the best I
could
> come up with.

If you look in DEPS, for webkit we have a webkit_trunk and a webkit_revision.
You should make webrtc do the same thing, and then you can tweak both of them as
custom_vars instead of custom_deps.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/webrtc/gclient_config.py:37: 'webrtc',
'webrtc.DEPS')
Why not just put this in the other method?

[kjellander_chromium, 2013-11-22 12:49:08]

Finally able to respond to this; I was Chrome sheriff yesterday and today.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/base_android/api.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/base_android/api.py:20: if
self.m.chromium.c.TARGET_ARCH == 'intel':
On 2013/11/21 00:33:59, iannucci wrote:
> I would add a @property to this class which converts TARGET_ARCH to the string
> that envsetup expects (i.e. 'arm' or 'x86') 

Done.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/base_android/api.py:28: for key, value in
env_diff.iteritems():
On 2013/11/21 00:33:59, iannucci wrote:
> On 2013/11/20 14:37:37, M-A Ruel wrote:
> > What you want is:
> > self._env.update((k, v) for k, v in env_diff.iteritems() if not
> > key.startswith('GYP_'))
> > 
> > it may require a wrapping dict(), I'm not 100% sure.
> 
> +1
> 
> I think it just needs to be an iterable of pairs

Done.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/chromium/config.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/chromium/config.py:225: c.gyp_env.GYP_CROSSCOMPILE
= 1
On 2013/11/21 00:33:59, iannucci wrote:
> Should we just set this in BASE() any time HOST_PLATFORM != TARGET_PLATFORM?

That is an excellent idea. I'm not 100% sure it's always needed since I don't
know where GYP_CROSSCOMPILE is used, but let's do it like that for now.
Looking at the updated config files, there were many different configurations
that now got this flag set. I'm not sure that's desired?

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/chromium/config.py:233: gyp_defs['target_arch'] =
c.TARGET_ARCH
On 2013/11/21 00:33:59, iannucci wrote:
> I think target_arch is now automatically handled for everything. Try rebasing
on
> HEAD.

Ah, that's great.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/webrtc/config.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/webrtc/config.py:38: pass
On 2013/11/21 00:33:59, iannucci wrote:
> You can omit this method entirely. root configs are optional.

Done.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/webrtc/gclient_config.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/webrtc/gclient_config.py:16:
'http://webrtc.googlecode.com/svn/trunk/webrtc')
On 2013/11/21 00:33:59, iannucci wrote:
> On 2013/11/20 14:29:16, Henrik Kjellander wrote:
> > I don't have a good way of getting the revision at this stage either, so I
add
> > this and append the revision to it in the recipe. Not ideal but the best I
> could
> > come up with.
> 
> If you look in DEPS, for webkit we have a webkit_trunk and a webkit_revision.
> You should make webrtc do the same thing, and then you can tweak both of them
as
> custom_vars instead of custom_deps.

It's correct we have a webrtc_revision variable, but we don't have one for
webrtc_trunk unfortunately. We could create one and use that approach, but as
we're about to retire the WebRTC stable branch and actually point to trunk, I
think it's better to wait for that to happen and then use webrtc_revision as you
suggest. Until then I think this is the way to go.
I added a TODO for it here and in the recipe.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/webrtc/gclient_config.py:37: 'webrtc',
'webrtc.DEPS')
On 2013/11/21 00:33:59, iannucci wrote:
> Why not just put this in the other method?

I plan to use it for all our "normal" bots later on. webrtc.DEPS is actually
used for more than Android nowadays too (so I clarified that comment).

[kjellander_chromium, 2013-11-22 12:49:09]

Finally able to respond to this; I was Chrome sheriff yesterday and today.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/base_android/api.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/base_android/api.py:20: if
self.m.chromium.c.TARGET_ARCH == 'intel':
On 2013/11/21 00:33:59, iannucci wrote:
> I would add a @property to this class which converts TARGET_ARCH to the string
> that envsetup expects (i.e. 'arm' or 'x86') 

Done.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/base_android/api.py:28: for key, value in
env_diff.iteritems():
On 2013/11/21 00:33:59, iannucci wrote:
> On 2013/11/20 14:37:37, M-A Ruel wrote:
> > What you want is:
> > self._env.update((k, v) for k, v in env_diff.iteritems() if not
> > key.startswith('GYP_'))
> > 
> > it may require a wrapping dict(), I'm not 100% sure.
> 
> +1
> 
> I think it just needs to be an iterable of pairs

Done.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/chromium/config.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/chromium/config.py:225: c.gyp_env.GYP_CROSSCOMPILE
= 1
On 2013/11/21 00:33:59, iannucci wrote:
> Should we just set this in BASE() any time HOST_PLATFORM != TARGET_PLATFORM?

That is an excellent idea. I'm not 100% sure it's always needed since I don't
know where GYP_CROSSCOMPILE is used, but let's do it like that for now.
Looking at the updated config files, there were many different configurations
that now got this flag set. I'm not sure that's desired?

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/chromium/config.py:233: gyp_defs['target_arch'] =
c.TARGET_ARCH
On 2013/11/21 00:33:59, iannucci wrote:
> I think target_arch is now automatically handled for everything. Try rebasing
on
> HEAD.

Ah, that's great.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/webrtc/config.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/webrtc/config.py:38: pass
On 2013/11/21 00:33:59, iannucci wrote:
> You can omit this method entirely. root configs are optional.

Done.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/webrtc/gclient_config.py (right):

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/webrtc/gclient_config.py:16:
'http://webrtc.googlecode.com/svn/trunk/webrtc')
On 2013/11/21 00:33:59, iannucci wrote:
> On 2013/11/20 14:29:16, Henrik Kjellander wrote:
> > I don't have a good way of getting the revision at this stage either, so I
add
> > this and append the revision to it in the recipe. Not ideal but the best I
> could
> > come up with.
> 
> If you look in DEPS, for webkit we have a webkit_trunk and a webkit_revision.
> You should make webrtc do the same thing, and then you can tweak both of them
as
> custom_vars instead of custom_deps.

It's correct we have a webrtc_revision variable, but we don't have one for
webrtc_trunk unfortunately. We could create one and use that approach, but as
we're about to retire the WebRTC stable branch and actually point to trunk, I
think it's better to wait for that to happen and then use webrtc_revision as you
suggest. Until then I think this is the way to go.
I added a TODO for it here and in the recipe.

https://codereview.chromium.org/75163006/diff/90001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/webrtc/gclient_config.py:37: 'webrtc',
'webrtc.DEPS')
On 2013/11/21 00:33:59, iannucci wrote:
> Why not just put this in the other method?

I plan to use it for all our "normal" bots later on. webrtc.DEPS is actually
used for more than Android nowadays too (so I clarified that comment).

[iannucci, 2013-12-05 22:15:10]

lgtm

[commit-bot: I haz the power, 2013-12-16 15:03:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kjellander@chromium.org/75163006/960001

[commit-bot: I haz the power, 2013-12-16 15:05:17]

Change committed as 240894