Stop to use contextmanager for goma.build_with_goma

Stop to use contextmanager for goma.build_with_goma

build_with_goma receives ninja_command and builds use it.

This is followup CL of https://codereview.chromium.org/2466793002/

* fix pdfium.py and chromium.py for this interface change

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

[tikuta, 2016-11-07 08:53:21]

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

[commit-bot: I haz the power, 2016-11-07 08:53:24]

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-07 08:56:54]

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

[commit-bot: I haz the power, 2016-11-07 08:57:04]

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-07 08:57:56]

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

[tikuta, 2016-11-07 08:57:56]

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

[tikuta, 2016-11-07 08:57:56]

PTAL.

[tikuta, 2016-11-07 09:04:18]

Description was changed from

==========
Stop implicit modification of env and command in goma.build_with_goma

This followup CL of https://codereview.chromium.org/2466793002/

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

to

==========
Stop implicit modification of env and command in goma.build_with_goma

This is followup CL of https://codereview.chromium.org/2466793002/

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

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

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

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

Dry run: This issue passed the CQ dry run.

[shinyak, 2016-11-07 11:28:14]

https://codereview.chromium.org/2480193002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/example.expected/linux_goma_disabled.json
(right):

https://codereview.chromium.org/2480193002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/example.expected/linux_goma_disabled.json:192:
"500"
-j500 for goma disabled? weird.

[tikuta, 2016-11-07 11:34:22]

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

[commit-bot: I haz the power, 2016-11-07 11:34:33]

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-07 11:35:24]

https://codereview.chromium.org/2480193002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/example.expected/linux_goma_disabled.json
(right):

https://codereview.chromium.org/2480193002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/example.expected/linux_goma_disabled.json:192:
"500"
On 2016/11/07 11:28:14, shinyak wrote:
> -j500 for goma disabled? weird.

Done.

[commit-bot: I haz the power, 2016-11-07 11:44:01]

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

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

Dry run: This issue passed the CQ dry run.

[ukai, 2016-11-08 04:11:57]

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

https://codereview.chromium.org/2480193002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:286: command += ['-j',
self.m.goma.recommended_goma_jobs]
might be better to add -j in goma.build_with_goma when start success, instead of
remove -j?

https://codereview.chromium.org/2480193002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2480193002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:332: ninja_log_command: Command used
for build.
could be ninja_command ?

https://codereview.chromium.org/2480193002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:334: NOT used to run actual build.
fix comment?

https://codereview.chromium.org/2480193002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:342: If starting goma fails, env and
ninja_log_command are modified.
update comment?

[tikuta, 2016-11-09 05:28:12]

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

[commit-bot: I haz the power, 2016-11-09 05:28:26]

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-09 05:31:12]

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

https://codereview.chromium.org/2480193002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:286: command += ['-j',
self.m.goma.recommended_goma_jobs]
On 2016/11/08 04:11:57, ukai wrote:
> might be better to add -j in goma.build_with_goma when start success, instead
of
> remove -j?

Done.

https://codereview.chromium.org/2480193002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2480193002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:332: ninja_log_command: Command used
for build.
On 2016/11/08 04:11:57, ukai wrote:
> could be ninja_command ?

Done.

https://codereview.chromium.org/2480193002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:334: NOT used to run actual build.
On 2016/11/08 04:11:57, ukai wrote:
> fix comment?

Done.

https://codereview.chromium.org/2480193002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:342: If starting goma fails, env and
ninja_log_command are modified.
On 2016/11/08 04:11:57, ukai wrote:
> update comment?

Done.

[commit-bot: I haz the power, 2016-11-09 06:09:12]

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

[commit-bot: I haz the power, 2016-11-09 06:09:13]

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

[ukai, 2016-11-10 02:47:02]

presubmit failed?

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

https://codereview.chromium.org/2480193002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:297: if self.m.platform.is_win and
'GOMA_DISABLED' in env_update:
edge case:
GOMA_DISABLED=false or GOMA_DISABLED=0 will run goma

