Merge lp:~mbp/bzr/704195-plugin-warnings into lp:bzr

[Martin Pool]

Don't show messages about plugins incompatible with bzr

This stops bzr spamming you about out-of-date plugins. Telling people on every single invocation is not helpful. Now we just tell you on in `bzr plugins`.

As a follow-on in bug 704238 we can try to load them anyhow.

[Martin Pool (mbp)]

I previously targeted 2.3 but this is more useful in 2.4. The NEWS probably needs to be fixed before landing.

[Jelmer Vernooij (jelmer)]

review: Approve

[Martin Pool (mbp)]

I'll update a couple of things before landing based on conversations
with Jelmer:

 * the code that formats the list could fold both the loaded and
failed-to-load plugins together, and be a bit simpler
 * should move into the 2.4 NEWS

Note that with this change, missing-dependency plugins will continue
to not load; we just represent that differently.

--
Martin

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2011-01-19 18:40:15 +0000
+++ bzrlib/builtins.py	2011-01-20 23:09:55 +0000
@@ -4587,26 +4587,9 @@
 
     @display_command
     def run(self, verbose=False):
-        import bzrlib.plugin
-        from inspect import getdoc
-        result = []
-        for name, plugin in bzrlib.plugin.plugins().items():
-            version = plugin.__version__
-            if version == 'unknown':
-                version = ''
-            name_ver = '%s %s' % (name, version)
-            d = getdoc(plugin.module)
-            if d:
-                doc = d.split('\n')[0]
-            else:
-                doc = '(no description)'
-            result.append((name_ver, doc, plugin.path()))
-        for name_ver, doc, path in sorted(result):
-            self.outf.write("%s\n" % name_ver)
-            self.outf.write("   %s\n" % doc)
-            if verbose:
-                self.outf.write("   %s\n" % path)
-            self.outf.write("\n")
+        from bzrlib import plugin
+        self.outf.writelines(
+            plugin.describe_plugins(show_paths=verbose))
 
 
 class cmd_testament(Command):
 
=== modified file 'bzrlib/crash.py'
--- bzrlib/crash.py	2011-01-18 00:25:04 +0000
+++ bzrlib/crash.py	2011-01-20 23:09:55 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2009, 2010 Canonical Ltd
+# Copyright (C) 2009, 2010, 2011 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
@@ -255,11 +255,7 @@
 
 
 def _format_plugin_list():
-    plugin_lines = []
-    for name, a_plugin in sorted(plugin.plugins().items()):
-        plugin_lines.append("  %-20s %s [%s]" %
-            (name, a_plugin.path(), a_plugin.__version__))
-    return '\n'.join(plugin_lines)
+    return ''.join(plugin.describe_plugins(show_paths=True))
 
 
 def _format_module_list():
 
=== modified file 'bzrlib/plugin.py'
--- bzrlib/plugin.py	2010-06-11 08:02:42 +0000
+++ bzrlib/plugin.py	2011-01-20 23:09:55 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2010 Canonical Ltd
+# Copyright (C) 2005-2011 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
@@ -63,6 +63,11 @@
 _plugins_disabled = False
 
 
+plugin_warnings = {}
+# Map from plugin name, to list of string warnings about eg plugin
+# dependencies.
+
+
 def are_plugins_disabled():
     return _plugins_disabled
 
@@ -77,6 +82,43 @@
     load_plugins([])
 
 
+def describe_plugins(show_paths=False):
+    """Generate text description of plugins.
+
+    Includes both those that have loaded, and those that failed to 
+    load.
+
+    :param show_paths: If true,
+    :returns: Iterator of text lines (including newlines.)
+    """
+    from inspect import getdoc
+    loaded_plugins = plugins()
+    all_names = sorted(list(set(
+        loaded_plugins.keys() + plugin_warnings.keys())))
+    for name in all_names:
+        if name in loaded_plugins:
+            plugin = loaded_plugins[name]
+            version = plugin.__version__
+            if version == 'unknown':
+                version = ''
+            yield '%s %s\n' % (name, version)
+            d = getdoc(plugin.module)
+            if d:
+                doc = d.split('\n')[0]
+            else:
+                doc = '(no description)'
+            yield ("  %s\n" % doc)
+            if show_paths:
+                yield ("   %s\n" % plugin.path())
+            del plugin
+        else:
+            yield "%s (failed to load)\n" % name
+        if name in plugin_warnings:
+            for line in plugin_warnings[name]:
+                yield "  ** " + line + '\n'
+        yield '\n'
+
+
 def _strip_trailing_sep(path):
     return path.rstrip("\\/")
 
