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..9ba34c1c14f5e3c6ede48d9a0e31efcfdb67853f 100755
--- a/scripts/slave/compile.py
+++ b/scripts/slave/compile.py
@@ -345,12 +345,9 @@ 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, 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.

@@ -452,18 +449,10 @@ def main_ninja(options, args):
         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):
@@ -479,7 +468,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 +535,33 @@ 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_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__: