Merge lp:~mbp/bzr/error-reporting into lp:bzr

[Martin Pool]

Per bug 716389 and previous feedback, this now shows just a concisely list of plugins in the non-apport crash message

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/28/2011 03:16 AM, Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/error-reporting into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #716389 in Bazaar: "In-terminal crash report is far too long, containing full `bzr plugins` output"
> https://bugs.launchpad.net/bzr/+bug/716389
>
> For more details, see:
> https://code.launchpad.net/~mbp/bzr/error-reporting/+merge/55029
>
> This stops bzr printing a full list of plugins (too long!) on a non-apport error; per bug 716389
>

What if we printed out the short forms, rather than the long form. Right
now we show:
news_merge 2.4.0dev2
  Merge hook for bzr's NEWS file.
   /home/jameinel/dev/bzr/bzr.dev/bzrlib/plugins/news_merge

qbzr 0.20.0
  QBzr - Qt-based frontend for Bazaar
   /usr/lib/python2.6/dist-packages/bzrlib/plugins/qbzr

weave_fmt
  Weave formats.
   /home/jameinel/dev/bzr/bzr.dev/bzrlib/plugins/weave_fmt

And I agree that it is overly verbose. We could, instead do:
bash_completion 2.4.0dev2
bzrtools 2.3.1
changelog_merge 2.4.0dev2
launchpad 2.4.0dev2
loggerhead 1.18.0
netrc_credential_store 2.4.0dev2
news_merge 2.4.0dev2
qbzr 0.20.0
weave_fmt

I actually do find it useful to see the plugin versions as part of bug
reports that people manually copy & paste. I don't think we want to get
rid of it completely. I wonder if we could even put them all-on-one-line
like:

bash_completion 2.4.0dev2, bzrtools 2.3.1, changelog_merge 2.4.0dev2,
launchpad 2.4.0dev2, loggerhead 1.18.0, netrc_credential_store
2.4.0dev2, news_merge 2.4.0dev2, qbzr 0.20.0, weave_fmt

That would still allow us to say "the bug is in bzr-svn, and you're
running an older version".

Just a thought. I don't think I want to completely get rid of the plugin
list. I do agree that it obscures seeing the backtrace the way it is now.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2QcWYACgkQJdeBCYSNAAOr5ACfV4PuAXPTIomnKxTE6Jc3MAd8
oMgAoMWBSWUkh7fX0160a5EQg1eXgjxE
=4fJS
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

I think one per line would still be too much: easily enough to make the
whole report more than a 25 line screen. But I can do one that puts them on
one line. It's true many bugs are related to plugins.
On 28/03/2011 10:34 PM, "John A Meinel" <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 03/28/2011 03:16 AM, Martin Pool wrote:
>> Martin Pool has proposed merging lp:~mbp/bzr/error-reporting into lp:bzr.
>>
>> Requested reviews:
>> bzr-core (bzr-core)
>> Related bugs:
>> Bug #716389 in Bazaar: "In-terminal crash report is far too long,
containing full `bzr plugins` output"
>> https://bugs.launchpad.net/bzr/+bug/716389
>>
>> For more details, see:
>> https://code.launchpad.net/~mbp/bzr/error-reporting/+merge/55029
>>
>> This stops bzr printing a full list of plugins (too long!) on a
non-apport error; per bug 716389
>>
>
> What if we printed out the short forms, rather than the long form. Right
> now we show:
> news_merge 2.4.0dev2
> Merge hook for bzr's NEWS file.
> /home/jameinel/dev/bzr/bzr.dev/bzrlib/plugins/news_merge
>
> qbzr 0.20.0
> QBzr - Qt-based frontend for Bazaar
> /usr/lib/python2.6/dist-packages/bzrlib/plugins/qbzr
>
> weave_fmt
> Weave formats.
> /home/jameinel/dev/bzr/bzr.dev/bzrlib/plugins/weave_fmt
>
>
> And I agree that it is overly verbose. We could, instead do:
> bash_completion 2.4.0dev2
> bzrtools 2.3.1
> changelog_merge 2.4.0dev2
> launchpad 2.4.0dev2
> loggerhead 1.18.0
> netrc_credential_store 2.4.0dev2
> news_merge 2.4.0dev2
> qbzr 0.20.0
> weave_fmt
>
> I actually do find it useful to see the plugin versions as part of bug
> reports that people manually copy & paste. I don't think we want to get
> rid of it completely. I wonder if we could even put them all-on-one-line
> like:
>
> bash_completion 2.4.0dev2, bzrtools 2.3.1, changelog_merge 2.4.0dev2,
> launchpad 2.4.0dev2, loggerhead 1.18.0, netrc_credential_store
> 2.4.0dev2, news_merge 2.4.0dev2, qbzr 0.20.0, weave_fmt
>
> That would still allow us to say "the bug is in bzr-svn, and you're
> running an older version".
>
> Just a thought. I don't think I want to completely get rid of the plugin
> list. I do agree that it obscures seeing the backtrace the way it is now.
>
> John
> =:->
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAk2QcWYACgkQJdeBCYSNAAOr5ACfV4PuAXPTIomnKxTE6Jc3MAd8
> oMgAoMWBSWUkh7fX0160a5EQg1eXgjxE
> =4fJS
> -----END PGP SIGNATURE-----
>
> --
> https://code.launchpad.net/~mbp/bzr/error-reporting/+merge/55029
> You are the owner of lp:~mbp/bzr/error-reporting.