@@ -327,6 +369,11 @@
     return None, None, (None, None, None)
 
 
+def record_plugin_warning(plugin_name, warning_message):
+    trace.mutter(warning_message)
+    plugin_warnings.setdefault(plugin_name, []).append(warning_message)
+
+
 def _load_plugin_module(name, dir):
     """Load plugin name from dir.
 
@@ -340,10 +387,12 @@
     except KeyboardInterrupt:
         raise
     except errors.IncompatibleAPI, e:
-        trace.warning("Unable to load plugin %r. It requested API version "
+        warning_message = (
+            "Unable to load plugin %r. It requested API version "
             "%s of module %s but the minimum exported version is %s, and "
             "the maximum is %s" %
             (name, e.wanted, e.api, e.minimum, e.current))
+        record_plugin_warning(name, warning_message)
     except Exception, e:
         trace.warning("%s" % e)
         if re.search('\.|-| ', name):
@@ -354,7 +403,9 @@
                     "file path isn't a valid module name; try renaming "
                     "it to %r." % (name, dir, sanitised_name))
         else:
-            trace.warning('Unable to load plugin %r from %r' % (name, dir))
+            record_plugin_warning(
+                name,
+                'Unable to load plugin %r from %r' % (name, dir))
         trace.log_exception_quietly()
         if 'error' in debug.debug_flags:
             trace.print_exception(sys.exc_info(), sys.stderr)
 
=== modified file 'bzrlib/tests/test_crash.py'
--- bzrlib/tests/test_crash.py	2011-01-12 01:01:53 +0000
+++ bzrlib/tests/test_crash.py	2011-01-20 23:09:55 +0000
@@ -26,6 +26,7 @@
     config,
     crash,
     osutils,
+    plugin,
     tests,
     )
 
@@ -42,6 +43,11 @@
         self.overrideEnv('APPORT_CRASH_DIR', crash_dir)
         self.assertEquals(crash_dir, config.crash_dir())
 
+        self.overrideAttr(
+            plugin,
+            'plugin_warnings',
+            {'example': ['Failed to load plugin foo']})
+
         stderr = StringIO()
 
         try:
@@ -71,3 +77,6 @@
         self.assertContainsRe(report, 'test_apport_report')
         # should also be in there
         self.assertContainsRe(report, '(?m)^CommandLine:')
+        self.assertContainsRe(
+            report,
+            'Failed to load plugin foo')
 
=== modified file 'bzrlib/tests/test_plugins.py'
--- bzrlib/tests/test_plugins.py	2011-01-10 22:20:12 +0000
+++ bzrlib/tests/test_plugins.py	2011-01-20 23:09:55 +0000
@@ -38,7 +38,7 @@
 
 # TODO: Write a test for plugin decoration of commands.
 
-class TestPluginMixin(object):
+class BaseTestPlugins(tests.TestCaseInTempDir):
 
     def create_plugin(self, name, source=None, dir='.', file_name=None):
         if source is None:
@@ -99,7 +99,7 @@
         self.failUnless('bzrlib.plugins.%s' % name in sys.modules)
 
 
-class TestLoadingPlugins(tests.TestCaseInTempDir, TestPluginMixin):
+class TestLoadingPlugins(BaseTestPlugins):
 
     activeattributes = {}
 
@@ -267,8 +267,13 @@
             stream.close()
 
     def test_plugin_with_bad_api_version_reports(self):
-        # This plugin asks for bzrlib api version 1.0.0, which is not supported
-        # anymore.
+        """Try loading a plugin that requests an unsupported api.
+        
+        Observe that it records the problem but doesn't complain on stderr.
+
+        See https://bugs.launchpad.net/bzr/+bug/704195
+        """
+        self.overrideAttr(plugin, 'plugin_warnings', {})
         name = 'wants100.py'
         f = file(name, 'w')
         try:
@@ -276,9 +281,14 @@
                 "bzrlib.api.require_any_api(bzrlib, [(1, 0, 0)])\n")
         finally:
             f.close()
-
         log = self.load_and_capture(name)
-        self.assertContainsRe(log,
+        self.assertNotContainsRe(log,
+            r"It requested API version")
+        self.assertEquals(
+            ['wants100'],
+            plugin.plugin_warnings.keys())
+        self.assertContainsRe(
+            plugin.plugin_warnings['wants100'][0],
             r"It requested API version")
 
     def test_plugin_with_bad_name_does_not_load(self):
@@ -292,7 +302,7 @@
             "it to 'bad_plugin_name_'\.")
 
 
-class TestPlugins(tests.TestCaseInTempDir, TestPluginMixin):
+class TestPlugins(BaseTestPlugins):
 
     def setup_plugin(self, source=""):
         # This test tests a new plugin appears in bzrlib.plugin.plugins().
@@ -768,7 +778,7 @@
                         ['+foo', '-bar'])
 
 
-class TestDisablePlugin(tests.TestCaseInTempDir, TestPluginMixin):
+class TestDisablePlugin(BaseTestPlugins):
 
     def setUp(self):
         super(TestDisablePlugin, self).setUp()
@@ -809,6 +819,7 @@
         self.assertLength(0, self.warnings)
 
 
+
 class TestLoadPluginAtSyntax(tests.TestCase):
 
     def _get_paths(self, paths):
@@ -832,7 +843,7 @@
                           os.pathsep.join(['batman@cave', '', 'robin@mobile']))
 
 
-class TestLoadPluginAt(tests.TestCaseInTempDir, TestPluginMixin):
+class TestLoadPluginAt(BaseTestPlugins):
 
     def setUp(self):
         super(TestLoadPluginAt, self).setUp()
@@ -847,6 +858,9 @@
         self.create_plugin_package('test_foo', dir='standard/test_foo')
         # All the tests will load the 'test_foo' plugin from various locations
         self.addCleanup(self._unregister_plugin, 'test_foo')
+        # Unfortunately there's global cached state for the specific
+        # registered paths.
+        self.addCleanup(plugin.PluginImporter.reset)
 
     def assertTestFooLoadedFrom(self, path):
         self.assertPluginKnown('test_foo')
@@ -945,3 +959,26 @@
         self.overrideEnv('BZR_PLUGINS_AT', 'test_foo@%s' % plugin_path)
         plugin.load_plugins(['standard'])
         self.assertTestFooLoadedFrom(plugin_path)
+
+
+class TestDescribePlugins(BaseTestPlugins):
+
+    def test_describe_plugins(self):
+        class DummyModule(object):
+            __doc__ = 'Hi there'
+        class DummyPlugin(object):
+            __version__ = '0.1.0'
+            module = DummyModule()
+        def dummy_plugins():
+            return { 'good': DummyPlugin() }
+        self.overrideAttr(plugin, 'plugin_warnings',
+            {'bad': ['Failed to load (just testing)']})
+        self.overrideAttr(plugin, 'plugins', dummy_plugins)
+        self.assertEquals("""\
+bad (failed to load)
+  ** Failed to load (just testing)
+
+good 0.1.0
+  Hi there
+
+""", ''.join(plugin.describe_plugins()))
 
=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-01-19 18:18:48 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-01-20 23:09:55 +0000
@@ -54,6 +54,11 @@
 * ``bzr whoami`` will now display an error if both a new identity and ``--email``
   were specified. (Jelmer Vernooij, #680449)
 
+* Plugins incompatible with the current version of bzr no longer produce a
+  warning on every command invocation.  Instead, a message is shown by
+  ``bzr plugins`` and in crash reports.
+  (#704195, Martin Pool)
+
 Documentation
 *************