Refactor emma_instr.py in preparation for porting EMMA GYP rules to GN

build/android/gyp/emma_instr.py

 #!/usr/bin/env python
 #
 # Copyright 2013 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.
 
 """Instruments classes and jar files.
 
 This script corresponds to the 'emma_instr' action in the java build process.
 Depending on whether emma_instrument is set, the 'emma_instr' action will either
@@ skipping 14 matching lines @@
 import tempfile
 
 sys.path.append(os.path.join(os.path.dirname(__file__), os.pardir))
 from pylib.utils import command_option_parser
 
 from util import build_utils
 
 
 def _AddCommonOptions(option_parser):
   """Adds common options to |option_parser|."""
+  build_utils.AddDepfileOption(option_parser)
   option_parser.add_option('--input-path',
                            help=('Path to input file(s). Either the classes '
                                  'directory, or the path to a jar.'))
   option_parser.add_option('--output-path',
                            help=('Path to output final file(s) to. Either the '
                                  'final classes directory, or the directory in '
                                  'which to place the instrumented/copied jar.'))
   option_parser.add_option('--stamp', help='Path to touch when done.')
   option_parser.add_option('--coverage-file',
                            help='File to create with coverage metadata.')
-  option_parser.add_option('--sources-file',
+  option_parser.add_option('--sources-list-file',
                            help='File to create with the list of sources.')
 
 
 def _AddInstrumentOptions(option_parser):
   """Adds options related to instrumentation to |option_parser|."""
   _AddCommonOptions(option_parser)
-  option_parser.add_option('--sources',
-                           help='Space separated list of sources.')
+  option_parser.add_option('--source-dirs',
+                           help='Space separated list of source directories. '
+                                'source-files should not be specified if '
+                                'source-dirs is specified')
+  option_parser.add_option('--source-files',
+                           help='Space separated list of source files. '
+                                'source-dirs should not be specified if '
+                                'source-files is specified')
   option_parser.add_option('--src-root',
                            help='Root of the src repository.')
   option_parser.add_option('--emma-jar',
                            help='Path to emma.jar.')
   option_parser.add_option(
       '--filter-string', default='',
       help=('Filter string consisting of a list of inclusion/exclusion '
             'patterns separated with whitespace and/or comma.'))
 
 
 def _RunCopyCommand(_command, options, _, option_parser):
   """Copies the jar from input to output locations.
 
   Also removes any old coverage/sources file.
 
   Args:
     command: String indicating the command that was received to trigger
         this function.
     options: optparse options dictionary.
     args: List of extra args from optparse.
     option_parser: optparse.OptionParser object.
 
   Returns:
     An exit code.
   """
   if not (options.input_path and options.output_path and
-          options.coverage_file and options.sources_file):
+          options.coverage_file and options.sources_list_file):
     option_parser.error('All arguments are required.')
 
-  coverage_file = os.path.join(os.path.dirname(options.output_path),
-                               options.coverage_file)
-  sources_file = os.path.join(os.path.dirname(options.output_path),
-                              options.sources_file)
-  if os.path.exists(coverage_file):
-    os.remove(coverage_file)
-  if os.path.exists(sources_file):
-    os.remove(sources_file)
+  if os.path.exists(options.coverage_file):
+    os.remove(options.coverage_file)
+  if os.path.exists(options.sources_list_file):
+    os.remove(options.sources_list_file)
 
   shutil.copy(options.input_path, options.output_path)
 
   if options.stamp:
     build_utils.Touch(options.stamp)
 
+  if options.depfile:
+    build_utils.WriteDepfile(options.depfile,
+                             build_utils.GetPythonDependencies())
 
-def _CreateSourcesFile(sources_string, sources_file, src_root):
-  """Adds all normalized source directories to |sources_file|.
+
+def _GetSourceDirsFromSourceFiles(source_files_string):
+  """Returns list of directories for the files in |source_files_string|.
 
   Args:
-    sources_string: String generated from gyp containing the list of sources.
-    sources_file: File into which to write the JSON list of sources.
+    source_files_string: String generated from GN or GYP containing the list
+      of source files.
+
+  Returns:
+    List of source directories.
+  """
+  source_files = build_utils.ParseGypList(source_files_string)
+  source_dirs = set()

[agrieve, 2015/11/18 15:30:46]

nit: More concise: return list(set(os.path.dirname(p) for p in source_files))

[pkotwicz, 2015/11/18 15:59:28]

It might be because I am relatively new to Python, but I find Python shorthand
to hard to read

[agrieve, 2015/11/18 16:06:18]

In general, List comprehensions are preferred when applicable. Feel free to
leave this on multiple lines, but the really-should-do part is replacing the for
loop with 

source_dirs = set(os.path.dirname(p) for p in source_files)

[pkotwicz, 2015/11/18 17:03:02]

According to the Google style guide, "List comprehensions are Okay to use for
simple cases.". I guess this is a "simple case"
Fixed!

