bug 587073 - stop uploading duplicate dSYM files in the symbol package. r=nthomas

toolkit/crashreporter/tools/symbolstore.py

@@ -27,7 +27,7 @@ import re
 import shutil
 import textwrap
 import fnmatch
-from subprocess import call, Popen, PIPE, STDOUT
+import subprocess
 from optparse import OptionParser
 
 # Utility classes
@@ -229,7 +229,7 @@ class SVNFileInfo(VCSFileInfo):
         return self.file
 
 def read_output(*args):
-    (stdout, _) = Popen(args=args, stdout=PIPE).communicate()
+    (stdout, _) = subprocess.Popen(args=args, stdout=subprocess.PIPE).communicate()
     return stdout.rstrip()
 
 class HGRepoInfo:
@@ -471,15 +471,17 @@ class Dumper:
         """Dump symbols from this file into a symbol file, stored
         in the proper directory structure in  |symbol_path|."""
         print >> sys.stderr, "Processing file: %s" % file
+        sys.stderr.flush()
         result = False
         sourceFileStream = ''
         # tries to get the vcs root from the .mozconfig first - if it's not set
         # the tinderbox vcs path will be assigned further down
         vcs_root = os.environ.get("SRCSRV_ROOT")
-        for arch in self.archs:
+        for arch_num, arch in enumerate(self.archs):
             try:
-                cmd = os.popen("%s %s %s" % (self.dump_syms, arch, file), "r")
+                proc = subprocess.Popen([self.dump_syms] + arch.split() + [file],
-                module_line = cmd.next()
+                                        stdout=subprocess.PIPE)
+                module_line = proc.stdout.next()
                 if module_line.startswith("MODULE"):
                     # MODULE os cpu guid debug_file
                     (guid, debug_file) = (module_line.split())[3:5]
@@ -498,17 +500,17 @@ class Dumper:
                     f = open(full_path, "w")
                     f.write(module_line)
                     # now process the rest of the output
-                    for line in cmd:
+                    for line in proc.stdout:
                         if line.startswith("FILE"):
                             # FILE index filename
-                            (x, index, filename) = line.split(None, 2)
+                            (x, index, filename) = line.rstrip().split(None, 2)
                             if sys.platform == "sunos5":
                                 for srcdir in self.srcdirs:
                                     start = filename.find(self.srcdir)
                                     if start != -1:
                                         filename = filename[start:]
                                         break
-                            filename = self.FixFilenameCase(filename.rstrip())
+                            filename = self.FixFilenameCase(filename)
                             sourcepath = filename
                             if self.vcsinfo:
                                 (filename, rootname) = GetVCSFilename(filename, self.srcdirs)
@@ -527,14 +529,15 @@ class Dumper:
                             # we want to return true only if at least one line is not a MODULE or FILE line
                             result = True
                     f.close()
-                    cmd.close()
+                    proc.wait()
                     # we output relative paths so callers can get a list of what
                     # was generated
                     print rel_path
                     if self.srcsrv and vcs_root:
                         # add source server indexing to the pdb file
                         self.SourceServerIndexing(file, guid, sourceFileStream, vcs_root)
-                    if self.copy_debug:
+                    # only copy debug the first time if we have multiple architectures
+                    if self.copy_debug and arch_num == 0:
                         self.CopyDebug(file, debug_file, guid)
             except StopIteration:
                 pass
@@ -593,8 +596,10 @@ class Dumper_Win32(Dumper):
         # try compressing it
         compressed_file = os.path.splitext(full_path)[0] + ".pd_"
         # ignore makecab's output
-        success = call(["makecab.exe", "/D", "CompressionType=LZX", "/D", "CompressionMemory=21",
+        success = subprocess.call(["makecab.exe", "/D", "CompressionType=LZX", "/D",
-                       full_path, compressed_file], stdout=open("NUL:","w"), stderr=STDOUT)
+                                   "CompressionMemory=21",
+                                   full_path, compressed_file],
+                                  stdout=open("NUL:","w"), stderr=subprocess.STDOUT)
         if success == 0 and os.path.exists(compressed_file):
             os.unlink(full_path)
             print os.path.splitext(rel_path)[0] + ".pd_"
@@ -611,9 +616,9 @@ class Dumper_Win32(Dumper):
         if self.copy_debug:
             pdbstr_path = os.environ.get("PDBSTR_PATH")
             pdbstr = os.path.normpath(pdbstr_path)
-            call([pdbstr, "-w", "-p:" + os.path.basename(debug_file),
+            subprocess.call([pdbstr, "-w", "-p:" + os.path.basename(debug_file),
-                  "-i:" + os.path.basename(streamFilename), "-s:srcsrv"],
+                             "-i:" + os.path.basename(streamFilename), "-s:srcsrv"],
-                 cwd=os.path.dirname(stream_output_path))
+                            cwd=os.path.dirname(stream_output_path))
             # clean up all the .stream files when done
             os.remove(stream_output_path)
         return result
@@ -636,8 +641,8 @@ class Dumper_Linux(Dumper):
         # .gnu_debuglink section to the object, so the debugger can
         # actually load our debug info later.
         file_dbg = file + ".dbg"
