Adding a script to generate au-geneate.zip file

generate_au_zip.py

+#!/usr/bin/python
+# -*- coding: utf-8 -*-
+
+"""
+  This script is to generate a zip file of delta-generator and it's dependencies
+"""
+import logging.handlers
+import optparse
+import os
+import re
+import string
+import subprocess
+import sys
+import time
+import tempfile
+import shutil
+# GLOBALS
+
+logging_format = '%(asctime)s - %(filename)s - %(levelname)-8s: %(message)s'
+date_format = '%Y/%m/%d %H:%M:%S'
+logging.basicConfig(level=logging.INFO, format=logging_format,
+                    datefmt=date_format)
+
+
+def BuildOptions():
+  """
+  Builds all required options using optparse
+  args: None

[scottz-goog, 2010/11/05 01:22:46]

If there are no arguments there is no need for this.

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+  return parser object
+  """
+
+  parser = optparse.OptionParser()

[scottz-goog, 2010/11/05 01:22:46]

Move these into main and drop this whole function

+  
+  parser.add_option('-d', '--debug', dest='debug', action='store_true',
+                    default=False, help='Verbose Default: "False"')
+  parser.add_option('-o','--output-dir', dest='output_dir', default=None,
+                    help='Specify the output location for copying the zipfile')
+  parser.add_option('-z','--zip-name', dest='zip_name',
+              default='au-generator.zip', help='Name of the zip file')
+  parser.add_option('-k','--keep-temp', dest='keep_temp', default=False,
+                    action='store_true', help='Keep the temp files...')
+  return parser
+
+
+
+def ValidateOptions(options, args):

[scottz-goog, 2010/11/05 01:22:46]

This should be in main as well. 

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+  """
+  Validates the set of given options on the command line before executing the 
+  rest of the script. This is to make sure that all the required options and 
+  option logic is validated before the execution.
+  Args:
+  options: options from parser object as input
+  args: args from parser objects 
+  return: False or True
+  """
+
+  if not options:
+    return False
+  if options.debug:

[scottz-goog, 2010/11/05 01:22:46]

Validating options should not be modifying things.

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+    logging.setLevel(logging.DEBUG)
+  logging.debug('Options are %s ', options)
+  if not options.output_dir:
+    options.output_dir='/tmp/au-generator'

[scottz-goog, 2010/11/05 01:22:46]

This should be the default set in the parser setup. 

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+  return True
+
+
+def CreateTempDir():
+  """
+  Generates a tempdir
+  args: None
+  return: name of tempdir 
+  """
+  temp_dir = tempfile.mkdtemp(suffix='au', prefix='tmp')
+  logging.info("Using tempdir = %s",temp_dir)
+  return temp_dir
+
+def CopyRequiredFiles(dest_files_root = None ):
+  if not dest_files_root:
+    logging.error("Invalid option passed for dest_files_root")

[scottz-goog, 2010/11/05 01:22:46]

shouldn't we be dying here?

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+
+  ldd_files = ['/usr/bin/delta_generator',

[scottz-goog, 2010/11/05 01:22:46]

ldd files?  Is there a better name for this? they look like random binaries to
me.

[Raja Aluri, 2010/11/05 18:58:01]

Added the explanation in the comments

+               '/usr/bin/bsdiff',
+               '/usr/bin/bspatch',
+               '/usr/bin/cgpt',
+              ]
+  static_files = ['~/trunk/src/scripts/common.sh',

[scottz-goog, 2010/11/05 01:22:46]

same deal. Static files in what regard? They never change? 

[Raja Aluri, 2010/11/05 18:58:01]

Added the explanation in the comments

+                  '~/trunk/src/scripts/cros_generate_update_payload',
+                  '~/trunk/src/scripts/chromeos-common.sh'
+                  ]
+  recurse_dirs = {'~/trunk/src/scripts/lib/shflags': 'lib/shflags'}
+
+  black_list = ['linux-vdso.so',
+                'libgcc_s.so',
+                'librt.so',
+                'libstdc',
+                'libgcc_s.so',
+                'libc.so',
+                'ld-linux-x86-64',
+                'libm.so',
+                'libdl.so',
+                'libresolv.so'
+               ]
+
+  all_files =ldd_files + static_files

[scottz-goog, 2010/11/05 01:22:46]

allfiles = ldd_files...

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+  all_files = map(os.path.expanduser,all_files)

[scottz-goog, 2010/11/05 01:22:46]

os.path.expanduser, all_files

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+  logging.debug("all files that need to be copied = %s",all_files)

[scottz-goog, 2010/11/05 01:22:46]

comma, SPACE please apply this to all areas. 

+  
+  for file_name in all_files:
+    if not os.path.isfile(file_name):
+      logging.error("file = %s does not exist",file_name)
+      sys.exit(1)
+
+  for file_name in ldd_files:

[scottz-goog, 2010/11/05 01:22:46]

this whole for loop should be broken out into a function that returns the list
of files you want.

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+    logging.info("Running ldd on %s",file_name)
+    cmd = ['/usr/bin/ldd',file_name]
+    stdout_data =''

[scottz-goog, 2010/11/05 01:22:46]

