Another attempt to land http://codereview.chromium.org/8589029.

chrome/tools/build/win/create_installer_archive.py

Index: chrome/tools/build/win/create_installer_archive.py
diff --git a/chrome/tools/build/win/create_installer_archive.py b/chrome/tools/build/win/create_installer_archive.py
index 7cec0d66c754069446cf8ed52a8b3cf4301da6f7..2b59fa034af1b9a66c1f939eb13c5ba28ecdbcdf 100755
--- a/chrome/tools/build/win/create_installer_archive.py
+++ b/chrome/tools/build/win/create_installer_archive.py
@@ -14,10 +14,10 @@
 
 import ConfigParser
 import glob
-import md5
 import optparse
 import os
 import shutil
+import subprocess
 import sys
 
 
@@ -35,24 +35,14 @@ COMPRESSED_ARCHIVE_SUFFIX = ".packed.7z"
 COMPRESSED_FILE_EXT = ".packed.7z"     # extension of patch archive file
 COURGETTE_EXEC = "courgette.exe"
 MINI_INSTALLER_INPUT_FILE = "packed_files.txt"
-PACKED_FILE_COMMENTS = """
-// This file is automatically generated by create_installer_archive.py.
-// It contains the resource entries that are going to be linked inside
-// mini_installer.exe. For each file to be linked there should be two
-// lines:
-// - The first line contains the output filename (without path) and the
-// type of the resource ('BN' - not compressed , 'BL' - LZ compressed,
-// 'B7' - LZMA compressed)
-// - The second line contains the path to the input file. Uses '/' to
-// separate path components.
-"""
 PATCH_FILE_EXT = '.diff'
 SETUP_EXEC = "setup.exe"
 SETUP_PATCH_FILE_PREFIX = "setup_patch"
 TEMP_ARCHIVE_DIR = "temp_installer_archive"
 VERSION_FILE = "VERSION"
 
-def BuildVersion(output_dir):
+
+def BuildVersion(build_dir):
   """Returns the full build version string constructed from information in
   VERSION_FILE.  Any segment not found in that file will default to '0'.
   """
@@ -60,7 +50,7 @@ def BuildVersion(output_dir):
   minor = 0
   build = 0
   patch = 0
-  for line in open(os.path.join(output_dir, "..", "..", "chrome", VERSION_FILE), 'r'):
+  for line in open(os.path.join(build_dir, '../../chrome', VERSION_FILE), 'r'):
     line = line.rstrip()
     if line.startswith('MAJOR='):
       major = line[6:]
@@ -72,9 +62,11 @@ def BuildVersion(output_dir):
       patch = line[6:]
   return '%s.%s.%s.%s' % (major, minor, build, patch)
 
-def CompressUsingLZMA(output_dir, compressed_file, input_file):
-  lzma_exec = GetLZMAExec(output_dir)
-  cmd = ('%s a -t7z '
+
+def CompressUsingLZMA(build_dir, compressed_file, input_file):
+  lzma_exec = GetLZMAExec(build_dir)
+  cmd = [lzma_exec,
+         'a', '-t7z',
           # Flags equivalent to -mx9 (ultra) but with the bcj2 turned on (exe
           # pre-filter). This results in a ~2.3MB decrease in installer size on
           # a 24MB installer.
@@ -83,36 +75,34 @@ def CompressUsingLZMA(output_dir, compressed_file, input_file):
           # 26bit = 64MB. This results in an additional ~3.5MB decrease.
           # Older 7zip versions can support these settings, as these changes
           # rely on existing functionality in the lzma format.
-          '-m0=BCJ2 '
-          '-m1=LZMA:d26:fb64 '
-          '-m2=LZMA:d20:fb64:mf=bt2 '
-          '-m3=LZMA:d20:fb64:mf=bt2 '
-          '-mb0:1 -mb0s1:2 '
-          '-mb0s2:3 '
-          '"%s" "%s"') % (lzma_exec, compressed_file, input_file)
+          '-m0=BCJ2',
+          '-m1=LZMA:d26:fb64',
+          '-m2=LZMA:d20:fb64:mf=bt2',
+          '-m3=LZMA:d20:fb64:mf=bt2',
+          '-mb0:1',
+          '-mb0s1:2',
+          '-mb0s2:3',
+          compressed_file,
+          input_file,]
   if os.path.exists(compressed_file):
     os.remove(compressed_file)
   RunSystemCommand(cmd)
 
