create_nmf improvements

native_client_sdk/src/tools/create_nmf.py

Index: native_client_sdk/src/tools/create_nmf.py
diff --git a/native_client_sdk/src/tools/create_nmf.py b/native_client_sdk/src/tools/create_nmf.py
index eb004c4a0cfc00cb4899312f45698d51240845b4..092358808b72f48c580d1340a555a58cefd4fddd 100755
--- a/native_client_sdk/src/tools/create_nmf.py
+++ b/native_client_sdk/src/tools/create_nmf.py
@@ -29,7 +29,7 @@ FORMAT_ARCH_MAP = {
     # Names returned by x86_64-nacl-objdump:
     'elf64-nacl': 'x86-64',
     'elf32-nacl': 'x86-32',
-    }
+}
 
 ARCH_LOCATION = {
     'x86-32': 'lib32',
@@ -69,7 +69,6 @@ class Error(Exception):
   '''Local Error class for this file.'''
   pass
 
-
 class ArchFile(object):
   '''Simple structure containing information about
 
@@ -85,6 +84,9 @@ class ArchFile(object):
     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
@@ -101,7 +103,7 @@ class NmfUtils(object):
   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
@@ -206,7 +208,7 @@ class NmfUtils(object):
 
     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]))
@@ -239,7 +241,8 @@ class NmfUtils(object):
         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):
@@ -252,15 +255,21 @@ class NmfUtils(object):
     '''
     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):
@@ -277,8 +286,7 @@ class NmfUtils(object):
     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
 
@@ -323,11 +331,6 @@ class NmfUtils(object):
     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('/')
@@ -338,8 +341,13 @@ def DetermineToolchain(objdump):
         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(
@@ -366,23 +374,26 @@ def Main(argv):
   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,
@@ -399,9 +410,17 @@ def Main(argv):
       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)