(client/env_flags.cc GOMA_EnvToBool)

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

https://codereview.chromium.org/2480193002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:331: This function does NOT run actual
build.
it returns this, and if goma is available add -j option 
or so?

https://codereview.chromium.org/2480193002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:380: ninja_command.extend(['-j',
self.recommended_goma_jobs])
prefer -j option before targets on command line.

[tikuta, 2016-11-10 03:02:49]

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

[commit-bot: I haz the power, 2016-11-10 03:02:55]

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-10 03:14:44]

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

[commit-bot: I haz the power, 2016-11-10 03:14:46]

Dry run: This issue passed the CQ dry run.

[tikuta, 2016-11-11 07:57:44]

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

[commit-bot: I haz the power, 2016-11-11 07:57:55]

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-11 08:11:29]

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

[commit-bot: I haz the power, 2016-11-11 08:11:37]

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-11 08:14:55]

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

https://codereview.chromium.org/2480193002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:297: if self.m.platform.is_win and
'GOMA_DISABLED' in env_update:
On 2016/11/10 02:47:02, ukai wrote:
> edge case:
> GOMA_DISABLED=false or GOMA_DISABLED=0 will run goma
> 
> (client/env_flags.cc GOMA_EnvToBool)

Done.

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

https://codereview.chromium.org/2480193002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:331: This function does NOT run actual
build.
On 2016/11/10 02:47:02, ukai wrote:
> it returns this, and if goma is available add -j option 
> or so?

Done.

https://codereview.chromium.org/2480193002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:380: ninja_command.extend(['-j',
self.recommended_goma_jobs])
On 2016/11/10 02:47:02, ukai wrote:
> prefer -j option before targets on command line.

Done.

[commit-bot: I haz the power, 2016-11-11 08:47:19]

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

[commit-bot: I haz the power, 2016-11-11 08:47:21]

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

[tikuta, 2016-11-11 08:54:01]

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

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

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-11 09:38:50]

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

[commit-bot: I haz the power, 2016-11-11 09:38:52]

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

[tikuta, 2016-11-11 10:41:18]

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

[commit-bot: I haz the power, 2016-11-11 10:41:24]

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-11 10:52:53]

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

[commit-bot: I haz the power, 2016-11-11 10:52:55]

Dry run: This issue passed the CQ dry run.

[shinyak, 2016-11-14 08:14:08]

lgtm

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:98: command.pop(i + 1)
I didn't notice before, but `ninja -j` could cause out of index access...?
(you can do in another patch)

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:353: if ninja_command is None:
Can we generate meaningful value when ninja_command is None?
(If this should not happen, assert would be ok, I believe.)

[Paweł Hajdan Jr., 2016-11-14 09:33:23]

"not lgtm" based on API concerns

Please keep up the work on the goma module, it's a good direction in general.
Let's just do some deliberate design on how the API should look like.

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:344: Please use env_update and
ninja_command
Some changes in this patch don't seem to follow this contract - they ignore
env_update.

A safer API would be to use "with api.step.context(env=...) inside
build_with_goma.

I'm also somewhat confused why do we return a command (as a list), rather than
just running it.

[tikuta, 2016-11-14 11:22:02]

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:98: command.pop(i + 1)
On 2016/11/14 08:14:08, shinyak wrote:
> I didn't notice before, but `ninja -j` could cause out of index access...?
> (you can do in another patch)

If there is no arg after '-j' option, IndexError is raised by train.
I think that this is sufficient and no need to take care of this.

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:344: Please use env_update and
ninja_command
On 2016/11/14 09:33:23, Paweł Hajdan Jr. wrote:
> Some changes in this patch don't seem to follow this contract - they ignore
> env_update.
> 
> A safer API would be to use "with api.step.context(env=...) inside
> build_with_goma.
> 
> I'm also somewhat confused why do we return a command (as a list), rather than
> just running it.

