[NaCl SDK] Fix create_nmf handling of missing libraries.

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 6ca83abbf574c8e0d5e8014d695706965213b31b..48b124bff39785ad8bad35faf37242f6441fc2f3 100755
--- a/native_client_sdk/src/tools/create_nmf.py
+++ b/native_client_sdk/src/tools/create_nmf.py
@@ -190,7 +190,7 @@ class ArchFile(object):
     self.path = path
     self.url = url
     self.arch = arch
-    if arch is None:
+    if not arch:
       self.arch = ParseElfHeader(path)[0]
 
   def __repr__(self):
@@ -235,15 +235,23 @@ class NmfUtils(object):
     self.remap = remap or {}
     self.pnacl = main_files and main_files[0].endswith('pexe')
 
-  def GleanFromObjdump(self, files):
+    for filename in self.main_files:
+      if not os.path.exists(filename):
+        raise Error('Input file not found: %s' % filename)
+      if not os.path.isfile(filename):
+        raise Error('Input is not a file: %s' % filename)
+
+  def GleanFromObjdump(self, files, arch):
     '''Get architecture and dependency information for given files
 
     Args:
-      files: A dict with key=filename and value=list or set of archs.  E.g.:
-          { '/path/to/my.nexe': ['x86-32']
-            '/path/to/lib64/libmy.so': ['x86-64'],
-            '/path/to/mydata.so': ['x86-32', 'x86-64'],
-            '/path/to/my.data': None }  # Indicates all architectures
+      files: A list of files to examine.
+          [ '/path/to/my.nexe',
+            '/path/to/lib64/libmy.so',
+            '/path/to/mydata.so',
+            '/path/to/my.data' ]
+      arch: The architecure we are looking for, or None to accept any
+            architecture.
 
     Returns: A tuple with the following members:
       input_info: A dict with key=filename and value=ArchFile of input files.
@@ -257,11 +265,22 @@ class NmfUtils(object):
       self.objdump = FindObjdumpExecutable()
       if not self.objdump:
         raise Error('No objdump executable found (see --help for more info)')
-    DebugPrint('GleanFromObjdump(%s)' % ([self.objdump, '-p'] + files.keys()))
-    proc = subprocess.Popen([self.objdump, '-p'] + files.keys(),
-                            stdout=subprocess.PIPE,
+
+    full_paths = set()
+    for filename in files:
+      if os.path.exists(filename):
+        full_paths.add(filename)
+      else:
+        for path in self.FindLibsInPath(filename):
+          full_paths.add(path)
+
+    cmd = [self.objdump, '-p'] + list(full_paths)
+    DebugPrint('GleanFromObjdump[%s](%s)' % (arch, cmd))
+    proc = subprocess.Popen(cmd, stdout=subprocess.PIPE,
                             stderr=subprocess.PIPE, bufsize=-1)
+
     input_info = {}
+    found_basenames = {}

[binji, 2013/05/28 22:52:41]

= set(), you never use the value

[Sam Clegg, 2013/05/28 23:04:40]

Its used below, unless I'm misunderstanding.

     needed = set()
     output, err_output = proc.communicate()
     if proc.returncode:
@@ -274,18 +293,27 @@ class NmfUtils(object):
       matched = FormatMatcher.match(line)
       if matched:
         filename = matched.group(1)
-        arch = OBJDUMP_ARCH_MAP[matched.group(2)]
-        if files[filename] is None or arch in files[filename]:
-          name = os.path.basename(filename)
-          input_info[filename] = ArchFile(
-              arch=arch,
-              name=name,
-              path=filename,
-              url='/'.join(self.lib_prefix + [ARCH_LOCATION[arch], name]))
+        file_arch = OBJDUMP_ARCH_MAP[matched.group(2)]
+        if arch and file_arch != arch:
+          continue
+        name = os.path.basename(filename)
+        found_basenames[name] = True
+        input_info[filename] = ArchFile(
+            arch=file_arch,
+            name=name,
+            path=filename,
+            url='/'.join(self.lib_prefix + [ARCH_LOCATION[file_arch], name]))
       matched = NeededMatcher.match(line)
       if matched:
-        if files[filename] is None or arch in files[filename]:
-          needed.add('/'.join([arch, matched.group(1)]))
+        if not arch or arch == file_arch:

[binji, 2013/05/28 22:52:41]

might be nicer to use
not(arch and arch != file_arch) to make the connection between this and the
above check clearer.

[binji, 2013/05/28 22:52:41]

now that I look at it closer, where is file_arch coming from here? It looks like
it is only set by line 296 above.

[Sam Clegg, 2013/05/28 23:04:40]

Just removed this condition as the continue above means it is alwasy true.

+          match = '/'.join([file_arch, matched.group(1)])
+          needed.add(match)
+          Trace("NEEDED: %s" % match)
+
+    for filename in files:
+      if os.path.basename(filename) not in found_basenames:
+        raise Error('Library not found [%s]: %s' % (arch, filename))

[binji, 2013/05/28 22:52:41]

seems like it would be useful to know which object needed this missing library.

[Sam Clegg, 2013/05/28 23:04:40]

Maybe.   That would require another re-factor since that information is not
available at this level.

+
     return input_info, needed
 
   def FindLibsInPath(self, name):
@@ -323,28 +351,36 @@ class NmfUtils(object):
 
     if dynamic:
       examined = set()
-      all_files, unexamined = self.GleanFromObjdump(
-          dict([(f, None) for f in self.main_files]))
-      for name, arch_file in all_files.items():
-        arch_file.url = name
+      all_files, unexamined = self.GleanFromObjdump(self.main_files, None)
+      for arch_file in all_files.itervalues():
+        arch_file.url = arch_file.path
         if unexamined:
           unexamined.add('/'.join([arch_file.arch, RUNNABLE_LD]))
+
       while unexamined:
         files_to_examine = {}
+
+        # Take all the currently unexamined files and group them
+        # by architecture.
         for arch_name in unexamined:
           arch, name = arch_name.split('/')
-          for path in self.FindLibsInPath(name):
-            files_to_examine.setdefault(path, set()).add(arch)
-        new_files, needed = self.GleanFromObjdump(files_to_examine)
-        all_files.update(new_files)
+          files_to_examine.setdefault(arch, []).append(name)
+
+        # Call GleanFromObjdump() for each architecture.
+        needed = set()
+        for arch, files in files_to_examine.iteritems():
+          new_files, new_needed = self.GleanFromObjdump(files, arch)
+          all_files.update(new_files)
+          needed |= new_needed
+
         examined |= unexamined
         unexamined = needed - examined
 
       # With the runnable-ld.so scheme we have today, the proper name of
       # the dynamic linker should be excluded from the list of files.
       ldso = [LD_NACL_MAP[arch] for arch in set(OBJDUMP_ARCH_MAP.values())]
-      for name, arch_map in all_files.items():
-        if arch_map.name in ldso:
+      for name, arch_file in all_files.items():
+        if arch_file.name in ldso:
           del all_files[name]
 
       self.needed = all_files
@@ -369,8 +405,9 @@ class NmfUtils(object):
     nexe_root = os.path.normcase(nexe_root)
 
     needed = self.GetNeeded()
-    for source, arch_file in needed.items():
+    for arch_file in needed.itervalues():
       urldest = arch_file.url
+      source = arch_file.path
 
       # for .nexe and .so files specified on the command line stage
       # them in paths relative to the .nexe (with the .nexe always
@@ -596,7 +633,7 @@ def main(argv):
                     help='Override the default "objdump" tool used to find '
                          'shared object dependencies',
                     metavar='TOOL')
-  parser.add_option('--no-default-libpath',
+  parser.add_option('--no-default-libpath', action='store_true',
                     help="Don't include the SDK default library paths")
   parser.add_option('--debug-libs', action='store_true',
                     help='Use debug library paths when constructing default '
@@ -635,12 +672,6 @@ def main(argv):
   if len(args) < 1:
     raise Error('No nexe files specified.  See --help for more info')
 
-  for filename in args:
-    if not os.path.exists(filename):
-      raise Error('Input file not found: %s' % filename)
-    if not os.path.isfile(filename):
-      raise Error('Input is not a file: %s' % filename)
-
   canonicalized = ParseExtraFiles(options.extra_files, sys.stderr)
   if canonicalized is None:
     parser.error('Bad --extra-files (-x) argument syntax')
@@ -657,6 +688,12 @@ def main(argv):
   else:
     path_prefix = []
 
+  for libpath in options.lib_path:
+    if not os.path.exists(libpath):
+      raise Error('Specified library path does not exist: %s' % libpath)
+    if not os.path.isdir(libpath):
+      raise Error('Specified library is not a directory: %s' % libpath)
+
   if not options.no_default_libpath:
     # Add default libraries paths to the end of the search path.
     config = options.debug_libs and 'Debug' or 'Release'
@@ -670,7 +707,7 @@ def main(argv):
                  remap=remap)
 
   nmf.GetManifest()
-  if options.output is None:
+  if not options.output:
     sys.stdout.write(nmf.GetJson())
   else:
     with open(options.output, 'w') as output: