Stop to use contextmanager for goma.build_with_goma

scripts/slave/recipe_modules/goma/api.py

 # Copyright 2015 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import re
 from contextlib import contextmanager
 
 from recipe_engine import recipe_api
 
 class GomaApi(recipe_api.RecipeApi):
@@ skipping 67 matching lines @@
 
     return self._goma_jobs
 
   def remove_j_flag(self, command):
     """Remove -j flag from command.
     This function elimitnates -j flag from command for
     '-j', '80' style and '-j80' style.
 
     Args:
       command: List of command line arg.
+
+    Returns:
+      list(string): Command line args parallel option removed.
     """
+    command = command[:]
     parallel_flag_regexp = re.compile('-j\d*')
     for i in range(len(command)):
       if (isinstance(command[i], str) and
           parallel_flag_regexp.match(command[i])):
         if command[i] == '-j':
           command.pop(i + 1)

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

I didn't notice before, but `ninja -j` could cause out of index access...?
(you can do in another patch)

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

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.

         command.pop(i)
-        return
+        break
+    return command
 
   def ensure_goma(self, canary=False):
     with self.m.step.nest('ensure_goma'):
       with self.m.step.context({'infra_step': True}):
         try:
           self.m.cipd.set_service_account_credentials(
               self.service_account_json_path)
 
           self.m.cipd.install_client()
           goma_package = ('infra_internal/goma/client/%s' %
@@ skipping 200 matching lines @@
     self.m.python(
       name=name or 'upload_log',
       script=self.package_repo_resource(
           'scripts', 'slave', 'upload_goma_logs.py'),
       args=args,
       env=self._goma_ctl_env
     )
 
   @contextmanager
   def build_with_goma(self, ninja_log_outdir=None, ninja_log_compiler=None,
-                      ninja_log_command=None, env=None,
+                      ninja_command=None, env=None,
                       allow_build_without_goma=False):
     """Make context wrapping goma start/stop.
 
-    Args ninja_log_* are NOT used to run actual build in this context.
-    These args are only used to collect log and upload them.
-
     Args:
       ninja_log_outdir: Directory of ninja log. (e.g. "out/Release")
       ninja_log_compiler: Compiler used in ninja. (e.g. "clang")
-      ninja_log_command: Command used for build.
-                         This is used only to be sent as part of log,
-                         NOT used to run actual build.
-                         (e.g. ['ninja', '-C', 'out/Release', '-j', '100'])
+      ninja_command: Command used for build.
+                     This is used to be sent as part of log.
+                     This function does NOT run actual build.
+                     ninja_command with -j option is returned
+                     if goma starts successfully.
+                     (e.g. ['ninja', '-C', 'out/Release'])
       env: Environment controlling goma behavior.
            This should be used for ninja if allow_build_without_goma is True.
            Otherwise, only used for goma behavior control.
       allow_build_without_goma (bool):
         If this is not True, goma starting failure is ignored.
-        This argument should be used with env.
-        If starting goma fails, env and ninja_log_command are modified.
+        Use returned env_update and ninja_command for the cases.
+
+    Returns:
+      (env_update, ninja_command):
+        Please use env_update and ninja_command

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

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]

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 07:07:10]

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?

+        for ninja in build_with_goma context
+        (e.g. ninja_env.update(env_update)).
+        Appropriate values are set depends on goma status.
 
     Raises:
       StepFailure or InfraFailure if it fails to build.
     """
     ninja_log_exit_status = 0
+    if ninja_command is None:

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

Can we generate meaningful value when ninja_command is None?
(If this should not happen, assert would be ok, I believe.)

[tikuta, 2016/11/15 07:07:10]

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]

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

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

+      ninja_command = []
+    ninja_command = self.remove_j_flag(ninja_command)
 
     if allow_build_without_goma:
       assert(env is not None)
+      env = env.copy()
+
       try:
         self.start(env)
       except:
-        env['GOMA_DISABLED'] = 'true'
-        self.remove_j_flag(ninja_log_command)
         try:
-          yield
+          yield ({'GOMA_DISABLED':'true'}, ninja_command)
         except self.m.step.StepFailure as e: # pragma: no cover
           ninja_log_exit_status = e.retcode
           raise e
         except self.m.step.InfraFailure as e: # pragma: no cover
           ninja_log_exit_status = -1
           raise e
         finally:
           self._upload_logs(ninja_log_outdir=ninja_log_outdir,
                             ninja_log_compiler=ninja_log_compiler,
-                            ninja_log_command=ninja_log_command,
+                            ninja_log_command=ninja_command,
                             ninja_log_exit_status=ninja_log_exit_status,
                             name='upload log goma start failed')
         return
     else:
       self.start(env)
 
+    ninja, args = ninja_command[:1], ninja_command[1:]
+    ninja_command = ninja + ['-j', self.recommended_goma_jobs] + args
+
     try:
-      yield
+      yield ({}, ninja_command)
     except self.m.step.StepFailure as e: # pragma: no cover
       ninja_log_exit_status = e.retcode
       raise e
     except self.m.step.InfraFailure as e: # pragma: no cover
       ninja_log_exit_status = -1
       raise e
     finally:
       self.stop(ninja_log_outdir=ninja_log_outdir,
                 ninja_log_compiler=ninja_log_compiler,
-                ninja_log_command=ninja_log_command,
+                ninja_log_command=ninja_command,
                 ninja_log_exit_status=ninja_log_exit_status)