Fix compile on the Blimp Client Engine Integration bot

Fix compile on the Blimp Client Engine Integration bot

BUG=655864
Committed: https://chromium.googlesource.com/chromium/tools/build/+/61728af22d26f33d09ed715d0c43f6051ee9c1ea

[shenghuazhang, 2016-10-14 01:12:10]

shenghuazhang@chromium.org changed reviewers:
+ mikecase@chromium.org

[mikecase (-- gone --), 2016-10-14 02:25:36]

https://codereview.chromium.org/2415963004/diff/1/scripts/slave/recipes/blimp...
File scripts/slave/recipes/blimp/integration.py (right):

https://codereview.chromium.org/2415963004/diff/1/scripts/slave/recipes/blimp...
scripts/slave/recipes/blimp/integration.py:42: android_build =
api.path['checkout'].join('src', 'out-android')
need to change these paths to api.path['checkout'].join('out-android')

remove the src/

If you look in the expectation files there is src/src/

https://codereview.chromium.org/2415963004/diff/1/scripts/slave/recipes/blimp...
scripts/slave/recipes/blimp/integration.py:54:
api.chromium.set_config('chromium')
The fix you want is to delete this line. Looks like it sets "Release" by default
and overrides the fact Debug is set by your call to
api.chromium_android.configure_from_properties().

https://codereview.chromium.org/2415963004/diff/1/scripts/slave/recipes/blimp...
scripts/slave/recipes/blimp/integration.py:67: target=builder['target'],
you don't need to specify this explicitly. This does fix the problem, but isnt
the fix you want/

[shenghuazhang, 2016-10-14 03:21:19]

Switch calling sequence for chromium.set_config and
api.chromium_android.configure_from_properties.

https://codereview.chromium.org/2415963004/diff/1/scripts/slave/recipes/blimp...
File scripts/slave/recipes/blimp/integration.py (right):

https://codereview.chromium.org/2415963004/diff/1/scripts/slave/recipes/blimp...
scripts/slave/recipes/blimp/integration.py:42: android_build =
api.path['checkout'].join('src', 'out-android')
On 2016/10/14 02:25:36, mikecase wrote:
> need to change these paths to api.path['checkout'].join('out-android')
> 
> remove the src/
> 
> If you look in the expectation files there is src/src/

Done.

https://codereview.chromium.org/2415963004/diff/1/scripts/slave/recipes/blimp...
scripts/slave/recipes/blimp/integration.py:54:
api.chromium.set_config('chromium')
On 2016/10/14 02:25:36, mikecase wrote:
> The fix you want is to delete this line. Looks like it sets "Release" by
default
> and overrides the fact Debug is set by your call to
> api.chromium_android.configure_from_properties().

Ohhh yes you are right... Or, maybe could call chromium.set_config  before
api.chromium_android.configure_from_properties()? I found something similar
like:
https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium/...

https://codereview.chromium.org/2415963004/diff/1/scripts/slave/recipes/blimp...
scripts/slave/recipes/blimp/integration.py:67: target=builder['target'],
On 2016/10/14 02:25:36, mikecase wrote:
> you don't need to specify this explicitly. This does fix the problem, but isnt
> the fix you want/

Done.

[shenghuazhang, 2016-10-14 03:25:32]

https://codereview.chromium.org/2415963004/diff/40001/scripts/slave/recipes/b...
File
scripts/slave/recipes/blimp/integration.expected/Blimp_Android_Tester_test_pass.json
(right):

https://codereview.chromium.org/2415963004/diff/40001/scripts/slave/recipes/b...
scripts/slave/recipes/blimp/integration.expected/Blimp_Android_Tester_test_pass.json:217:
"[SLAVE_BUILD]/src/out-linux"
Wait... it causes more changes like this. 
Probably it's better to remove api.chromium.set_config('chromium'), rather than
switch place, in case it causes more effects? 
But, not sure any shortage yet if without the chromium.set_config.

[shenghuazhang, 2016-10-14 03:37:10]

https://codereview.chromium.org/2415963004/diff/40001/scripts/slave/recipes/b...
File
scripts/slave/recipes/blimp/integration.expected/Blimp_Android_Tester_test_pass.json
(right):

https://codereview.chromium.org/2415963004/diff/40001/scripts/slave/recipes/b...
scripts/slave/recipes/blimp/integration.expected/Blimp_Android_Tester_test_pass.json:217:
"[SLAVE_BUILD]/src/out-linux"
On 2016/10/14 03:25:32, shenghuazhang1 wrote:
> Wait... it causes more changes like this. 
> Probably it's better to remove api.chromium.set_config('chromium'), rather
than
> switch place, in case it causes more effects? 
> But, not sure any shortage yet if without the chromium.set_config.

