Add option to not use compile.py in chromium recipe_module

Add option to not use compile.py in chromium recipe_module

BUG=656540
Committed: https://chromium.googlesource.com/chromium/tools/build/+/dd5b78b68ea21f4f2f8a6b5fb91675f10f62fea6

[tikuta, 2016-11-01 04:11:39]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 04:11:47]

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

[tikuta, 2016-11-01 04:17:27]

Description was changed from

==========
Add option to not use compile.py in chromium recipe_module

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

to

==========
Add option to not use compile.py in chromium recipe_module

BUG=656540
==========

[tikuta, 2016-11-01 04:17:28]

tikuta@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, shinyak@chromium.org, ukai@chromium.org,
yyanagisawa@chromium.org

[tikuta, 2016-11-01 04:19:03]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 04:19:10]

Dry run: 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-11-01 04:28:37]

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

[commit-bot: I haz the power, 2016-11-01 04:28:38]

Dry run: This issue passed the CQ dry run.

[shinyak, 2016-11-01 04:37:09]

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (left):

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:306:
ninja_env.update(kwargs.pop('env', {}))
Could you explain why we don't need to do `ninja_env.update(kwargs.pop('env',
{}))` in new code?

[tikuta, 2016-11-01 04:45:05]

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (left):

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:306:
ninja_env.update(kwargs.pop('env', {}))
On 2016/11/01 04:37:09, shinyak wrote:
> Could you explain why we don't need to do `ninja_env.update(kwargs.pop('env',
> {}))` in new code?

I noticed that same thing is done in line 172 and 173.
So, let me remove this line.

[shinyak, 2016-11-01 04:55:22]

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (left):

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:306:
ninja_env.update(kwargs.pop('env', {}))
Oh, I see. Then, maybe it would be good to use kwargs.pop in L172 (right) and
remove L293 (right)?

[tikuta, 2016-11-01 05:09:00]

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (left):

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:306:
ninja_env.update(kwargs.pop('env', {}))
On 2016/11/01 04:55:22, shinyak wrote:
> Oh, I see. Then, maybe it would be good to use kwargs.pop in L172 (right) and
> remove L293 (right)? 

kwargs['env'] is used in line 324.
I will use kwargs.pop in L172 after removing compile.py.

[shinyak (Google), 2016-11-01 05:20:06]

shinyak@google.com changed reviewers:
+ shinyak@google.com

[shinyak (Google), 2016-11-01 05:20:06]

lgtm

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (left):

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:306:
ninja_env.update(kwargs.pop('env', {}))
Hmm, sorry. if-block from L265 is too large ><

[shinyak, 2016-11-01 05:52:46]

lgtm from chromium account

[ukai, 2016-11-02 01:22:08]

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:314: env=ninja_env,
ninja_env will have GOMA_DISABLED if failed to start goma?

GOMA_DISABLED should be passed to gomacc, so it must be in ninja_env?

maybe, L303 should check ninja_env, instead of goma_env?

[tikuta, 2016-11-02 04:49:40]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 04:49:51]

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

[tikuta, 2016-11-02 04:51:30]

tikuta@chromium.org changed required reviewers:
+ phajdan.jr@chromium.org

[tikuta, 2016-11-02 04:51:30]

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://codereview.chromium.org/2466793002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:314: env=ninja_env,
On 2016/11/02 01:22:07, ukai wrote:
> ninja_env will have GOMA_DISABLED if failed to start goma?
> 
> GOMA_DISABLED should be passed to gomacc, so it must be in ninja_env?
> 
> maybe, L303 should check ninja_env, instead of goma_env?

'GOMA_DISABLED' is set in goma_env passed to goma.build_with_goma(...).
So, I added 'GOMA_DISABLED' in ninja_env if 'GOMA_DISABLED' is set in goma_env.

[commit-bot: I haz the power, 2016-11-02 04:54:00]

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

[commit-bot: I haz the power, 2016-11-02 04:54:01]

Dry run: Try jobs failed on following builders:
  Build Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/323c32bd6b67d010)

[tikuta, 2016-11-02 05:11:37]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 05:11:48]

Dry run: 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-11-02 05:21:18]

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

[commit-bot: I haz the power, 2016-11-02 05:21:19]

Dry run: This issue passed the CQ dry run.

[Paweł Hajdan Jr., 2016-11-02 10:04:16]

LGTM

[ukai, 2016-11-04 02:35:46]

lgtm

https://codereview.chromium.org/2466793002/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://codereview.chromium.org/2466793002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:301:
allow_build_without_goma=allow_build_without_goma):
goma.build_with_goma says env should be used for ninja if
allow_build_without_goma is True, so

 if allow_build_without_goma:
    ninja_env = goma_env

or fix comment of goma.build_with_goma?

[tikuta, 2016-11-04 03:09:02]

https://codereview.chromium.org/2466793002/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://codereview.chromium.org/2466793002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:301:
allow_build_without_goma=allow_build_without_goma):
On 2016/11/04 02:35:45, ukai wrote:
> goma.build_with_goma says env should be used for ninja if
> allow_build_without_goma is True, so
> 
>  if allow_build_without_goma:
>     ninja_env = goma_env
> 
> or fix comment of goma.build_with_goma?

Hmm, please let me make another CL for this.
I think modifing goma_env and command in goma.build_with_goma implicitly is not
good.
I'll use yield to pass updated env and command for 'with' scope.

Also goma_env contains many vars for goma that is not needed to pass ninja.

[tikuta, 2016-11-07 02:02:48]

The CQ bit was checked by tikuta@chromium.org

[tikuta, 2016-11-07 02:02:48]

The patchset sent to the CQ was uploaded after l-g-t-m from shinyak@google.com,
shinyak@chromium.org
Link to the patchset: https://codereview.chromium.org/2466793002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-07 02:02:54]

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-11-07 02:19:08]

Description was changed from

==========
Add option to not use compile.py in chromium recipe_module

BUG=656540
==========

to

==========
Add option to not use compile.py in chromium recipe_module

BUG=656540

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

[commit-bot: I haz the power, 2016-11-07 02:19:10]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/tools/build/+/dd5b78b68ea21f4f2f8a...