[Jelmer Vernooij (jelmer)]

> bash_completion 2.4.0dev2, bzrtools 2.3.1, changelog_merge 2.4.0dev2,
> launchpad 2.4.0dev2, loggerhead 1.18.0, netrc_credential_store
> 2.4.0dev2, news_merge 2.4.0dev2, qbzr 0.20.0, weave_fmt
>
> That would still allow us to say "the bug is in bzr-svn, and you're
> running an older version".
>
> Just a thought. I don't think I want to completely get rid of the plugin
> list. I do agree that it obscures seeing the backtrace the way it is now.
I think putting them on one line would be a good idea. Usually the version of the plugin is the main thing that matters anyway. The summary lines aren't particularly useful when reading bug reports, and the location is rarely useful to see whether the plugin was loaded from system, site or user dir.

Cheers,

Jelmer

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 3/31/2011 6:49 AM, Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/error-reporting into lp:bzr with lp:~mbp/bzr/assert-doctest as a prerequisite.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #716389 in Bazaar: "In-terminal crash report is far too long, containing full `bzr plugins` output"
> https://bugs.launchpad.net/bzr/+bug/716389
>
> For more details, see:
> https://code.launchpad.net/~mbp/bzr/error-reporting/+merge/55677
>
> Per bug 716389 and previous feedback, this now shows just a concisely list of plugins in the non-apport crash message
>

Right now plugin.describe_plugins() uses:
  plugin = loaded_plugins[name]
  version = plugin.__version__
  if version == 'unknown':
      version = ''

We might want to do that for the formatting, so you don't get:
bzrtools[unknown], qbzr[1.2.3]

I don't have strong feelings on it, though. It might be good to report
unknown...

This is what the formatting looks like here (I have a fair number of
'unknown' version plugins):

bzr 2.4.0dev2 on python 2.6.4 (Windows-Vista-6.0.6002-SP2)
arguments: ['C:/Users/jameinel/dev/bzr/jam-integration/bzr', 'assert-fail']
plugins: bash_completion[2.4.0dev2], bzrtools[2.1b1],
    changelog_merge[2.4.0dev2], check_chk[unknown], colo[0.2.1dev],
    dummy_request[unknown], dump[unknown], email[unknown],
explorer[1.1.0dev],
    fastimport[0.9.0dev], file_locking[0.1.0dev], gimmepdb[unknown],
    grep[0.2.0], in_ancestry[unknown], launchpad[2.4.0dev2],
    lock_test[0.1.0dev], loggerhead[1.18.0], loom[2.2.1dev],
    netrc_credential_store[2.4.0dev2], news_merge[2.4.0dev2],
    per_file_graph[unknown], pqm[1.4.0dev], pybloom[unknown],
    qbzr[0.20.0dev1], rebase[0.4.3dev], register[unknown],
    repodetails[1.9.0dev], search[1.7.0dev], start[unknown],
stats[0.1.0dev],
    test_groupcompress[unknown], time_iter_changes[unknown],
    track_wow_renames[0.0.1dev], update_copyright[0.1.0dev],
vimdiff[unknown],
    weave_fmt[unknown]
encoding: 'cp1252', fsenc: 'mbcs', lang: 'C.UTF-8'

...

=== modified file 'bzrlib/tests/__init__.py'
- --- bzrlib/tests/__init__.py 2011-03-31 04:48:22 +0000
+++ bzrlib/tests/__init__.py 2011-03-31 04:48:30 +0000
@@ -1465,7 +1465,7 @@
         checker = doctest.OutputChecker()
         if optionflags is None:
             # XXX: More here by default?
- - optionflags = doctest.ELLIPSIS
+ optionflags = doctest.ELLIPSIS | doctest.REPORT_UDIFF
         if not checker.check_output(expected, actual, optionflags):
             # The Doctest api insists on an internal object here but
doesn't
             # really need it; just pretend.

^- Looks like this was meant to be part of the earlier patch.

test_crash does pass here on a Windows machine. So I'm happy that the
doctest matcher worked for what you wanted.

I do think you could have crafted some sort of multiline regex to get
the same results as the doctest matcher. I suppose escaping '.' is a bit
ugly (no worse than <BLANKLINE>, IMO, and '.*' is shorter than '...')

You do get much better error messages, though. Since regexes tend to say
"no, didn't match" without any conte...

review: Approve

=== modified file 'bzrlib/crash.py'
--- bzrlib/crash.py	2011-01-20 23:07:25 +0000
+++ bzrlib/crash.py	2011-03-31 04:48:30 +0000
@@ -1,4 +1,8 @@
+<<<<<<< TREE
 # Copyright (C) 2009, 2010, 2011 Canonical Ltd