Oh not right.. I know what happened here. The missing 'Debug' was caused due to
change of 'linux_build' definition, rather than chromium.set_config...

I.e. though defined BUILD_CONFIG=builder['target'] (which is 'Debug') via
api.chromium_android.configure_from_properties, still needs to define
linux_build as api.path['checkout'].join('out-linux', 'Debug'), rather than
api.path['checkout'].join('out-linux')...?

Sorry for the confusing... I don't know if I have explained this clearly...

[mikecase (-- gone --), 2016-10-14 16:48:34]

https://codereview.chromium.org/2415963004/diff/40001/scripts/slave/recipes/b...
File scripts/slave/recipes/blimp/integration.py (right):

https://codereview.chromium.org/2415963004/diff/40001/scripts/slave/recipes/b...
scripts/slave/recipes/blimp/integration.py:64: build_dir=linux_build,
You should probably add the "Debug" back here.

https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium/...

I think a fix for these is to set the following for the build dirs.

  android_build = api.path['checkout'].join('out-android',
api.chromium.c.build_config_fs)
  linux_build = api.path['checkout'].join('out-linux',
api.chromium.c.build_config_fs)

Since you use the Chromium config in these paths, you will have to set these
paths AFTER setting all the chromium configs.

[shenghuazhang, 2016-10-14 18:32:25]

https://codereview.chromium.org/2415963004/diff/40001/scripts/slave/recipes/b...
File scripts/slave/recipes/blimp/integration.py (right):

https://codereview.chromium.org/2415963004/diff/40001/scripts/slave/recipes/b...
scripts/slave/recipes/blimp/integration.py:64: build_dir=linux_build,
On 2016/10/14 16:48:34, mikecase wrote:
> You should probably add the "Debug" back here.
> 
>
https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium/...
> 
> I think a fix for these is to set the following for the build dirs.
> 
>   android_build = api.path['checkout'].join('out-android',
> api.chromium.c.build_config_fs)
>   linux_build = api.path['checkout'].join('out-linux',
> api.chromium.c.build_config_fs)
> 
> Since you use the Chromium config in these paths, you will have to set these
> paths AFTER setting all the chromium configs.

Done.

[mikecase (-- gone --), 2016-10-14 19:04:14]

looks good. Just 1 nit to fix.

https://codereview.chromium.org/2415963004/diff/60001/scripts/slave/recipes/b...
File scripts/slave/recipes/blimp/integration.py (right):

https://codereview.chromium.org/2415963004/diff/60001/scripts/slave/recipes/b...
scripts/slave/recipes/blimp/integration.py:65: api.chromium.c.build_config_fs)
Indenting should be either ....


linux_build = api.path['checkout'].join(
    'out-linux', api.chromium.c.build_config_fs)


OR

linux_build = api.path['checkout'].join('out-linux',
                                        api.chromium.c.build_config_fs)


*I prefer the first.

[shenghuazhang, 2016-10-14 19:27:23]

https://codereview.chromium.org/2415963004/diff/60001/scripts/slave/recipes/b...
File scripts/slave/recipes/blimp/integration.py (right):

https://codereview.chromium.org/2415963004/diff/60001/scripts/slave/recipes/b...
scripts/slave/recipes/blimp/integration.py:65: api.chromium.c.build_config_fs)
On 2016/10/14 19:04:14, mikecase wrote:
> Indenting should be either ....
> 
> 
> linux_build = api.path['checkout'].join(
>     'out-linux', api.chromium.c.build_config_fs)
> 
> 
> OR
> 
> linux_build = api.path['checkout'].join('out-linux',
>                                         api.chromium.c.build_config_fs)
> 
> 
> *I prefer the first.

I see. Will do with the first one. Second one is possible to have 80 characters
problem.

[shenghuazhang, 2016-10-14 19:54:00]

shenghuazhang@chromium.org changed reviewers:
+ dpranke@chromium.org

[shenghuazhang, 2016-10-14 19:54:01]

+Dirk
Can you please have a review on the fix?

[mikecase (-- gone --), 2016-10-14 20:05:18]

lgtm

[shenghuazhang, 2016-10-14 20:13:20]

The CQ bit was checked by shenghuazhang@chromium.org

[commit-bot: I haz the power, 2016-10-14 20:13:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 20:17:43]

Description was changed from

==========
Fix compile on the Blimp Client Engine Integration bot

BUG=655864
==========

to

==========
Fix compile on the Blimp Client Engine Integration bot

BUG=655864

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/61728af22d26f33d09ed...
==========

[commit-bot: I haz the power, 2016-10-14 20:17:48]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/tools/build/+/61728af22d26f33d09ed...

[Dirk Pranke, 2016-10-15 17:23:34]

lgtm