the format is 
"variable = value"

Note the spaces. You do that in most places please do it everywhere.

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+    stderr_data =''
+  
+    try:
+      proc = subprocess.Popen(cmd, stdout=subprocess.PIPE,
+                               stderr=subprocess.PIPE )

[scottz-goog, 2010/11/05 01:22:46]

align cmd and stderr= no space after PIPE)

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+      (stdout_data, stderr_data) = proc.communicate(input=None)
+    except subprocess.CalledProcessError as e:
+      logging.error("Command %s failed", cmd)
+      logging.error("error code %s", e.returncode)
+      logging.error("ouput %s", e.output)
+      raise
+
+    library_list =[]

[scottz-goog, 2010/11/05 01:22:46]

library_list = []

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+
+    if stdout_data:

[scottz-goog, 2010/11/05 01:22:46]

should you not always have stdout_data from ldd if the command completes without
error?

[Raja Aluri, 2010/11/05 18:58:01]

Just be extra cautious,because of ldd doesn't give structure output

[scottz-goog, 2010/11/09 03:52:50]

Then shouldn't you die if you don't have output? It isn't really being cautious
but adding ambiguity to your flow. 

+      logging.debug("ldd for %s = stdout = %s stderr =%s",file_name,
+                     stdout_data, stderr_data)
+      library_list = SplitAndStrip(stdout_data)
+
+    if library_list:

[scottz-goog, 2010/11/05 01:22:46]

as above shouldn't this always be set? If not aren't we in trouble already?

[Raja Aluri, 2010/11/05 18:58:01]

It can happen that all the output is stripped.
Like 'not a dyanamic executable' is a valid output and that gets stripped
completely.

[scottz-goog, 2010/11/09 03:52:50]

If that is the case you should add an if statement higher that checks for that
and acts accordingly. i.e 
if not stdout_data:
    return...

[Raja Aluri, 2010/11/09 22:59:45]

It can become empty after the _SplitAndStrip and still not an error. 

[Raja Aluri, 2010/11/09 22:59:45]

Done.

+      library_list = ExcludeBlacklist(library_list,black_list)
+
+    if library_list:
+      logging.info("Required libraries for %s=%s=",file_name,library_list)
+      all_files += library_list
+
+  for file_name in all_files:
+    logging.info("Copying file  %s to %s",file_name,dest_files_root)
+    shutil.copy2(file_name,dest_files_root)
+
+  for k,v in recurse_dirs.iteritems():

[scottz-goog, 2010/11/05 01:22:46]

please use variables that are descriptive. Nice usage of iteritems :) 

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+    logging.info("Processing directory %s",k)
+    full_path = os.path.expanduser(k)
+    if not os.path.isdir(full_path):
+      logging.error("Directory given for %s exapnded to %s doens't exist.",
+                    k,full_path)
+      sys.exit(1)
+  dest = os.path.join(dest_files_root,v)
+  logging.info("Copying directory %s to %s.",full_path,dest)
+  shutil.copytree(full_path,dest,symlinks=None,ignore=None)
+
+def CleanUP(temp_dir=None):
+  """
+  

[scottz-goog, 2010/11/05 01:22:46]

docstring?

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+  """
+  if not temp_dir:

[scottz-goog, 2010/11/05 01:22:46]

Instead of setting a default just make it a required argument   or is there a
reason to call CleanUp with None?

[Raja Aluri, 2010/11/05 18:58:01]

It's a harmless non-fatal operation. Don't want the script to fail for this. if
it's unmounting a mount or something, I would make not non-benign.

[scottz-goog, 2010/11/09 03:52:50]

If it is harmless then you should be running it all the time, it is definitely
an error if you are not sending a variable required for the function to work.

[Raja Aluri, 2010/11/09 22:59:45]

Changed to if os.path.exists(temp_dir): 
I should have done this check regardless.

+    return
+  shutil.rmtree(temp_dir, ignore_errors=True)
+  logging.info("Removed tempdir = %s",temp_dir)

[scottz-goog, 2010/11/05 01:22:46]

empty lines between functions

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+def GenerateZipFile(base_name=None,format='zip',root_dir=None):

[scottz-goog, 2010/11/05 01:22:46]

docstring

+  if not base_name:
+    return False

[scottz-goog, 2010/11/05 01:22:46]

same as above, if a function should never get a None value for an option then
don't set a default in the function definition.

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+  if not root_dir:
+    return False
+  logging.info("Generating zip file %s.zip with contents from %s",
+               base_name,root_dir)
+  current_dir = os.getcwd()
+  os.chdir(root_dir)
+  try:
+    output = subprocess.Popen(["zip", "-r","-9",base_name,'.'],
+                               stdout=subprocess.PIPE).communicate()[0]
+  except OSError, e:
+    print >>sys.stderr, "Execution failed:", e

[scottz-goog, 2010/11/05 01:22:46]

Why are we not using logging anymore?

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+  os.chdir(current_dir)

[scottz-goog, 2010/11/05 01:22:46]

This should be wrapped in a try: finally: so that it is always executed. 

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+  return False

[scottz-goog, 2010/11/05 01:22:46]