When I made build_with_goma, I thought we need to consider build systems other
than ninja.
Also update_windows_env is necessary when starting goma fails before ninja.
https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium/...

Do you think it is better to
* support ninja only, and
* do update_windows_env in build_with_goma
?

[tikuta, 2016-11-15 06:55:26]

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

[commit-bot: I haz the power, 2016-11-15 06:55:32]

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-15 07:06:55]

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

[commit-bot: I haz the power, 2016-11-15 07:06:57]

Dry run: This issue passed the CQ dry run.

[tikuta, 2016-11-15 07:07:11]

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:344: Please use env_update and
ninja_command
On 2016/11/14 11:22:02, tikuta wrote:
> On 2016/11/14 09:33:23, Paweł Hajdan Jr. wrote:
> > Some changes in this patch don't seem to follow this contract - they ignore
> > env_update.
> > 
> > A safer API would be to use "with api.step.context(env=...) inside
> > build_with_goma.
> > 
> > I'm also somewhat confused why do we return a command (as a list), rather
than
> > just running it.
> 
> When I made build_with_goma, I thought we need to consider build systems other
> than ninja.
> Also update_windows_env is necessary when starting goma fails before ninja.
>
https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium/...
> 
> Do you think it is better to
> * support ninja only, and
> * do update_windows_env in build_with_goma
> ?

Hmm, there is a recipe that does not use ninja.
https://cs.chromium.org/chromium/build/scripts/slave/recipes/wasm_llvm.py?q=r...

I modified pdfium.py to use updated_env.

When using api.step.context(env=...), does ninja set env for gomacc in windows?

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:353: if ninja_command is None:
On 2016/11/14 08:14:08, shinyak wrote:
> Can we generate meaningful value when ninja_command is None?
> (If this should not happen, assert would be ok, I believe.)

There is a recipe not using ninja, so ninja_command could become None.
I think using empty array is the easiest way in such case.

[shinyak, 2016-11-15 07:27:19]

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:353: if ninja_command is None:
Then ninja_command in L386 could be ['-j', '50'] or something when
allow_build_without_goma is false and ninja_command is None?

[tikuta, 2016-11-16 06:48:19]

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

[commit-bot: I haz the power, 2016-11-16 06:48:23]

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-16 06:51:51]

Paweł, would you review again?

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2480193002/diff/110001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:353: if ninja_command is None:
On 2016/11/15 07:27:19, shinyak wrote:
> Then ninja_command in L386 could be ['-j', '50'] or something when
> allow_build_without_goma is false and ninja_command is None?

I'm not sure what is the best way to handle such case.
I choose to modify ninja_command if it is not None.

[commit-bot: I haz the power, 2016-11-16 06:59:52]

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

[commit-bot: I haz the power, 2016-11-16 06:59:53]

Dry run: This issue passed the CQ dry run.

[Paweł Hajdan Jr., 2016-11-16 08:38:33]

phajdan.jr@chromium.org changed reviewers:
+ iannucci@chromium.org

[Paweł Hajdan Jr., 2016-11-16 08:38:33]

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

[Paweł Hajdan Jr., 2016-11-16 08:38:35]

+iannucci for recipes

Since build_with_goma seems to already heavily use ninja, the alternatives I see
are:

a) make everything in goma module generic, making it possible to support
arbitrary build systems

b) assume ninja everywhere

I'm leaning towards b) for practical reasons. My main point though is
consistency in the goma module.

Also note about recipes using build_with_goma with something other than ninja
(there's probably ninja under the hood, just with different wrapper scripts):
wasn't this part of conversion to goma recipe module done by the goma team? If
necessary, either parts of that conversion could be reverted, or if really
needed, take a) option above.

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

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

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

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-17 04:38:25]

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

[commit-bot: I haz the power, 2016-11-17 04:38:27]

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

[tikuta, 2016-11-17 04:45:54]

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

