From 37872833638b5d2e887fc8004bd6cb30f2c8b8dc Mon Sep 17 00:00:00 2001 From: Ted Mielczarek Date: Tue, 19 Jun 2012 09:24:49 -0400 Subject: [PATCH] bug 587073 - stop uploading duplicate dSYM files in the symbol package. r=nthomas --- toolkit/crashreporter/tools/symbolstore.py | 49 +++++++------ .../crashreporter/tools/unit-symbolstore.py | 73 ++++++++++++++++++- 2 files changed, 98 insertions(+), 24 deletions(-) diff --git a/toolkit/crashreporter/tools/symbolstore.py b/toolkit/crashreporter/tools/symbolstore.py index 807b06a90f5..727c4273647 100755 --- a/toolkit/crashreporter/tools/symbolstore.py +++ b/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") - module_line = cmd.next() + proc = subprocess.Popen([self.dump_syms] + arch.split() + [file], + 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", - full_path, compressed_file], stdout=open("NUL:","w"), stderr=STDOUT) + success = subprocess.call(["makecab.exe", "/D", "CompressionType=LZX", "/D", + "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), - "-i:" + os.path.basename(streamFilename), "-s:srcsrv"], - cwd=os.path.dirname(stream_output_path)) + subprocess.call([pdbstr, "-w", "-p:" + os.path.basename(debug_file), + "-i:" + os.path.basename(streamFilename), "-s:srcsrv"], + 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 \ - call([self.objcopy, '--add-gnu-debuglink=%s' % file_dbg, file]) == 0: + if subprocess.call([self.objcopy, '--only-keep-debug', file, file_dbg]) == 0 and \ + 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]), - file)) + subprocess.call(["dsymutil"] + [a.replace('-a ', '--arch=') for a in self.archs if a] + + [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)], - cwd=os.path.dirname(file), - stdout=open("/dev/null","w"), stderr=STDOUT) + success = subprocess.call(["tar", "cjf", full_path, os.path.basename(file)], + cwd=os.path.dirname(file), + stdout=open("/dev/null","w"), stderr=subprocess.STDOUT) if success == 0 and os.path.exists(full_path): print rel_path diff --git a/toolkit/crashreporter/tools/unit-symbolstore.py b/toolkit/crashreporter/tools/unit-symbolstore.py index 24a29e74bde..0a0bcbc717b 100644 --- a/toolkit/crashreporter/tools/unit-symbolstore.py +++ b/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()