Merge lp:~mbp/bzr/417881-selftest-no-apport into lp:bzr

[Martin Pool]

[Martin Pool (mbp)]

Turn off apport during selftest, and put the traceback at the end where it's more useful to developers and less likely to be scrolled off by plugins.

Fixes bug 417881

=== modified file 'NEWS'
--- NEWS	2010-02-02 10:38:47 +0000
+++ NEWS	2010-02-03 00:05:22 +0000
@@ -19,6 +19,21 @@
 New Features
 ************
 
+* If the Apport crash-reporting tool is available, bzr crashes are now
+  stored into the ``/var/crash`` apport spool directory, and the user is
+  invited to report them to the developers from there, either
+  automatically or by running ``apport-bug``.  No information is sent
+  without specific permission from the user.  (Martin Pool, #515052)
+
+Testing
+*******
+
+* Don't use Apport for errors while loading or running tests.
+  (Martin Pool, #417881)
+
+* Stop sending apport crash files to ``.cache`` in the directory from
+  which ``bzr selftest`` was run.  (Martin Pool, #422350)
+
 bzr 2.1.0 (not released yet)
 ############################
 
 
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-01-29 10:36:23 +0000
+++ bzrlib/builtins.py	2010-02-03 00:05:22 +0000
@@ -3524,6 +3524,13 @@
 
         # Make deprecation warnings visible, unless -Werror is set
         symbol_versioning.activate_deprecation_warnings(override=False)
+       
+        # Whatever you normally want, for the purposes of running selftest you
+        # probably actually want to see the exception, not to turn it into an
+        # apport failure.  This is specifically turned off again for tests of
+        # apport.  We turn it off here so that eg a SyntaxError loading the
+        # tests is caught.
+        os.environ['APPORT_DISABLE'] = '1'
 
         if cache_dir is not None:
             tree_creator.TreeCreator.CACHE_ROOT = osutils.abspath(cache_dir)
 
=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2009-12-17 10:01:25 +0000
+++ bzrlib/config.py	2010-02-03 00:05:22 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2007, 2008 Canonical Ltd
+# Copyright (C) 2005, 2007, 2008, 2010 Canonical Ltd
 #   Authors: Robert Collins <[e-mail]>
 #            and others
 #
@@ -866,12 +866,16 @@
 
     This doesn't implicitly create it.
 
-    On Windows it's in the config directory; elsewhere in the XDG cache directory.
+    On Windows it's in the config directory; elsewhere it's /var/crash
+    which may be monitored by apport.  It can be overridden by
+    $APPORT_CRASH_DIR.
     """
     if sys.platform == 'win32':
         return osutils.pathjoin(config_dir(), 'Crash')
     else:
-        return osutils.pathjoin(xdg_cache_dir(), 'crash')
+        # XXX: hardcoded in apport_python_hook.py; therefore here too -- mbp
+        # 2010-01-31
+        return os.environ.get('APPORT_CRASH_DIR', '/var/crash')
 
 
 def xdg_cache_dir():
 
=== modified file 'bzrlib/crash.py'
--- bzrlib/crash.py	2009-08-20 05:47:53 +0000
+++ bzrlib/crash.py	2010-02-03 00:05:22 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2009 Canonical Ltd
+# Copyright (C) 2009, 2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -16,10 +16,32 @@
 
 
 """Handling and reporting crashes.
+
+A crash is an exception propagated up almost to the top level of Bazaar.
+
+If we have apport <https://launchpad.net/apport/>, we store a report of the
+crash using apport into it's /var/crash spool directory, from where the user
+can either manually send it to Launchpad.  In some cases (at least Ubuntu
+development releases), Apport may pop up a window asking if they want
+to send it.
+
+Without apport, we just write a crash report to stderr and the user can report
+this manually if the wish.
+
+We never send crash data across the network without user opt-in.
+
+In principle apport can run on any platform though as of Feb 2010 there seem
+to be some portability bugs.
+
+To force this off in bzr turn set APPORT_DISBLE in the environment or 
+-Dno_apport.
 """
 
 # for interactive testing, try the 'bzr assert-fail' command 
 # or see http://code.launchpad.net/~mbp/bzr/bzr-fail
+#
+# to test with apport it's useful to set
+# export APPORT_IGNORE_OBSOLETE_PACKAGES=1
 
 import os
 import platform
@@ -39,39 +61,42 @@
 
 
 def report_bug(exc_info, stderr):
-    if 'no_apport' not in debug.debug_flags:
-        try:
-            report_bug_to_apport(exc_info, stderr)
+    if ('no_apport' in debug.debug_flags) or \
+        os.environ.get('APPORT_DISABLE', None):
+        return report_bug_legacy(exc_info, stderr)
+    try:
+        if report_bug_to_apport(exc_info, stderr):
+            # wrote a file; if None then report the old way
             return
-        except ImportError, e:
-            trace.mutter("couldn't find apport bug-reporting library: %s" % e)
-            pass
-        except Exception, e:
-            # this should only happen if apport is installed but it didn't
-            # work, eg because of an io error writing the crash file
-            sys.stderr.write("bzr: failed to report crash using apport:\n "
-                "    %r\n" % e)
-            pass
-    report_bug_legacy(exc_info, stderr)
+    except ImportError, e:
+        trace.mutter("couldn't find apport bug-reporting library: %s" % e)
+        pass
+    except Exception, e:
+        # this should only happen if apport is installed but it didn't
+        # work, eg because of an io error writing the crash file
+        stderr.write("bzr: failed to report crash using apport:\n "
+            "    %r\n" % e)
+        pass
+    return report_bug_legacy(exc_info, stderr)
 
 
 def report_bug_legacy(exc_info, err_file):
     """Report a bug by just printing a message to the user."""
-    trace.print_exception(exc_info, err_file)
-    err_file.write('\n')
     err_file.write('bzr %s on python %s (%s)\n' % \
                        (bzrlib.__version__,
                         bzrlib._format_version_tuple(sys.version_info),
                         platform.platform(aliased=1)))
+    err_file.write("plugins:\n")
+    err_file.write(_format_plugin_list())
     err_file.write('arguments: %r\n' % sys.argv)
     err_file.write(
         'encoding: %r, fsenc: %r, lang: %r\n' % (
             osutils.get_user_encoding(), sys.getfilesystemencoding(),
             os.environ.get('LANG')))
-    err_file.write("plugins:\n")
-    err_file.write(_format_plugin_list())
+    err_file.write('\n')
+    trace.print_exception(exc_info, err_file)
     err_file.write(
-        "\n\n"
+        "\n"
         "*** Bazaar has encountered an internal error.  This probably indicates a\n"
         "    bug in Bazaar.  You can help us fix it by filing a bug report at\n"
         "        https://bugs.launchpad.net/bzr/+filebug\n"
@@ -81,47 +106,54 @@
 
 def report_bug_to_apport(exc_info, stderr):
     """Report a bug to apport for optional automatic filing.
+
+    :returns: The name of the crash file, or None if we didn't write one.
     """
-    # this is based on apport_package_hook.py, but omitting some of the
+    # this function is based on apport_package_hook.py, but omitting some of the
     # Ubuntu-specific policy about what to report and when
 
-    # if this fails its caught at a higher level; we don't want to open the
-    # crash file unless apport can be loaded.
+    # if the import fails, the exception will be caught at a higher level and
+    # we'll report the error by other means
     import apport
 
-    crash_file = _open_crash_file()
-    try:
-        _write_apport_report_to_file(exc_info, crash_file)
-    finally:
-        crash_file.close()
-
-    stderr.write("bzr: ERROR: %s.%s: %s\n" 
-        "\n"
-        "*** Bazaar has encountered an internal error.  This probably indicates a\n"
-        "    bug in Bazaar.  You can help us fix it by filing a bug report at\n"
-        "        https://bugs.launchpad.net/bzr/+filebug\n"
-        "    attaching the crash file\n"
-        "        %s\n"
-        "    and including a description of the problem.\n"
-        "\n"
-        "    The crash file is plain text and you can inspect or edit it to remove\n"
-        "    private information.\n"
-        % (exc_info[0].__module__, exc_info[0].__name__, exc_info[1],
-           crash_file.name))
-
-
-def _write_apport_report_to_file(exc_info, crash_file):
+    crash_filename = _write_apport_report_to_file(exc_info)
+
+    if crash_filename is None:
+        stderr.write("\n"
+            "apport is set to ignore crashes in this version of bzr.\n"
+            )
+    else:
+        trace.print_exception(exc_info, stderr)
+        stderr.write("\n"
+            "You can report this problem to Bazaar's developers by running\n"
+            "    apport-bug %s\n"
+            "if a bug-reporting window does not automatically appear.\n"
+            % (crash_filename))
+        # XXX: on Windows, Mac, and other platforms where we might have the
+        # apport libraries but not have an apport always running, we could
+        # synchronously file now
+
+    return crash_filename
+
+
+def _write_apport_report_to_file(exc_info):
     import traceback
     from apport.report import Report
 
     exc_type, exc_object, exc_tb = exc_info
 
     pr = Report()
-    # add_proc_info gives you the memory map of the process: this seems rarely
-    # useful for Bazaar and it does make the report harder to scan, though it
-    # does tell you what binary modules are loaded.
-    # pr.add_proc_info()
+    # add_proc_info gets the executable and interpreter path, which is needed,
+    # plus some less useful stuff like the memory map
+    pr.add_proc_info()
     pr.add_user_info()
+
+    # Package and SourcePackage are needed so that apport will report about even
+    # non-packaged versions of bzr; also this reports on their packaged
+    # dependencies which is useful.
+    pr['SourcePackage'] = 'bzr'
+    pr['Package'] = 'bzr'
+
     pr['CommandLine'] = pprint.pformat(sys.argv)
     pr['BzrVersion'] = bzrlib.__version__
     pr['PythonVersion'] = bzrlib._format_version_tuple(sys.version_info)
@@ -137,18 +169,74 @@
     traceback.print_exception(exc_type, exc_object, exc_tb, file=tb_file)
     pr['Traceback'] = tb_file.getvalue()
 
-    pr.write(crash_file)
+    _attach_log_tail(pr)
+
+    # We want to use the 'bzr' crashdb so that it gets sent directly upstream,
+    # which is a reasonable default for most internal errors.  However, if we
+    # set it here then apport will crash later if it doesn't know about that
+    # crashdb.  Instead, we rely on the bzr package installing both a
+    # source hook telling crashes to go to this crashdb, and a crashdb
+    # configuration describing it.
+
+    # these may contain some sensitive info (smtp_passwords)
+    # TODO: strip that out and attach the rest
+    #
+    #attach_file_if_exists(report,
+    #   os.path.join(dot_bzr, 'bazaar.conf', 'BzrConfig')
+    #attach_file_if_exists(report,
+    #   os.path.join(dot_bzr, 'locations.conf', 'BzrLocations')
+    
+    # strip username, hostname, etc
+    pr.anonymize()
+
+    if pr.check_ignored():
+        # eg configured off in ~/.apport-ignore.xml
+        return None
+    else:
+        crash_file_name, crash_file = _open_crash_file()
+        pr.write(crash_file)
+        crash_file.close()
+        return crash_file_name
+
+
+def _attach_log_tail(pr):
+    try:
+        bzr_log = open(trace._get_bzr_log_filename(), 'rt')
+    except (IOError, OSError), e:
+        pr['BzrLogTail'] = repr(e)
+        return
+    try:
+        lines = bzr_log.readlines()
+        pr['BzrLogTail'] = ''.join(lines[-40:])
+    finally:
+        bzr_log.close()
 
 
 def _open_crash_file():
     crash_dir = config.crash_dir()
-    # user-readable only, just in case the contents are sensitive.
     if not osutils.isdir(crash_dir):
-        os.makedirs(crash_dir, mode=0700)
-    filename = 'bzr-%s-%s.crash' % (
-        osutils.compact_date(time.time()),
-        os.getpid(),)
-    return open(osutils.pathjoin(crash_dir, filename), 'wt')
+        # on unix this should be /var/crash and should already exist; on
+        # Windows or if it's manually configured it might need to be created,
+        # and then it should be private
+        os.makedirs(crash_dir, mode=0600)
+    date_string = time.strftime('%Y-%m-%dT%H:%M', time.gmtime())
+    # XXX: getuid doesn't work on win32, but the crash directory is per-user
+    if sys.platform == 'win32':
+        user_part = ''
+    else:
+        user_part = '.%d' % os.getuid()
+    filename = osutils.pathjoin(
+        crash_dir,
+        'bzr%s.%s.crash' % (
+            user_part,
+            date_string))
+    # be careful here that people can't play tmp-type symlink mischief in the
+    # world-writable directory
+    return filename, os.fdopen(
+        os.open(filename, 
+            os.O_WRONLY|os.O_CREAT|os.O_EXCL,
+            0600),
+        'w')
 
 
 def _format_plugin_list():
 
=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-01-25 17:48:22 +0000
+++ bzrlib/tests/__init__.py	2010-02-03 00:05:22 +0000
@@ -1540,6 +1540,11 @@
             'ftp_proxy': None,
             'FTP_PROXY': None,
             'BZR_REMOTE_PATH': None,
+            # Generally speaking, we don't want apport reporting on crashes in
+            # the test envirnoment unless we're specifically testing apport,
+            # so that it doesn't leak into the real system environment.  We
+            # use an env var so it propagates to subprocesses.
+            'APPORT_DISABLE': '1',
         }
         self.__old_env = {}
         self.addCleanup(self._restoreEnvironment)
 
=== modified file 'bzrlib/tests/test_crash.py'
--- bzrlib/tests/test_crash.py	2009-12-22 15:39:20 +0000
+++ bzrlib/tests/test_crash.py	2010-02-03 00:05:22 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2009 Canonical Ltd
+# Copyright (C) 2009, 2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -18,31 +18,62 @@
 from StringIO import StringIO
 import sys
 
+
+import os
+
+
 from bzrlib import (
+    config,
     crash,
     tests,
-    )
-
-from bzrlib.tests import features
-
-
-class TestApportReporting(tests.TestCase):
+    osutils,
+    )
+
+from bzrlib.tests import (
+    features,
+    TestCaseInTempDir,
+    )
+from bzrlib.tests.features import ApportFeature
+
+
+class TestApportReporting(TestCaseInTempDir):
 
     _test_needs_features = [features.apport]
 
-    def test_apport_report_contents(self):
+    def setUp(self):
+        TestCaseInTempDir.setUp(self)
+        self.requireFeature(ApportFeature)
+
+    def test_apport_report(self):
+        crash_dir = osutils.joinpath((self.test_base_dir, 'crash'))
+        os.mkdir(crash_dir)
+        os.environ['APPORT_CRASH_DIR'] = crash_dir
+        self.assertEquals(crash_dir, config.crash_dir())
+    
+        stderr = StringIO()
+
         try:
             raise AssertionError("my error")
         except AssertionError, e:
             pass
-        outf = StringIO()
-        crash._write_apport_report_to_file(sys.exc_info(), outf)
-        report = outf.getvalue()
-
-        self.assertContainsRe(report, '(?m)^BzrVersion:')
-        # should be in the traceback
+
+        crash_filename = crash.report_bug_to_apport(sys.exc_info(),
+            stderr)
+
+        # message explaining the crash
+        self.assertContainsRe(stderr.getvalue(),
+            "    apport-bug %s" % crash_filename)
+
+        crash_file = open(crash_filename)
+        try:
+            report = crash_file.read()
+        finally:
+            crash_file.close()
+
+        self.assertContainsRe(report,
+            '(?m)^BzrVersion:') # should be in the traceback
         self.assertContainsRe(report, 'my error')
         self.assertContainsRe(report, 'AssertionError')
-        self.assertContainsRe(report, 'test_apport_report_contents')
+        self.assertContainsRe(report, 'test_apport_report')
         # should also be in there
         self.assertContainsRe(report, '(?m)^CommandLine:')
 
=== added directory 'contrib/apport'
=== added file 'contrib/apport/README'
--- contrib/apport/README	1970-01-01 00:00:00 +0000
+++ contrib/apport/README	2010-02-03 00:05:22 +0000
@@ -0,0 +1,13 @@
+Bazaar supports semi-automatic bug reporting through Apport
+<https://launchpad.net/apport/>.
+
+If apport is not installed, an exception is printed to stderr in the usual way.
+
+For this to work properly it's suggested that two files be installed when a
+package of bzr is installed:
+
+``bzr.conf``
+    into ``/etc/apport/crashdb.conf.d``
+
+``source_bzr.py`` 
+    into ``/usr/share/apport/package-hooks``
 
=== added file 'contrib/apport/bzr.conf'
--- contrib/apport/bzr.conf	1970-01-01 00:00:00 +0000
+++ contrib/apport/bzr.conf	2010-02-03 00:05:22 +0000
@@ -0,0 +1,6 @@
+bzr = {
+	# most bzr bugs are upstream bugs; file them there
+	'impl': 'launchpad',
+	'project': 'bzr',
+	'bug_pattern_base': 'http://people.canonical.com/~pitti/bugpatterns',
+}
 
=== added file 'contrib/apport/source_bzr.py'
--- contrib/apport/source_bzr.py	1970-01-01 00:00:00 +0000
+++ contrib/apport/source_bzr.py	2010-02-03 00:05:22 +0000
@@ -0,0 +1,54 @@
+'''apport package hook for Bazaar'''
+
+# Copyright (c) 2009, 2010 Canonical Ltd.
+# Author: Matt Zimmerman <[e-mail]>
+#         and others
+
+from apport.hookutils import *
+import os
+
+bzr_log = os.path.expanduser('~/.bzr.log')
+dot_bzr = os.path.expanduser('~/.bazaar')
+
+def _add_log_tail(report):
+    # may have already been added in-process
+    if 'BzrLogTail' in report:
+        return
+
+    bzr_log_lines = open(bzr_log).readlines()
+    bzr_log_lines.reverse()
+
+    bzr_log_tail = []
+    blanks = 0
+    for line in bzr_log_lines:
+        if line == '\n':
+            blanks += 1
+        bzr_log_tail.append(line)
+        if blanks >= 2: 
+            break
+
+    bzr_log_tail.reverse()
+    report['BzrLogTail'] = ''.join(bzr_log_tail)
+
+
+def add_info(report):
+
+    _add_log_tail()
+    if 'BzrPlugins' not in report:
+        # may already be present in-process
+        report['BzrPlugins'] = command_output(['bzr', 'plugins', '-v'])
+        
+    # by default assume bzr crashes are upstream bugs; this relies on
+    # having a bzr entry under /etc/apport/crashdb.conf.d/
+    report['CrashDB'] = 'bzr'
+
+    # these may contain some sensitive info (smtp_passwords)
+    # TODO: strip that out and attach the rest
+
+    #attach_file_if_exists(report,
+    #	os.path.join(dot_bzr, 'bazaar.conf', 'BzrConfig')
+    #attach_file_if_exists(report,
+    #	os.path.join(dot_bzr, 'locations.conf', 'BzrLocations')
+
+        
+# vim: expandtab shiftwidth=4