-        if call([self.objcopy, '--only-keep-debug', file, file_dbg]) == 0 and \
+        if subprocess.call([self.objcopy, '--only-keep-debug', file, file_dbg]) == 0 and \
-           call([self.objcopy, '--add-gnu-debuglink=%s' % file_dbg, file]) == 0:
+           subprocess.call([self.objcopy, '--add-gnu-debuglink=%s' % file_dbg, file]) == 0:
             rel_path = os.path.join(debug_file,
                                     guid,
                                     debug_file + ".dbg")
@@ -700,8 +705,8 @@ class Dumper_Mac(Dumper):
         if os.path.exists(dsymbundle):
             shutil.rmtree(dsymbundle)
         # dsymutil takes --arch=foo instead of -a foo like everything else
-        os.system("dsymutil %s %s >/dev/null" % (' '.join([a.replace('-a ', '--arch=') for a in self.archs]),
+        subprocess.call(["dsymutil"] + [a.replace('-a ', '--arch=') for a in self.archs if a]
-                                      file))
+                        + [file])
         if not os.path.exists(dsymbundle):
             # dsymutil won't produce a .dSYM for files without symbols
             return False
@@ -727,9 +732,9 @@ class Dumper_Mac(Dumper):
                                 os.path.basename(file) + ".tar.bz2")
         full_path = os.path.abspath(os.path.join(self.symbol_path,
                                                   rel_path))
-        success = call(["tar", "cjf", full_path, os.path.basename(file)],
+        success = subprocess.call(["tar", "cjf", full_path, os.path.basename(file)],
-                       cwd=os.path.dirname(file),
+                                  cwd=os.path.dirname(file),
-                       stdout=open("/dev/null","w"), stderr=STDOUT)
+                                  stdout=open("/dev/null","w"), stderr=subprocess.STDOUT)
         if success == 0 and os.path.exists(full_path):
             print rel_path
 

toolkit/crashreporter/tools/unit-symbolstore.py

@@ -3,7 +3,7 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-import os, tempfile, unittest, shutil, struct, platform
+import os, tempfile, unittest, shutil, struct, platform, subprocess
 import symbolstore
 
 # Some simple functions to mock out files that the platform-specific dumpers will accept.
@@ -34,7 +34,7 @@ extension = {'Windows': ".pdb",
 def add_extension(files):
     return [f + extension for f in files]
 
-class TestExclude(unittest.TestCase):
+class HelperMixin(object):
     """
     Test that passing filenames to exclude from processing works.
     """
@@ -54,6 +54,7 @@ class TestExclude(unittest.TestCase):
                 os.makedirs(d)
             writer(f)
 
+class TestExclude(HelperMixin, unittest.TestCase):
     def test_exclude_wildcard(self):
         """
         Test that using an exclude list with a wildcard pattern works.
@@ -92,5 +93,73 @@ class TestExclude(unittest.TestCase):
         expected.sort()
         self.assertEqual(processed, expected)
 
+def popen_factory(stdouts):
+    """
+    Generate a class that can mock subprocess.Popen. |stdouts| is an iterable that
+    should return an iterable for the stdout of each process in turn.
+    """
+    class mock_popen(object):
+        def __init__(self, args, *args_rest, **kwargs):
+            self.stdout = stdouts.next()
+
+        def wait(self):
+            return 0
+    return mock_popen
+
+def mock_dump_syms(module_id, filename):
+    return ["MODULE os x86 %s %s" % (module_id, filename),
+            "FILE 0 foo.c",
+            "PUBLIC xyz 123"]
+
+class TestCopyDebugUniversal(HelperMixin, unittest.TestCase):
+    """
+    Test that CopyDebug does the right thing when dumping multiple architectures.
+    """
+    def setUp(self):
+        HelperMixin.setUp(self)
+        self.symbol_dir = tempfile.mkdtemp()
+        self._subprocess_call = subprocess.call
+        subprocess.call = self.mock_call
+        self._subprocess_popen = subprocess.Popen
+        subprocess.Popen = popen_factory(self.next_mock_stdout())
+        self.stdouts = []
+        
+    def tearDown(self):
+        HelperMixin.tearDown(self)
+        shutil.rmtree(self.symbol_dir)
+        subprocess.call = self._subprocess_call
+        subprocess.Popen = self._subprocess_popen
+
+    def mock_call(self, args, **kwargs):
+        if args[0].endswith("dsymutil"):
+            filename = args[-1]
+            os.makedirs(filename + ".dSYM")
+        return 0
+
+    def next_mock_stdout(self):
+        if not self.stdouts:
+            yield iter([])
+        for s in self.stdouts:
+            yield iter(s)
+    
+    def test_copy_debug_universal(self):
+        """
+        Test that dumping symbols for multiple architectures only copies debug symbols once
+        per file.
+        """
+        copied = []
+        def mock_copy_debug(filename, debug_file, guid):
+            copied.append(filename[len(self.symbol_dir):] if filename.startswith(self.symbol_dir) else filename)
+        self.add_test_files(add_extension(["foo"]))
+        self.stdouts.append(mock_dump_syms("X" * 33, add_extension(["foo"])[0]))
+        self.stdouts.append(mock_dump_syms("Y" * 33, add_extension(["foo"])[0]))
+        d = symbolstore.GetPlatformSpecificDumper(dump_syms="dump_syms",
+                                                  symbol_path=self.symbol_dir,
+                                                  copy_debug=True,
+                                                  archs="abc xyz")
+        d.CopyDebug = mock_copy_debug
+        self.assertTrue(d.Process(self.test_dir))
+        self.assertEqual(1, len(copied))
+
 if __name__ == '__main__':
   unittest.main()