Refactoring: separate build steps in real_main for compile.py

scripts/slave/compile.py

Index: scripts/slave/compile.py
diff --git a/scripts/slave/compile.py b/scripts/slave/compile.py
index d7310ec875da1963335f312b7520f5f561c74b47..5e26a0b539712aee4ee612a9ab6ec6575d9ec90b 100755
--- a/scripts/slave/compile.py
+++ b/scripts/slave/compile.py
@@ -71,7 +71,7 @@ class EchoDict(dict):
         fh.write('  %s (removed)\n' % k)
     fh.write('\n')
 
-
+# TODO(tikuta): move to goma_utils.py
 def goma_setup(options, env):
   """Sets up goma if necessary.
 
@@ -222,7 +222,7 @@ def goma_setup(options, env):
   env['GOMA_DISABLED'] = '1'
   return False, None
 
-
+# TODO(tikuta): move to goma_utils.py
 def goma_teardown(options, env, exit_status, cloudtail_proc):
   """Tears down goma if necessary. """
   if (options.compiler in ('goma', 'goma-clang') and
@@ -345,125 +345,117 @@ def UpdateWindowsEnvironment(envfile_dir, env):
       f.write(nul * 2)
 
 
-def main_ninja(options, args):
-  """Interprets options, clobbers object files, and calls ninja."""
+def main_ninja(options, args, env):
+  """This function calls ninja.

[ukai, 2016/07/20 07:07:31]

it does clobbers object files etc?

[Nico, 2016/07/20 08:30:43]

It currently does (but only on the very few bots that don't use recipes --
recipe bots have a separate clobber step already), but I'd like to remove that.
See the bug # in the TODO for the clobber bits in this file.

 
-  # Prepare environment.
-  env = EchoDict(os.environ)
-  goma_ready, goma_cloudtail = goma_setup(options, env)
-  exit_status = -1
-  try:
-    if not goma_ready:
-      assert options.compiler not in ('goma', 'goma-clang')
-      assert options.goma_dir is None
-
-    # ninja is different from all the other build systems in that it requires
-    # most configuration to be done at gyp time. This is why this function does
-    # less than the other comparable functions in this file.
-    print 'chdir to %s' % options.src_dir
-    os.chdir(options.src_dir)
-
-    command = [options.ninja_path, '-w', 'dupbuild=err',
-                                   '-C', options.target_output_dir]
-
-    # HACK(yyanagisawa): update environment files on |env| update.
-    # For compiling on Windows, environment in environment files are used.
-    # It means even if enviroment such as GOMA_DISABLED is updated in
-    # compile.py, the update will be ignored.
-    # We need to update environment files to reflect the update.
-    if chromium_utils.IsWindows() and NeedEnvFileUpdateOnWin(env):
-      print 'Updating environment.{x86,x64} files.'
-      UpdateWindowsEnvironment(options.target_output_dir, env)
-
-    if options.clobber:
-      print 'Removing %s' % options.target_output_dir
-      # Deleting output_dir would also delete all the .ninja files necessary to
-      # build. Clobbering should run before runhooks (which creates .ninja
-      # files). For now, only delete all non-.ninja files.
-      # TODO(thakis): Make "clobber" a step that runs before "runhooks".
-      # Once the master has been restarted, remove all clobber handling
-      # from compile.py, https://crbug.com/574557
-      build_directory.RmtreeExceptNinjaOrGomaFiles(options.target_output_dir)
-
-    command.extend(options.build_args)
-    command.extend(args)
-
-    maybe_set_official_build_envvars(options, env)
-
-    if options.compiler:
-      print 'using', options.compiler
-
-    if options.compiler in ('goma', 'goma-clang'):
-      assert options.goma_dir
-
-      def determine_goma_jobs():
-        # We would like to speed up build on Windows a bit, since it is slowest.
-        number_of_processors = 0
-        try:
-          number_of_processors = multiprocessing.cpu_count()
-        except NotImplementedError:
-          print 'cpu_count() is not implemented, using default value 50.'
-          return 50
-
-        assert number_of_processors > 0
-
-        # When goma is used, 10 * number_of_processors is basically good in
-        # various situations according to our measurement. Build speed won't
-        # be improved if -j is larger than that.
-        #
-        # Since Mac had process number limitation before, we had to set
-        # the upper limit to 50. Now that the process number limitation is 2000,
-        # so we would be able to use 10 * number_of_processors.
-        # For the safety, we'd like to set the upper limit to 200.
-        #
-        # Note that currently most try-bot build slaves have 8 processors.
-        if chromium_utils.IsMac() or chromium_utils.IsWindows():
-          return min(10 * number_of_processors, 200)
-
-        # For Linux, we also would like to use 10 * cpu. However, not sure
-        # backend resource is enough, so let me set Linux and Linux x64 builder
-        # only for now.
-        hostname = goma_utils.GetShortHostname()
-        if hostname in (
-            ['build14-m1', 'build48-m1'] +
-            # Also increasing cpus for v8/blink trybots.
-            ['build%d-m4' % x for x in xrange(45, 48)] +
-            # Also increasing cpus for LTO buildbots.
-            ['slave%d-c1' % x for x in [20, 33] + range(78, 108)]):
-          return min(10 * number_of_processors, 200)
+  Args:
+      options (Option): options for ninja command.
+      args (str): extra args for ninja command.
+      env (dict): Used when ninja command executes.
 