+=======
+# Copyright (C) 2009-2011 Canonical Ltd
+>>>>>>> MERGE-SOURCE
 #
 # 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
@@ -84,19 +88,27 @@
     """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('arguments: %r\n' % sys.argv)
-    err_file.write(
+    import textwrap
+    def print_wrapped(l):
+        err_file.write(textwrap.fill(l,
+            width=78, subsequent_indent='    ') + '\n')
+    print_wrapped('bzr %s on python %s (%s)\n' % \
+        (bzrlib.__version__,
+        bzrlib._format_version_tuple(sys.version_info),
+        platform.platform(aliased=1)))
+    print_wrapped('arguments: %r\n' % sys.argv)
+    print_wrapped(textwrap.fill(
+        'plugins: ' + plugin.format_concise_plugin_list(),
+        width=78,
+        subsequent_indent='    ',
+        ) + '\n')
+    print_wrapped(
         '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())
+    # We used to show all the plugins here, but it's too verbose.
     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"

=== modified file 'bzrlib/plugin.py'
--- bzrlib/plugin.py	2011-01-20 01:02:34 +0000
+++ bzrlib/plugin.py	2011-03-31 04:48:30 +0000
@@ -451,6 +451,17 @@
     return result
 
 
+def format_concise_plugin_list():
+    """Return a string holding a concise list of plugins and their version.
+    """
+    items = []
+    for name, a_plugin in sorted(plugins().items()):
+        items.append("%s[%s]" %
+            (name, a_plugin.__version__))
+    return ', '.join(items)
+
+
+
 class PluginsHelpIndex(object):
     """A help index that returns help topics for plugins."""
 

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2011-03-31 04:48:22 +0000
+++ bzrlib/tests/__init__.py	2011-03-31 04:48:30 +0000
@@ -1465,7 +1465,7 @@
         checker = doctest.OutputChecker()
         if optionflags is None:
             # XXX: More here by default?
-            optionflags = doctest.ELLIPSIS
+            optionflags = doctest.ELLIPSIS | doctest.REPORT_UDIFF
         if not checker.check_output(expected, actual, optionflags):
             # The Doctest api insists on an internal object here but doesn't
             # really need it; just pretend.

=== modified file 'bzrlib/tests/test_crash.py'
--- bzrlib/tests/test_crash.py	2011-01-18 00:41:29 +0000
+++ bzrlib/tests/test_crash.py	2011-03-31 04:48:30 +0000
@@ -15,13 +15,12 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
 
+import doctest
+import os
 from StringIO import StringIO
 import sys
 
 
-import os
-
-
 from bzrlib import (
     config,
     crash,
@@ -77,6 +76,51 @@
         self.assertContainsRe(report, 'test_apport_report')
         # should also be in there
         self.assertContainsRe(report, '(?m)^CommandLine:')
+<<<<<<< TREE
         self.assertContainsRe(
             report,
             'Failed to load plugin foo')
+=======
+
+
+class TestNonApportReporting(tests.TestCase):
+    """Reporting of crash-type bugs without apport.
+    
+    This should work in all environments.
+    """
+
+    def setup_fake_plugins(self):
+        def fake_plugins():
+            fake = plugin.PlugIn('fake_plugin', plugin)
+            fake.version_info = lambda: (1, 2, 3)
+            return {"fake_plugin": fake}
+        self.overrideAttr(plugin, 'plugins', fake_plugins)
+
+    def test_report_bug_legacy(self):
+        self.setup_fake_plugins()
+        err_file = StringIO()
+        try:
+            raise AssertionError("my error")
+        except AssertionError, e:
+            pass
+        crash.report_bug_legacy(sys.exc_info(), err_file)
+        self.assertDoctestExampleMatches("""\
+bzr: ERROR: exceptions.AssertionError: my error
+<BLANKLINE>
+Traceback (most recent call last):
+  ...
+AssertionError: my error
+<BLANKLINE>
+bzr ... on python ...
+arguments: ...
+plugins: fake_plugin[1.2.3]
+encoding: ...
+<BLANKLINE>
+*** Bazaar has encountered an internal error.  This probably indicates a
+    bug in Bazaar.  You can help us fix it by filing a bug report at
+        https://bugs.launchpad.net/bzr/+filebug
+    including this traceback and a description of the problem.
+""", 
+    err_file.getvalue(),
+    )
+>>>>>>> MERGE-SOURCE

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2011-03-31 04:48:22 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2011-03-31 04:48:30 +0000
@@ -5,6 +5,7 @@
 .. toctree::
    :maxdepth: 1
 
+<<<<<<< TREE
 bzr 2.3.2
 #########
 
@@ -80,6 +81,15 @@
   in python > 2.7.1).  (Vincent Ladeuil, #654733)
 
 
+=======
+Bug Fixes
+*********
+
+ * When reporting a crash without apport, don't print the full list of
+   plugins because it's often too long.
+   (Martin Pool, #716389)
+
+>>>>>>> MERGE-SOURCE
 bzr 2.3.1
 #########