+  for source_file in source_files:
+    source_dirs.add(os.path.dirname(source_file))
+  return list(source_dirs)
+
+
+def _CreateSourcesListFile(source_dirs, sources_list_file, src_root):
+  """Adds all normalized source directories to |sources_list_file|.
+
+  Args:
+    source_dirs: List of source directories.
+    sources_list_file: File into which to write the JSON list of sources.
     src_root: Root which sources added to the file should be relative to.
 
   Returns:
     An exit code.
   """
   src_root = os.path.abspath(src_root)
-  sources = build_utils.ParseGypList(sources_string)
   relative_sources = []
-  for s in sources:
+  for s in source_dirs:
     abs_source = os.path.abspath(s)
     if abs_source[:len(src_root)] != src_root:
       print ('Error: found source directory not under repository root: %s %s'
              % (abs_source, src_root))
       return 1
     rel_source = os.path.relpath(abs_source, src_root)
 
     relative_sources.append(rel_source)
 
-  with open(sources_file, 'w') as f:
+  with open(sources_list_file, 'w') as f:
     json.dump(relative_sources, f)
 
 
 def _RunInstrumentCommand(_command, options, _, option_parser):
   """Instruments jar files using EMMA.
 
   Args:
     command: String indicating the command that was received to trigger
         this function.
     options: optparse options dictionary.
     args: List of extra args from optparse.
     option_parser: optparse.OptionParser object.
 
   Returns:
     An exit code.
   """
   if not (options.input_path and options.output_path and
-          options.coverage_file and options.sources_file and options.sources and
+          options.coverage_file and options.sources_list_file and
+          (options.source_files or options.source_dirs) and
           options.src_root and options.emma_jar):
     option_parser.error('All arguments are required.')
 
-  coverage_file = os.path.join(os.path.dirname(options.output_path),
-                               options.coverage_file)
-  sources_file = os.path.join(os.path.dirname(options.output_path),
-                              options.sources_file)
-  if os.path.exists(coverage_file):
-    os.remove(coverage_file)
+  if os.path.exists(options.coverage_file):
+    os.remove(options.coverage_file)
   temp_dir = tempfile.mkdtemp()
   try:
     cmd = ['java', '-cp', options.emma_jar,
            'emma', 'instr',
            '-ip', options.input_path,
            '-ix', options.filter_string,
            '-d', temp_dir,
-           '-out', coverage_file,
+           '-out', options.coverage_file,
            '-m', 'fullcopy']
     build_utils.CheckOutput(cmd)
 
-    for jar in os.listdir(os.path.join(temp_dir, 'lib')):
-      shutil.copy(os.path.join(temp_dir, 'lib', jar),
-                    options.output_path)
+    temp_jar_dir = os.path.join(temp_dir, 'lib')
+    jars = os.listdir(temp_jar_dir)
+    if len(jars) != 1:
+      print('Error: multiple output files in: %s' % (temp_jar_dir))

[agrieve, 2015/11/18 15:30:46]

you should raise an exception rather than explicitly returning an error code:

raise Exception(msg)

[pkotwicz, 2015/11/18 15:59:28]

This way of handling exceptions is consistent with other places which handle
exceptions in this file

+      return 1
+
+    shutil.copy(os.path.join(temp_jar_dir, jars[0]), options.output_path)
   finally:
     shutil.rmtree(temp_dir)
 
-  _CreateSourcesFile(options.sources, sources_file, options.src_root)
+  source_dirs = []

[agrieve, 2015/11/18 15:30:46]

no need to initialize since it's re-assigned in both branches

+  if options.source_dirs:
+    source_dirs = build_utils.ParseGypList(options.source_dirs)
+  else:
+    source_dirs = _GetSourceDirsFromSourceFiles(options.source_files)
+  _CreateSourcesListFile(source_dirs, options.sources_list_file,
+                           options.src_root)

[agrieve, 2015/11/18 15:30:46]

nit: indent is off

[pkotwicz, 2015/11/18 15:59:28]

I am confused. There is an '_' before the 'C" in _CreateSourcesListFile()

 
   if options.stamp:
     build_utils.Touch(options.stamp)
 
+  if options.depfile:
+    build_utils.WriteDepfile(options.depfile,
+                             build_utils.GetPythonDependencies())
+
   return 0
 
 
 CommandFunctionTuple = collections.namedtuple(
     'CommandFunctionTuple', ['add_options_func', 'run_command_func'])
 VALID_COMMANDS = {
     'copy': CommandFunctionTuple(_AddCommonOptions,
                                  _RunCopyCommand),
     'instrument_jar': CommandFunctionTuple(_AddInstrumentOptions,
                                            _RunInstrumentCommand),
 }
 
 
 def main():
   option_parser = command_option_parser.CommandOptionParser(
       commands_dict=VALID_COMMANDS)
   command_option_parser.ParseAndExecute(option_parser)
 
 
 if __name__ == '__main__':
   sys.exit(main())