-        return 50
+  Returns:
+      (int, str): first element for ninja command exit status.
+                  secont element is actual command to run ninja.

[ukai, 2016/07/20 07:07:31]

secont?

[tikuta, 2016/07/21 02:44:35]

Done.

 
-      goma_jobs = determine_goma_jobs()
-      command.append('-j%d' % goma_jobs)
-
-    # Run the build.
-    env.print_overrides()
-    exit_status = chromium_utils.RunCommand(command, env=env)
-    if exit_status == 0 and options.ninja_ensure_up_to_date:
-      # Run the build again if we want to check that the no-op build is clean.
-      filter_obj = EnsureUpToDateFilter()
-      # Append `-d explain` to help diagnose in the failure case.
-      command += ['-d', 'explain']
-      chromium_utils.RunCommand(command, env=env, filter_obj=filter_obj)
-      if not filter_obj.was_up_to_date:
-        print 'Failing build because ninja reported work to do.'
-        print 'This means that after completing a compile, another was run and'
-        print 'it resulted in still having work to do (that is, a no-op build'
-        print 'wasn\'t a no-op). Consult the first "ninja explain:" line for a'
-        print 'likely culprit.'
-        return 1
-    return exit_status
-  finally:
-    goma_teardown(options, env, exit_status, goma_cloudtail)
+  """
 
-    override_gsutil = None
-    if options.gsutil_py_path:
-      override_gsutil = [sys.executable, options.gsutil_py_path]
+  # ninja is different from all the other build systems in that it requires
+  # most configuration to be done at gyp time. This is why this function does
+  # less than the other comparable functions in this file.

[Yoshisato Yanagisawa, 2016/07/20 07:31:57]

Do we need this explanation?  We might be only have ninja, and nothing else?

[tikuta, 2016/07/21 02:44:35]

Done.

+  print 'chdir to %s' % options.src_dir
+  os.chdir(options.src_dir)
+
+  command = [options.ninja_path, '-w', 'dupbuild=err',
+                                 '-C', options.target_output_dir]
+
+  # HACK(yyanagisawa): update environment files on |env| update.
+  # For compiling on Windows, environment in environment files are used.
+  # It means even if enviroment such as GOMA_DISABLED is updated in
+  # compile.py, the update will be ignored.
+  # We need to update environment files to reflect the update.
+  if chromium_utils.IsWindows() and NeedEnvFileUpdateOnWin(env):
+    print 'Updating environment.{x86,x64} files.'
+    UpdateWindowsEnvironment(options.target_output_dir, env)
+
+  if options.clobber:
+    print 'Removing %s' % options.target_output_dir
+    # Deleting output_dir would also delete all the .ninja files necessary to
+    # build. Clobbering should run before runhooks (which creates .ninja
+    # files). For now, only delete all non-.ninja files.
+    # TODO(thakis): Make "clobber" a step that runs before "runhooks".
+    # Once the master has been restarted, remove all clobber handling
+    # from compile.py, https://crbug.com/574557
+    build_directory.RmtreeExceptNinjaOrGomaFiles(options.target_output_dir)
+
+  command.extend(options.build_args)
+  command.extend(args)
+
+  maybe_set_official_build_envvars(options, env)
+
+  if options.compiler:
+    print 'using', options.compiler
+
+  if options.compiler in ('goma', 'goma-clang'):

[ukai, 2016/07/20 07:07:31]

might be better to pass number of jobs via options or so?

[tikuta, 2016/07/21 02:44:35]

Done.

+    assert options.goma_dir
+
+    def determine_goma_jobs():
+      # We would like to speed up build on Windows a bit, since it is slowest.
+      number_of_processors = 0
+      try:
+        number_of_processors = multiprocessing.cpu_count()
+      except NotImplementedError:
+        print 'cpu_count() is not implemented, using default value 50.'
+        return 50
 
-    goma_utils.UploadNinjaLog(
-        options.target_output_dir, options.compiler, command, exit_status,
-        override_gsutil=override_gsutil)
+      assert number_of_processors > 0
+
+      # When goma is used, 10 * number_of_processors is basically good in
+      # various situations according to our measurement. Build speed won't
+      # be improved if -j is larger than that.
+      #
+      # Since Mac had process number limitation before, we had to set
+      # the upper limit to 50. Now that the process number limitation is 2000,
+      # so we would be able to use 10 * number_of_processors.
+      # For the safety, we'd like to set the upper limit to 200.
+      #
+      # Note that currently most try-bot build slaves have 8 processors.
+      if chromium_utils.IsMac() or chromium_utils.IsWindows():
+        return min(10 * number_of_processors, 200)
+
+      # For Linux, we also would like to use 10 * cpu. However, not sure
+      # backend resource is enough, so let me set Linux and Linux x64 builder
+      # only for now.
+      hostname = goma_utils.GetShortHostname()
+      if hostname in (
+          ['build14-m1', 'build48-m1'] +
+          # Also increasing cpus for v8/blink trybots.
+          ['build%d-m4' % x for x in range(45, 48)] +
+          # Also increasing cpus for LTO buildbots.
+          ['slave%d-c1' % x for x in [20, 33] + range(78, 108)]):
+        return min(10 * number_of_processors, 200)
+
+      return 50
+
+    goma_jobs = determine_goma_jobs()
+    command.append('-j%d' % goma_jobs)
+
+  # Run the build.
+  env.print_overrides()
+  exit_status = chromium_utils.RunCommand(command, env=env)
+  if exit_status == 0 and options.ninja_ensure_up_to_date:
+    # Run the build again if we want to check that the no-op build is clean.
+    filter_obj = EnsureUpToDateFilter()
+    # Append `-d explain` to help diagnose in the failure case.
+    command += ['-d', 'explain']
+    chromium_utils.RunCommand(command, env=env, filter_obj=filter_obj)
+    if not filter_obj.was_up_to_date:
+      print 'Failing build because ninja reported work to do.'
+      print 'This means that after completing a compile, another was run and'
+      print 'it resulted in still having work to do (that is, a no-op build'
+      print 'wasn\'t a no-op). Consult the first "ninja explain:" line for a'
+      print 'likely culprit.'
+      return 1, command
+  return exit_status, command
 
 
 def get_target_build_dir(args, options):
@@ -479,7 +471,7 @@ def get_target_build_dir(args, options):
   return os.path.abspath(os.path.join(options.src_dir, outdir, options.target))
 
 
-def real_main():
+def get_parsed_options():
   option_parser = optparse.OptionParser()
   option_parser.add_option('--clobber', action='store_true', default=False,
                            help='delete the output directory before compiling')
@@ -546,7 +538,40 @@ def real_main():
   options.target_output_dir = get_target_build_dir(args, options)
 
   assert options.build_tool in (None, 'ninja')
-  return main_ninja(options, args)
+  return options, args
+
+
+def real_main():
+  options, args = get_parsed_options()
+
+  # Prepare environment.
+  env = EchoDict(os.environ)
+
+  # start goma
+  goma_ready, goma_cloudtail = goma_setup(options, env)
+
+  if not goma_ready:
+    assert options.compiler not in ('goma', 'goma-clang')
+    assert options.goma_dir is None
+
+  # build
+  try:
+    exit_status, command = main_ninja(options, args, env)
+  except Exception as e:
+    print 'failed to build using ninja: %s' % e
+
+  # stop goma
+  goma_teardown(options, env, exit_status, goma_cloudtail)
+
+  override_gsutil = None
+  if options.gsutil_py_path:
+    override_gsutil = [sys.executable, options.gsutil_py_path]
+
+  goma_utils.UploadNinjaLog(

[ukai, 2016/07/20 07:07:31]

upload ninja log in main_ninja?

[tikuta, 2016/07/21 02:44:35]

Done.

+      options.target_output_dir, options.compiler, command, exit_status,
+      override_gsutil=override_gsutil)
+
+  return exit_status
 
 
 if '__main__' == __name__: