Refactoring: separate build steps in real_main for compile.py

scripts/slave/compile.py

 #!/usr/bin/env python
 # Copyright (c) 2012 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.
 
 """A tool to build chrome, executed by buildbot.
 
   When this is run, the current directory (cwd) should be the outer build
   directory (e.g., chrome-release/build/).
 
@@ skipping 327 matching lines @@
         if not entry:
           continue
         key, value = entry.split('=', 1)
         env_in_file[key] = value
     env_in_file.update(env_to_store)
     with open(path, 'wb') as f:
       f.write(nul.join(['%s=%s' % (k, v) for k, v in env_in_file.iteritems()]))
       f.write(nul * 2)
 
 
-def main_ninja(options, args):
-  """Interprets options, clobbers object files, and calls ninja."""
+def main_ninja(options, args, env, goma_ready):

[ukai, 2016/07/20 01:33:55]

instead of passing goma_ready, use options.goma_dir for goma readiness.

[tikuta, 2016/07/20 04:20:07]

Done.

+  """calls ninja."""

[ukai, 2016/07/20 01:33:55]

Args and Returns
?

[tikuta, 2016/07/20 04:20:07]

Done.

 
-  # Prepare environment.
-  env = EchoDict(os.environ)
-  goma_ready, goma_cloudtail = goma_setup(options, env)
   exit_status = -1
   try:
     if not goma_ready:

[shinyak, 2016/07/20 02:54:43]

Since goma_ready is used only for assertion purpose here.
So, I feel it's good to do out of this function.

[tikuta, 2016/07/20 04:20:07]

Done.

       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',
@@ skipping 78 matching lines @@
       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
+        return 1, command
+    return exit_status, command

[ukai, 2016/07/20 01:33:55]

better to emit json in a file for additional text to ninja_log,
so goma_util.UploadNinjaLog just reads the file, instead of taking command line
argument (so no need to return command here)

[Yoshisato Yanagisawa, 2016/07/20 02:30:32]

I understand that saying different opinions between reviewers confuses owner of
the change list but I prefer explicit way of passing data. i.e. current
implementation.
By the way, what is preferred behavior when the JSON file is missing?

If returning so much data is not good, why can't we execute
goma_utils.UploadNinjaLog here?  Command execution has already done.  .ninja_log
should already be updated?

   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]
-
-    goma_utils.UploadNinjaLog(
-        options.target_output_dir, options.compiler, command, exit_status,
-        override_gsutil=override_gsutil)
+    pass

[Yoshisato Yanagisawa, 2016/07/20 02:30:32]

you do not need finally.

[tikuta, 2016/07/20 04:20:07]

Done.

 
 
 def get_target_build_dir(args, options):
   """Keep this function in sync with src/build/landmines.py"""
   if chromium_utils.IsLinux() and options.cros_board:
     # When building ChromeOS's Simple Chrome workflow, the output directory
     # has a CROS board name suffix.
     outdir = 'out_%s' % (options.cros_board,)
   elif options.out_dir:
     outdir = options.out_dir
   else:
     outdir = 'out'
   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')
   option_parser.add_option('--target', default='Release',
                            help='build target (Debug or Release)')
   option_parser.add_option('--src-dir', default=None,
                            help='path to the root of the source tree')
   option_parser.add_option('--mode', default='dev',
                            help='build mode (dev or official) controlling '
                                 'environment variables set during build')
@@ skipping 46 matching lines @@
 
   options, args = option_parser.parse_args()
 
   if not options.src_dir:
     options.src_dir = 'src'
   options.src_dir = os.path.abspath(options.src_dir)
 
   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_man():

[Yoshisato Yanagisawa, 2016/07/20 02:30:32]

real_main?

pylint did not detect this?

[tikuta, 2016/07/20 04:20:07]

Thanks.
It looks that `git cl upload` does not run pylint.

+  options, args = get_parsed_options()
+
+  # Prepare environment.
+  env = EchoDict(os.environ)
+
+  # start goma
+  goma_ready, goma_cloudtail = goma_setup(options, env)

[ukai, 2016/07/20 01:33:55]

move goma_setup and goma_teardown in goma_utils?
(maybe, todo)

[tikuta, 2016/07/20 04:20:07]

Let me do in another CL.
I added TODO comment in goma_setup and goma_teardown.

+
+  # build
+  exit_status, command = main_ninja(options, args, env, goma_ready)

[ukai, 2016/07/20 01:33:55]

catch exception in main_ninja?

[tikuta, 2016/07/20 04:20:07]

Done.

+
+  # 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(
+      options.target_output_dir, options.compiler, command, exit_status,
+      override_gsutil=override_gsutil)
+
+  return exit_status
 
 
 if '__main__' == __name__:
   sys.exit(real_main())