Always return False? 

[Raja Aluri, 2010/11/05 18:58:01]

Mistake. Fixed

+
+
+def ExcludeBlacklist(library_list=None,black_list=None):

[scottz-goog, 2010/11/05 01:22:46]

docstrings

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+  if not library_list:

[scottz-goog, 2010/11/05 01:22:46]

same as above with default arguments. Do you know what I mean when I say this?
I'd be happy to discuss this with you

[Raja Aluri, 2010/11/08 17:57:17]

Done.

+    return None
+  if not black_list:
+    return library_list
+  return_list = []
+  pattern =  r'%s' % '|'.join(black_list)
+  logging.debug("PATTERN: %s=",pattern)
+  for library in library_list:
+    if re.search(pattern, library):

[scottz-goog, 2010/11/05 01:22:46]

no need for regex or generating a regex pattern. Use the list. I imagine the
values you are using are very exact so a

if library in black_list:
  logging.....
  continue # to get out of hte for loop

[Raja Aluri, 2010/11/05 18:58:01]

The values are not exact.
libc.so.1 -> we only look for lib.c.so* and etc

[Raja Aluri, 2010/11/08 17:57:17]

This is not an exact match to the last dot. regex is needed

+      logging.debug("BLACK-LISTED = %s=",library)
+    else:
+      return_list.append(library)

[scottz-goog, 2010/11/05 01:22:46]

You should continue in the if statement and the else is not needed. 

[Raja Aluri, 2010/11/08 17:57:17]

Done.

+  logging.debug("Returning return_list=%s=",return_list)
+  return return_list
+
+def SplitAndStrip(data=None):

[scottz-goog, 2010/11/05 01:22:46]

default values, docstring..

The docstring should ahve information of "Data example of this...." so when I
read through what you are doing I know why you are actually parsing. 

[Raja Aluri, 2010/11/05 18:58:01]

Done.

[Raja Aluri, 2010/11/08 17:57:17]

Done.

+  if not data:
+    return None
+  return_list = []
+  for line in string.split(data,'\n'):
+    line = re.sub('.*not a dynamic executable.*','',line)
+    line = re.sub('.* =>\s+','',line)
+    line = re.sub('\(0x.*\)\s?','',line)
+    line = line.strip()
+    if not len(line):
+      continue
+    logging.debug("MATCHED line = %s",line)
+    return_list.append(line)
+  return return_list
+
+def CopyZipToFinalDestination(output_dir=None,zip_file_name=None):
+  """
+  Copies the generated zip file to a final destination
+  Args:
+    output_dir: Directory where the file should be copied to
+    zip_file_name: name of the zip file that should be copied
+  Returns:
+    boolean
+  """
+  if not output_dir:

[scottz-goog, 2010/11/05 01:22:46]

defaults.

Looking thrugh this it doesn't look like you should need any of these checks.

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+    logging.error("Empty output dir passed in. Returning False")
+    return False
+
+  if not zip_file_name:
+    logging.error("Empty zip file name passed in. Returning False")
+    return False
+  
+  if not os.path.isfile(zip_file_name):
+    logging.error("Zip file %s doesn't exist. Returning False",zip_file_name)
+    return False
+  
+  if not os.path.isdir(output_dir):
+    logging.debug("Creating %s",output_dir)
+    os.makedirs(output_dir)
+  logging.info("Copying %s to %s",zip_file_name,output_dir)
+  shutil.copy2(zip_file_name,output_dir)
+  return True
+def main():

[scottz-goog, 2010/11/05 01:22:46]

spaces

[Raja Aluri, 2010/11/05 18:58:01]

Done.

+  """
+  Main function to start the script
+  """
+
+  #SetConsoleLogger()

[scottz-goog, 2010/11/05 01:22:46]

Remove these

[Raja Aluri, 2010/11/05 18:58:01]

Done.

[Raja Aluri, 2010/11/08 17:57:17]

Done.

+  #logging.info('Logging to Console...')
+
+  parser = BuildOptions()
+  (options, args) = parser.parse_args()
+  if not ValidateOptions(options, args):
+    parser.print_help()
+    sys.exit(1)
+  temp_dir = CreateTempDir()
+  dest_files_root = os.path.join(temp_dir,'au-generator')

[scottz-goog, 2010/11/05 01:22:46]

Since you only use CreateTempDir for this purpose you might as well make it a
CreateTempAuGenDir and have it return the au-generator path you want

[Raja Aluri, 2010/11/05 18:58:01]

I also need the parent directory for storing the final .zip file. I don't want
to generate the .zip file in the files directory.

+  os.makedirs(dest_files_root) 
+  CopyRequiredFiles(dest_files_root=dest_files_root)
+  zip_file_name = os.path.join(temp_dir,options.zip_name)
+  GenerateZipFile(base_name=zip_file_name,root_dir=dest_files_root)
+  CopyZipToFinalDestination(output_dir=options.output_dir,
+                            zip_file_name=zip_file_name)
+  if not options.keep_temp:
+    CleanUP(temp_dir=temp_dir)
+    
+if __name__ == '__main__':
+  try:
+    main()
+  except Exception:
+    raise