[commit-bot: I haz the power, 2016-11-17 04:46:05]

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-17 04:49:13]

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

[commit-bot: I haz the power, 2016-11-17 04:49:15]

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-17 04:56:05]

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

[commit-bot: I haz the power, 2016-11-17 04:56:06]

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

[tikuta, 2016-11-17 05:50:56]

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

[commit-bot: I haz the power, 2016-11-17 05:51:05]

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-17 06:03:48]

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

[commit-bot: I haz the power, 2016-11-17 06:03:59]

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-17 06:26:56]

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

[commit-bot: I haz the power, 2016-11-17 06:26:59]

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-17 06:34:55]

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

[commit-bot: I haz the power, 2016-11-17 06:35:07]

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-17 06:37:50]

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

[commit-bot: I haz the power, 2016-11-17 06:37:56]

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-17 06:41:43]

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

[commit-bot: I haz the power, 2016-11-17 06:41:46]

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-17 06:48:40]

Description was changed from

==========
Stop implicit modification of env and command in goma.build_with_goma

This is followup CL of https://codereview.chromium.org/2466793002/

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

to

==========
Stop to use contextmanager for goma.build_with_goma

build_with_goma receives ninja_command and builds use it.

This is followup CL of https://codereview.chromium.org/2466793002/

* fix pdfium.py and chromium.py for this interface change

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

[tikuta, 2016-11-17 06:53:00]

On 2016/11/16 08:38:35, Paweł Hajdan Jr. wrote:
> +iannucci for recipes
> 
> Since build_with_goma seems to already heavily use ninja, the alternatives I
see
> are:
> 
> a) make everything in goma module generic, making it possible to support
> arbitrary build systems
> 
> b) assume ninja everywhere
> 
> I'm leaning towards b) for practical reasons. My main point though is
> consistency in the goma module.
> 
> Also note about recipes using build_with_goma with something other than ninja
> (there's probably ninja under the hood, just with different wrapper scripts):
> wasn't this part of conversion to goma recipe module done by the goma team? If
> necessary, either parts of that conversion could be reverted, or if really
> needed, take a) option above.

Thank you for comment.
I decided to support only ninja build system.

I rollbacked some changes generating many expectation diff and left TODOs.

Review again, please.

[commit-bot: I haz the power, 2016-11-17 07:22:27]

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

[commit-bot: I haz the power, 2016-11-17 07:22:28]

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

[tikuta, 2016-11-17 07:29:25]

On 2016/11/17 06:53:00, tikuta wrote:
> On 2016/11/16 08:38:35, Paweł Hajdan Jr. wrote:
> > +iannucci for recipes
> > 
> > Since build_with_goma seems to already heavily use ninja, the alternatives I
> see
> > are:
> > 
> > a) make everything in goma module generic, making it possible to support
> > arbitrary build systems
> > 
> > b) assume ninja everywhere
> > 
> > I'm leaning towards b) for practical reasons. My main point though is
> > consistency in the goma module.
> > 
> > Also note about recipes using build_with_goma with something other than
ninja
> > (there's probably ninja under the hood, just with different wrapper
scripts):
> > wasn't this part of conversion to goma recipe module done by the goma team?
If
> > necessary, either parts of that conversion could be reverted, or if really
> > needed, take a) option above.
> 
> Thank you for comment.
> I decided to support only ninja build system.
> 
> I rollbacked some changes generating many expectation diff and left TODOs.
> 
> Review again, please.

This CL depends on following CL's.
https://chromium-review.googlesource.com/c/411558/
https://chromium-review.googlesource.com/c/411666/
https://chromium-review.googlesource.com/c/411748/

CL in rietveld cannot have upstream branch uploaded to gerrit?
Presubmit failure looks to caused by such reason.

[tikuta, 2016-11-17 07:37:00]

