create_nmf improvements

native_client_sdk/src/tools/create_nmf.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.
 
 from __future__ import with_statement
 
 import errno
 import optparse
 import os
@@ skipping 11 matching lines @@
 NeededMatcher = re.compile('^ *NEEDED *([^ ]+)\n$')
 FormatMatcher = re.compile('^(.+):\\s*file format (.+)\n$')
 
 FORMAT_ARCH_MAP = {
     # Names returned by Linux's objdump:
     'elf64-x86-64': 'x86-64',
     'elf32-i386': 'x86-32',
     # Names returned by x86_64-nacl-objdump:
     'elf64-nacl': 'x86-64',
     'elf32-nacl': 'x86-32',
-    }
+}
 
 ARCH_LOCATION = {
     'x86-32': 'lib32',
     'x86-64': 'lib64',
 }
 
 NAME_ARCH_MAP = {
     '32.nexe': 'x86-32',
     '64.nexe': 'x86-64',
     'arm.nexe': 'arm'
@@ skipping 19 matching lines @@
 def DebugPrint(message):
   if _debug_mode:
     sys.stderr.write('%s\n' % message)
     sys.stderr.flush()
 
 
 class Error(Exception):
   '''Local Error class for this file.'''
   pass
 
-
 class ArchFile(object):
   '''Simple structure containing information about
 
   Attributes:
     arch: Architecture of this file (e.g., x86-32)
     filename: name of this file
     path: Full path to this file on the build system
     url: Relative path to file in the staged web directory.
         Used for specifying the "url" attribute in the nmf file.'''
   def __init__(self, arch, name, path='', url=None):
     self.arch = arch
     self.name = name
     self.path = path
     self.url = url or '/'.join([arch, name])
 
+  def __repr__(self):
+    return "ArchFile: %s" % self.path

[binji, 2012/08/01 04:06:46]

repr(foo) is usually "<foo ...>"

[Sam Clegg, 2012/08/01 17:21:03]

Done.

+
   def __str__(self):
     '''Return the file path when invoked with the str() function'''
     return self.path
 
 
 class NmfUtils(object):
   '''Helper class for creating and managing nmf files
 
   Attributes:
     manifest: A JSON-structured dict containing the nmf structure
     needed: A dict with key=filename and value=ArchFile (see GetNeeded)
   '''
 
   def __init__(self, main_files=None, objdump='x86_64-nacl-objdump',
                lib_path=None, extra_files=None, lib_prefix=None,
                toolchain=None, remap={}):
-    ''' Constructor
+    '''Constructor
 
     Args:
       main_files: List of main entry program files.  These will be named
           files->main.nexe for dynamic nexes, and program for static nexes
       objdump: path to x86_64-nacl-objdump tool (or Linux equivalent)
       lib_path: List of paths to library directories
       extra_files: List of extra files to include in the nmf
       lib_prefix: A list of path components to prepend to the library paths,
           both for staging the libraries and for inclusion into the nmf file.
           Examples:  ['..'], ['lib_dir']
@@ skipping 84 matching lines @@
     Returns:
       A dict with key=filename and value=ArchFile of input files.
           Includes the input files as well, with arch filled in if absent.
           Example: { '/path/to/my.nexe': ArchFile(my.nexe),
                      '/path/to/libfoo.so': ArchFile(libfoo.so) }'''
     if self.needed:
       return self.needed
 
     runnable = (self.toolchain != 'newlib' and self.toolchain != 'pnacl')
     DebugPrint('GetNeeded(%s)' % self.main_files)
-    if runnable:    
+    if runnable:
       examined = set()
       all_files, unexamined = self.GleanFromObjdump(
           dict([(file, None) for file in self.main_files]))
       for name, arch_file in all_files.items():
         arch_file.url = name
         if unexamined:
           unexamined.add('/'.join([arch_file.arch, RUNNABLE_LD]))
       while unexamined:
         files_to_examine = {}
         for arch_name in unexamined:
@@ skipping 12 matching lines @@
           del all_files[name]
       self.needed = all_files
     else:
       need = {}
       for filename in self.main_files:
         arch = filename.split('_')[-1]
         arch = NAME_ARCH_MAP[arch]
         url = os.path.split(filename)[1]
         need[filename] = ArchFile(arch=arch, name=os.path.basename(filename),
                                   path=filename, url=url)
-      self.needed = need        
+      self.needed = need
+
     return self.needed
 
   def StageDependencies(self, destination_dir):
     '''Copies over the dependencies into a given destination directory
 
     Each library will be put into a subdirectory that corresponds to the arch.
 
     Args:
       destination_dir: The destination directory for staging the dependencies
     '''
     needed = self.GetNeeded()
     for source, arch_file in needed.items():
-      destination = os.path.join(destination_dir,
-                                 urllib.url2pathname(arch_file.url))
-      try:
-        os.makedirs(os.path.dirname(destination))
-      except OSError as exception_info:
-        if exception_info.errno != errno.EEXIST:
-          raise
+      urldest = urllib.url2pathname(arch_file.url)
+      if source.endswith('.nexe') and source in self.main_files:

[binji, 2012/08/01 04:06:46]

I don't think we should make any determination based on the filename. The user
can name their nexe whatever they like.

[Sam Clegg, 2012/08/01 17:21:03]

I'm only mimicking what _GenerateManifest does.  The job of this function is to
copy files into place to match the manifest right?  So I have to do the same
thing.  You could argue that _GenerateManifest is wrong to use endswith() I
guess.

[binji, 2012/08/01 18:05:20]

ok

+        urldest = os.path.basename(urldest)
+
+      destination = os.path.join(destination_dir, urldest)
+
+      # make sure target dir exists
+      dirname = os.path.dirname(destination)
+      if not os.path.exists(dirname):

[binji, 2012/08/01 04:06:46]

IMO, catching the exception is better. Testing for existence doesn't mean
os.makedirs will succeed.

[binji, 2012/08/01 18:05:20]

opinion on this?

[Sam Clegg, 2012/08/01 18:26:39]

Well.. it pretty much guarantees that it won't fail with EEXIST doesn't, which? 
 Which is exactly what the old code does.

I suppose you could argue that EEXISTS could still fire if some other process
makes that directory between the check and the call to makedirs?

Is that what you are getting at?  Some kind of protection against
multi-process/multi-thread race condition?

I would argue that if you have another process messing with the tree while this
script is running it is going to fail
at some point for sure :)    For example, if another process were to delete the
nexe files between GenerateManifest and
StageDependancies.  

On balance I guess the try/catch version is marginally safer,  but I find it
offensive that it takes 5 lines to call mkdir :)   Maybe I'll split it into a
separate function ?

http://stackoverflow.com/questions/600268/mkdir-p-functionality-in-python

+        Trace("mkdir: %s" % dirname)
+        os.makedirs(dirname)
+
       if (os.path.normcase(os.path.abspath(source)) !=
           os.path.normcase(os.path.abspath(destination))):
+        Trace("copy: %s -> %s" % (source, destination))
         shutil.copy2(source, destination)
 
   def _GenerateManifest(self):
     '''Create a JSON formatted dict containing the files
 
     NaCl will map url requests based on architecture.  The startup NEXE
     can always be found under the top key PROGRAM.  Additional files are under
     the FILES key further mapped by file name.  In the case of 'runnable' the
     PROGRAM key is populated with urls pointing the runnable-ld.so which acts
     as the startup nexe.  The application itself, is then placed under the
     FILES key mapped as 'main.exe' instead of it's original name so that the
     loader can find it.'''
     manifest = { FILES_KEY: {}, PROGRAM_KEY: {} }
     runnable = (self.toolchain != 'newlib' and self.toolchain != 'pnacl')
 
     needed = self.GetNeeded()
-    for need in needed:
-      archinfo = needed[need]
+    for need, archinfo in needed.items():
       urlinfo = { URL_KEY: archinfo.url }
       name = archinfo.name
 
       # If starting with runnable-ld.so, make that the main executable.
       if runnable:
         if need.endswith(RUNNABLE_LD):
           manifest[PROGRAM_KEY][archinfo.arch] = urlinfo
           continue
 
       # For the main nexes:
@@ skipping 24 matching lines @@
 
   def GetJson(self):
     '''Returns the Manifest as a JSON-formatted string'''
     pretty_string = json.dumps(self.GetManifest(), indent=2)
     # json.dumps sometimes returns trailing whitespace and does not put
     # a newline at the end.  This code fixes these problems.
     pretty_lines = pretty_string.split('\n')
     return '\n'.join([line.rstrip() for line in pretty_lines]) + '\n'
 
 
-def ErrorOut(text):
-  sys.stderr.write(text + '\n')
-  sys.exit(1)
-
-
 def DetermineToolchain(objdump):
   objdump = objdump.replace('\\', '/')
   paths = objdump.split('/')
   count = len(paths)
   for index in range(count - 2, 0, -1):
     if paths[index] == 'toolchain':
       if paths[index + 1].endswith('newlib'):
         return 'newlib'
       if paths[index + 1].endswith('glibc'):
         return 'glibc'
-  ErrorOut('Could not deternime which toolchain to use.')
+  raise Error('Could not deternime which toolchain to use.')
 

[binji, 2012/08/01 04:06:46]

style: Two newlines between top-level definitions.

[Sam Clegg, 2012/08/01 17:21:03]

Done.

+def Trace(msg):
+  if Trace.verbose:
+    sys.stderr.write(str(msg) + '\n')
+
+Trace.verbose = False
 
 def Main(argv):
   parser = optparse.OptionParser(
       usage='Usage: %prog [options] nexe [extra_libs...]')
   parser.add_option('-o', '--output', dest='output',
                     help='Write manifest file to FILE (default is stdout)',
                     metavar='FILE')
   parser.add_option('-D', '--objdump', dest='objdump', default='objdump',
                     help='Use TOOL as the "objdump" tool to run',
                     metavar='TOOL')
   parser.add_option('-L', '--library-path', dest='lib_path',
                     action='append', default=[],
                     help='Add DIRECTORY to library search path',
                     metavar='DIRECTORY')
   parser.add_option('-s', '--stage-dependencies', dest='stage_dependencies',
                     help='Destination directory for staging libraries',
                     metavar='DIRECTORY')
   parser.add_option('-r', '--remove', dest='remove',
                     help='Remove the prefix from the files.',
                     metavar='PATH')
   parser.add_option('-t', '--toolchain', dest='toolchain',
                     help='Add DIRECTORY to library search path',
                     default=None, metavar='TOOLCHAIN')
   parser.add_option('-n', '--name', dest='name',
                     help='Rename FOO as BAR',
                     action='append', default=[], metavar='FOO,BAR')
+  parser.add_option('-v', '--verbose',
+                    help='Verbose output', action='store_true')
   (options, args) = parser.parse_args(argv)
-  
+  if options.verbose:
+    Trace.verbose = True
+
+  if len(args) < 1:
+    raise Error("No nexe files specified.  See --help for more info")

[binji, 2012/08/01 04:06:46]

maybe just display help in this case?

[Sam Clegg, 2012/08/01 17:21:03]

The convention seems to be to tell the user what he did 
wrong on one line (and sometimes point to the help option)..
unless there really did as for --help.

Try.. ld --bad-arg.. vi --bad-arg .. or even create_mkb.py 
--bad-arg (which is automatically generated by optparse).

[binji, 2012/08/01 18:05:20]

ok

+
+  remap = {}

[binji, 2012/08/01 04:06:46]

I prefer initialization closer to first use.

[Sam Clegg, 2012/08/01 17:21:03]

Ooops.  Didn't mean to move that.

Done.

   if not options.toolchain:
     options.toolchain = DetermineToolchain(os.path.abspath(options.objdump))
 
   if options.toolchain not in ['newlib', 'glibc', 'pnacl']:
-    ErrorOut('Unknown toolchain: ' + str(options.toolchain))
+    raise Error('Unknown toolchain: ' + str(options.toolchain))
 
-  if len(args) < 1:
-    parser.print_usage()
-    sys.exit(1)
-
-  remap = {}
   for ren in options.name:
     parts = ren.split(',')
     if len(parts) != 2:
-      ErrorOut('Expecting --name=<orig_arch.so>,<new_name.so>')
+      raise Error('Expecting --name=<orig_arch.so>,<new_name.so>')
     remap[parts[0]] = parts[1]
 
   nmf = NmfUtils(objdump=options.objdump,
                  main_files=args,
                  lib_path=options.lib_path,
                  toolchain=options.toolchain,
                  remap=remap)
 
   manifest = nmf.GetManifest()
   if options.output is None:
     sys.stdout.write(nmf.GetJson())
   else:
     with open(options.output, 'w') as output:
       output.write(nmf.GetJson())
 
   if options.stage_dependencies:
+    Trace("Staging dependancies...")

[binji, 2012/08/01 04:06:46]

sp: dependencies 

[Sam Clegg, 2012/08/01 17:21:03]

Done.

     nmf.StageDependencies(options.stage_dependencies)
 
+  return 0
+
 
 # Invoke this file directly for simple testing.
 if __name__ == '__main__':
-  sys.exit(Main(sys.argv[1:]))
+  try:
+    rtn = Main(sys.argv[1:])
+  except Error, e:

[binji, 2012/08/01 04:06:46]

How does this output differ from the default output when a thrown exception
isn't caught?

[Sam Clegg, 2012/08/01 17:21:03]

When an Error (local class) is thrown it represents an 
error that the user should see.

We catch it here and display it for the user rather than 
letting the interpreter catch it on the way out and display
a backtrace.  This is a direct replacement for code that 
calls sys.exit() after printing an error message (i.e. times
when a backtrace is not wanted/needed/appropriate).

In my experience this is much better to use a local 
exception class than have code calling sys.exit().

It allows for much nicer code reuse and testing.

[binji, 2012/08/01 18:05:20]

sgtm

+    print "%s: %s" % (os.path.basename(__file__), e)
+    rtn = 1
+  sys.exit(rtn)