-def CopyAllFilesToStagingDir(config, distribution, staging_dir, output_dir):
+
+def CopyAllFilesToStagingDir(config, distribution, staging_dir, build_dir):
   """Copies the files required for installer archive.
   Copies all common files required for various distributions of Chromium and
   also files for the specific Chromium build specified by distribution.
   """
-  CopySectionFilesToStagingDir(config, 'GENERAL', staging_dir, output_dir)
+  CopySectionFilesToStagingDir(config, 'GENERAL', staging_dir, build_dir)
   if distribution:
     if len(distribution) > 1 and distribution[0] == '_':
       distribution = distribution[1:]
     CopySectionFilesToStagingDir(config, distribution.upper(),
-                                 staging_dir, output_dir)
+                                 staging_dir, build_dir)
 
-def IsChromeFrameFile(file):
-  for cf_file in ['npchrome_frame', 'chrome_launcher']:
-    if file.lower().find(cf_file) != -1:
-      return True
-  return False
 
-def CopySectionFilesToStagingDir(config, section, staging_dir, output_dir):
+def CopySectionFilesToStagingDir(config, section, staging_dir, build_dir):
   """Copies installer archive files specified in section to staging dir.
   This method copies reads section from config file and copies all the files
   specified to staging dir.
@@ -124,63 +114,55 @@ def CopySectionFilesToStagingDir(config, section, staging_dir, output_dir):
     dst = os.path.join(staging_dir, config.get(section, option))
     if not os.path.exists(dst):
       os.makedirs(dst)
-    for file in glob.glob(os.path.join(output_dir, option)):
-      if IsChromeFrameFile(file):
-        try:
-          shutil.copy(file, dst)
-        except IOError:
-          # TODO(robertshield): Temporary hack to work around problems building
-          # Chrome Frame binaries on non-Chrome Frame builders. Remove this
-          # asap.
-          print 'Error attempting to copy ' + file + ' to ' + dst
-      else:
-        shutil.copy(file, dst)
+    for file in glob.glob(os.path.join(build_dir, option)):
+      shutil.copy(file, dst)
 
 def GenerateDiffPatch(options, orig_file, new_file, patch_file):
   if (options.diff_algorithm == "COURGETTE"):
     exe_file = os.path.join(options.last_chrome_installer, COURGETTE_EXEC)
     cmd = '%s -gen "%s" "%s" "%s"' % (exe_file, orig_file, new_file, patch_file)
   else:
-    exe_file = os.path.join(options.output_dir, BSDIFF_EXEC)
-    cmd = '%s "%s" "%s" "%s"' % (exe_file, orig_file, new_file, patch_file)
+    exe_file = os.path.join(options.build_dir, BSDIFF_EXEC)
+    cmd = [exe_file, orig_file, new_file, patch_file,]
   RunSystemCommand(cmd)
 
-def GetLZMAExec(output_dir):
-  lzma_exec = os.path.join(output_dir, "..", "..", "third_party",
+def GetLZMAExec(build_dir):
+  lzma_exec = os.path.join(build_dir, "..", "..", "third_party",
                            "lzma_sdk", "Executable", "7za.exe")
   return lzma_exec
 
-def GetPrevVersion(output_dir, temp_dir, last_chrome_installer):
+def GetPrevVersion(build_dir, temp_dir, last_chrome_installer):
   if not last_chrome_installer:
     return ''
 
-  lzma_exec = GetLZMAExec(options.output_dir)
+  lzma_exec = GetLZMAExec(options.build_dir)
   prev_archive_file = os.path.join(options.last_chrome_installer,
                                    options.output_name + ARCHIVE_SUFFIX)
-  cmd = '%s x -o"%s" "%s" Chrome-bin/*/chrome.dll' % (lzma_exec, temp_dir,
-                                                      prev_archive_file)
+  cmd = [lzma_exec,
+         'x',
+         '-o"%s"' % temp_dir,
+         prev_archive_file,
+         'Chrome-bin/*/chrome.dll',]
   RunSystemCommand(cmd)
   dll_path = glob.glob(os.path.join(temp_dir, 'Chrome-bin', '*', 'chrome.dll'))
   return os.path.split(os.path.split(dll_path[0])[0])[1]
 
-def MakeStagingDirectories(output_dir):
+def MakeStagingDirectories(staging_dir):
   """Creates a staging path for installer archive. If directory exists already,
   deletes the existing directory.
   """
-  prefixed_archive_dir = (options.archive_prefix or "") + ARCHIVE_DIR
-  file_path = os.path.join(output_dir, prefixed_archive_dir)
+  file_path = os.path.join(staging_dir, TEMP_ARCHIVE_DIR)

[brucedawson, 2016/04/16 00:08:59]

I hate to do a drive-by code review 52 months after the fact, but...

This function used to return two different directory names. The appended name
for file_path was changed by this CL from ARCHIVE_DIR to TEMP_ARCHIVE_DIR, so
now this function destroys and creates and returns the same directory twice.

I'm not sure what difference this makes - apparently nothing too serious since
it's been unnoticed for a long time. Thoughts?

[Nico, 2017/09/05 17:37:02]

I noticed the same thing 57 months after the fact. I too am curious about this.

[Nico, 2017/09/05 17:38:10]

Ah, there's a reply on the toplevel issue here:
https://codereview.chromium.org/8615002/#msg8

Guess I'll send a CL to clean this up.

   if os.path.exists(file_path):
     shutil.rmtree(file_path)
   os.makedirs(file_path)
 
-  prefixed_temp_archive_dir = (options.archive_prefix or "") + TEMP_ARCHIVE_DIR
-  temp_file_path = os.path.join(output_dir, prefixed_temp_archive_dir)
+  temp_file_path = os.path.join(staging_dir, TEMP_ARCHIVE_DIR)
   if os.path.exists(temp_file_path):
     shutil.rmtree(temp_file_path)
   os.makedirs(temp_file_path)
   return (file_path, temp_file_path)
 
-def Readconfig(output_dir, input_file, current_version):
+def Readconfig(build_dir, input_file, current_version):
   """Reads config information from input file after setting default value of
   global variabes.
   """
@@ -193,8 +175,8 @@ def Readconfig(output_dir, input_file, current_version):
   return config
 
 def RunSystemCommand(cmd):
-  print 'Running [' + cmd + ']'
-  exit_code = os.system(cmd)
+  print 'Running', cmd
+  exit_code = subprocess.call(cmd)
   if (exit_code != 0):
     raise Exception("Error while running cmd: %s, exit_code: %s" %
                     (cmd, exit_code))
@@ -203,11 +185,15 @@ def CreateArchiveFile(options, staging_dir, current_version, prev_version):
   """Creates a new installer archive file after deleting any existing old file.
   """
   # First create an uncompressed archive file for the current build (chrome.7z)
-  lzma_exec = GetLZMAExec(options.output_dir)
-  archive_file = os.path.join(options.output_dir,
+  lzma_exec = GetLZMAExec(options.build_dir)
+  archive_file = os.path.join(options.build_dir,
                               options.output_name + ARCHIVE_SUFFIX)
-  cmd = '%s a -t7z "%s" "%s" -mx0' % (lzma_exec, archive_file,
-                                      os.path.join(staging_dir, CHROME_DIR))
+  cmd = [lzma_exec,
+         'a',
+         '-t7z',
+         archive_file,
+         os.path.join(staging_dir, CHROME_DIR),
+         '-mx0',]
   # There doesnt seem to be any way in 7za.exe to override existing file so
   # we always delete before creating a new one.
   if not os.path.exists(archive_file):
@@ -223,7 +209,7 @@ def CreateArchiveFile(options, staging_dir, current_version, prev_version):
   if options.last_chrome_installer:
     prev_archive_file = os.path.join(options.last_chrome_installer,
                                      options.output_name + ARCHIVE_SUFFIX)
-    patch_file = os.path.join(options.output_dir, patch_name_prefix +
+    patch_file = os.path.join(options.build_dir, patch_name_prefix +
                                                   PATCH_FILE_EXT)
     GenerateDiffPatch(options, prev_archive_file, archive_file, patch_file)
     compressed_archive_file = patch_name_prefix + '_' + \
@@ -234,9 +220,9 @@ def CreateArchiveFile(options, staging_dir, current_version, prev_version):
     compressed_archive_file = options.output_name + COMPRESSED_ARCHIVE_SUFFIX
     orig_file = archive_file
 
-  compressed_archive_file_path = os.path.join(options.output_dir,
+  compressed_archive_file_path = os.path.join(options.build_dir,
                                               compressed_archive_file)
-  CompressUsingLZMA(options.output_dir, compressed_archive_file_path, orig_file)
+  CompressUsingLZMA(options.build_dir, compressed_archive_file_path, orig_file)
 
   return compressed_archive_file
 
@@ -250,63 +236,88 @@ def PrepareSetupExec(options, staging_dir, current_version, prev_version):
       raise Exception(
           "To use DIFF for setup.exe, --last_chrome_installer is needed.")
     prev_setup_file = os.path.join(options.last_chrome_installer, SETUP_EXEC)
-    new_setup_file = os.path.join(options.output_dir, SETUP_EXEC)
-    patch_file = os.path.join(options.output_dir, SETUP_PATCH_FILE_PREFIX +
+    new_setup_file = os.path.join(options.build_dir, SETUP_EXEC)
+    patch_file = os.path.join(options.build_dir, SETUP_PATCH_FILE_PREFIX +
                                                   PATCH_FILE_EXT)
     GenerateDiffPatch(options, prev_setup_file, new_setup_file, patch_file)
     setup_file = SETUP_PATCH_FILE_PREFIX + '_' + current_version + \
                  '_from_' + prev_version + COMPRESSED_FILE_EXT
-    setup_file_path = os.path.join(options.output_dir, setup_file)
-    CompressUsingLZMA(options.output_dir, setup_file_path, patch_file)
+    setup_file_path = os.path.join(options.build_dir, setup_file)
+    CompressUsingLZMA(options.build_dir, setup_file_path, patch_file)
   else:
-    cmd = 'makecab.exe /D CompressionType=LZX /V1 /L "%s" "%s"' % (
-        options.output_dir, os.path.join(options.output_dir, SETUP_EXEC))
+    cmd = ['makecab.exe',
+           '/D', 'CompressionType=LZX',
+           '/V1',
+           '/L', options.build_dir,
+           os.path.join(options.build_dir, SETUP_EXEC),]
     RunSystemCommand(cmd)
-    setup_file = SETUP_EXEC[:len(SETUP_EXEC) - 1] + "_"
+    setup_file = SETUP_EXEC[:-1] + "_"
   return setup_file
 
 
-def CreateResourceInputFile(output_dir, setup_format, archive_file, setup_file):
+_RESOURCE_FILE_TEMPLATE = """\
+// This file is automatically generated by create_installer_archive.py.
+// It contains the resource entries that are going to be linked inside
+// mini_installer.exe. For each file to be linked there should be two
+// lines:
+// - The first line contains the output filename (without path) and the
+// type of the resource ('BN' - not compressed , 'BL' - LZ compressed,
+// 'B7' - LZMA compressed)
+// - The second line contains the path to the input file. Uses '/' to
+// separate path components.
+
+%(setup_file)s  %(setup_file_resource_type)s
+    "%(setup_file_path)s"
+
+%(archive_file)s  B7
+    "%(archive_file_path)s"
+"""
+
+
+def CreateResourceInputFile(
+    build_dir, setup_format, archive_file, setup_file, resource_file_path):
   """Creates resource input file (packed_files.txt) for mini_installer project.
 
   This method checks the format of setup.exe being used and according sets
   its resource type.
   """
   setup_resource_type = "BL"
-  if (options.setup_exe_format == "FULL"):
+  if (setup_format == "FULL"):
     setup_resource_type = "BN"
-  elif (options.setup_exe_format == "DIFF"):
+  elif (setup_format == "DIFF"):
     setup_resource_type = "B7"
-  setup_file_entry = "%s\t\t%s\n\"%s\"" % (setup_file, setup_resource_type,
-      os.path.join(output_dir, setup_file).replace("\\","/"))
 
-  archive_file_entry = "\n%s\t\tB7\n\"%s\"\n" % (archive_file,
-      os.path.join(output_dir, archive_file).replace("\\","/"))
-  output_file = os.path.join(output_dir, MINI_INSTALLER_INPUT_FILE)
-  f = open(output_file, 'w')
-  try:
-    f.write(PACKED_FILE_COMMENTS)
-    f.write(setup_file_entry)
-    f.write(archive_file_entry)
-  finally:
-    f.close()
+  # Expand the resource file template.
+  args = {
+      'setup_file': setup_file,
+      'setup_file_resource_type': setup_resource_type,
+      'setup_file_path':
+          os.path.join(build_dir, setup_file).replace("\\","/"),
+      'archive_file': archive_file,
+      'archive_file_path':
+          os.path.join(build_dir, archive_file).replace("\\","/"),
+      }
+  resource_file = _RESOURCE_FILE_TEMPLATE % args
+
+  with open(resource_file_path, 'w') as f:
+    f.write(resource_file)
 
 
 def main(options):
   """Main method that reads input file, creates archive file and write
   resource input file.
   """
-  current_version = BuildVersion(options.output_dir)
+  current_version = BuildVersion(options.build_dir)
 
-  config = Readconfig(options.output_dir, options.input_file, current_version)
+  config = Readconfig(options.build_dir, options.input_file, current_version)
 
-  (staging_dir, temp_dir) = MakeStagingDirectories(options.output_dir)
+  (staging_dir, temp_dir) = MakeStagingDirectories(options.staging_dir)
 
-  prev_version = GetPrevVersion(options.output_dir, temp_dir,
+  prev_version = GetPrevVersion(options.build_dir, temp_dir,
                                 options.last_chrome_installer)
 
   CopyAllFilesToStagingDir(config, options.distribution,
-                           staging_dir, options.output_dir)
+                           staging_dir, options.build_dir)
 
   version_numbers = current_version.split('.')
   current_build_number = version_numbers[2] + '.' + version_numbers[3]
@@ -323,34 +334,54 @@ def main(options):
   setup_file = PrepareSetupExec(options, staging_dir,
                                 current_build_number, prev_build_number)
 
-  CreateResourceInputFile(options.output_dir, options.setup_exe_format,
-                          archive_file, setup_file)
-
-
-if '__main__' == __name__:
-  option_parser = optparse.OptionParser()
-  option_parser.add_option('-o', '--output_dir', help='Output directory')
-  option_parser.add_option('-i', '--input_file', help='Input file')
-  option_parser.add_option('-d', '--distribution',
+  CreateResourceInputFile(options.build_dir, options.setup_exe_format,
+                          archive_file, setup_file, options.resource_file_path)
+
+def _ParseOptions():
+  parser = optparse.OptionParser()
+  parser.add_option('-i', '--input_file',
+      help='Input file describing which files to archive.')
+  parser.add_option('-o', '--build_dir',
+      help='Build directory. The paths in input_file are relative to this.')
+  parser.add_option('--staging_dir',
+      help='Staging directory where intermediate files and directories '
+           'will be created'),
+  parser.add_option('--resource_file_path',
+      help='The path where the resource file will be output. '
+           'Defaults to %s in the build directory.' %
+               MINI_INSTALLER_INPUT_FILE),
+  parser.add_option('-d', '--distribution',
       help='Name of Chromium Distribution. Optional.')
-  option_parser.add_option('-s', '--skip_rebuild_archive',
+  parser.add_option('-s', '--skip_rebuild_archive',
       default="False", help='Skip re-building Chrome.7z archive if it exists.')
-  option_parser.add_option('-l', '--last_chrome_installer',
-      help='Generate differential installer. The value of this parameter ' +
-           'specifies the directory that contains base versions of ' +
-           'setup.exe, courgette.exe (if --diff_algorithm is COURGETTE) ' +
+  parser.add_option('-l', '--last_chrome_installer',
+      help='Generate differential installer. The value of this parameter '
+           'specifies the directory that contains base versions of '
+           'setup.exe, courgette.exe (if --diff_algorithm is COURGETTE) '
            '& chrome.7z.')
-  option_parser.add_option('-p', '--archive_prefix',
-      help='Specifies a prefix to the archive path. Useful if building ' +
-           'multiple installer archives.')
-  option_parser.add_option('-f', '--setup_exe_format', default='COMPRESSED',
+  parser.add_option('-f', '--setup_exe_format', default='COMPRESSED',
       help='How setup.exe should be included {COMPRESSED|DIFF|FULL}.')
-  option_parser.add_option('-a', '--diff_algorithm', default='BSDIFF',
-      help='Diff algorithm to use when generating differential patches ' +
+  parser.add_option('-a', '--diff_algorithm', default='BSDIFF',
+      help='Diff algorithm to use when generating differential patches '
            '{BSDIFF|COURGETTE}.')
-  option_parser.add_option('-n', '--output_name', default='chrome',
+  parser.add_option('-n', '--output_name', default='chrome',
       help='Name used to prefix names of generated archives.')
 
-  options, args = option_parser.parse_args()
+  options, args = parser.parse_args()
+
+  if not options.build_dir:
+    parser.error('You must provide a build dir.')
+
+  if not options.staging_dir:
+    parser.error('You must provide a staging dir.')
+
+  if not options.resource_file_path:
+    options.options.resource_file_path = os.path.join(options.build_dir,
+                                                      MINI_INSTALLER_INPUT_FILE)
+
   print sys.argv
-  sys.exit(main(options))
+  return options
+
+
+if '__main__' == __name__:
+  sys.exit(main(_ParseOptions()))