On 2016/11/17 07:29:25, tikuta wrote:
> On 2016/11/17 06:53:00, tikuta wrote:
> > On 2016/11/16 08:38:35, Paweł Hajdan Jr. wrote:
> > > +iannucci for recipes
> > > 
> > > Since build_with_goma seems to already heavily use ninja, the alternatives
I
> > see
> > > are:
> > > 
> > > a) make everything in goma module generic, making it possible to support
> > > arbitrary build systems
> > > 
> > > b) assume ninja everywhere
> > > 
> > > I'm leaning towards b) for practical reasons. My main point though is
> > > consistency in the goma module.
> > > 
> > > Also note about recipes using build_with_goma with something other than
> ninja
> > > (there's probably ninja under the hood, just with different wrapper
> scripts):
> > > wasn't this part of conversion to goma recipe module done by the goma
team?
> If
> > > necessary, either parts of that conversion could be reverted, or if really
> > > needed, take a) option above.
> > 
> > Thank you for comment.
> > I decided to support only ninja build system.
> > 
> > I rollbacked some changes generating many expectation diff and left TODOs.
> > 
> > Review again, please.
> 
> This CL depends on following CL's.
> https://chromium-review.googlesource.com/c/411558/
> https://chromium-review.googlesource.com/c/411666/
> https://chromium-review.googlesource.com/c/411748/
> 
> CL in rietveld cannot have upstream branch uploaded to gerrit?
> Presubmit failure looks to caused by such reason.

filed https://bugs.chromium.org/p/chromium/issues/detail?id=666212

[tikuta, 2016-11-17 07:54:37]

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

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

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-17 08:35:49]

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

[commit-bot: I haz the power, 2016-11-17 08:35:50]

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

[tikuta, 2016-11-21 04:41:19]

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

[commit-bot: I haz the power, 2016-11-21 04:41:27]

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-21 04:51:39]

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

[commit-bot: I haz the power, 2016-11-21 04:51:40]

Dry run: This issue passed the CQ dry run.

[tikuta, 2016-11-21 05:00:51]

Presubmit passed.
PTAL?

[Paweł Hajdan Jr., 2016-11-21 17:17:16]

LGTM . Thanks for making these changes.

Consider checking with Robbie (may need a ping over IM). In case he requests
changes conflicting with my advice, I defer.

[tikuta, 2016-11-22 05:44:39]

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

[commit-bot: I haz the power, 2016-11-22 05:44:49]

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-22 05:54:39]

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

[commit-bot: I haz the power, 2016-11-22 05:54:40]

Dry run: This issue passed the CQ dry run.

[ukai, 2016-11-22 06:53:28]

lgtm

[iannucci, 2016-11-22 08:29:38]

lgtm

[tikuta, 2016-11-22 08:32:35]

Thank you for review!
I commit this.

[tikuta, 2016-11-22 08:32:43]

The CQ bit was checked by tikuta@chromium.org

[tikuta, 2016-11-22 08:32:44]

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

[commit-bot: I haz the power, 2016-11-22 08:32:58]

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-22 08:42:52]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1479803563896040,
"parent_rev": "34eea10c63569f86d2b83889b71a1b2f8335033b", "commit_rev":
"27cea7cd1caf1c472b4f55dfe648c52bf0642500"}

[commit-bot: I haz the power, 2016-11-22 08:42:56]

Description was changed from

==========
Stop to use contextmanager for goma.build_with_goma

build_with_goma receives ninja_command and builds use it.

This is followup CL of https://codereview.chromium.org/2466793002/

* fix pdfium.py and chromium.py for this interface change

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

to

==========
Stop to use contextmanager for goma.build_with_goma

build_with_goma receives ninja_command and builds use it.

This is followup CL of https://codereview.chromium.org/2466793002/

* fix pdfium.py and chromium.py for this interface change

BUG=656540

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

[commit-bot: I haz the power, 2016-11-22 08:42:58]

Committed patchset #23 (id:360001) as
https://chromium.googlesource.com/chromium/tools/build/+/27cea7cd1caf1c472b4f...