Merge lp:~vila/bzr/411413-disable-plugin into lp:bzr

[Vincent Ladeuil]

This fix bug #411413 by allowing the '-<plugin_name>' syntax to be used in BZR_PLUGIN_PATH.

Note that it works for:
- the automatic loading triggered by bzrlib.plugin.load_plugins([path])
- direct imports from other plugins, import bzrlib.plugins.myplugin
  will now raise an import error

[Matthew Fuller (fullermd)]

> [...] allowing the '-<plugin_name>' syntax to be used in
> BZR_PLUGIN_PATH.

That seems... unclean.

[Vincent Ladeuil (vila)]

> > [...] allowing the '-<plugin_name>' syntax to be used in
> > BZR_PLUGIN_PATH.
>
> That seems... unclean.

More precisely ? Any better idea ?

We already use +user/-site/+core to give access to the "standard" parts of the paths.
Disabling a plugin is not something you do on a regular basis (or you'd better
just uninstall it).

[Martin Pool (mbp)]

On 17 March 2010 03:09, Vincent Ladeuil <email address hidden> wrote:
>> > [...] allowing the '-<plugin_name>' syntax to be used in
>> > BZR_PLUGIN_PATH.
>>
>> That seems...   unclean.

Matthew's right, that's just pretty weird to allow a mix of plugin
names and paths.

>
> More precisely ? Any better idea ?
>
> We already use +user/-site/+core to give access to the "standard" parts of the paths.
> Disabling a plugin is not something you do on a regular basis (or you'd better
> just uninstall it).

BZR_DISABLE_PLUGINS=svn:git:qbzr

--
Martin <http://launchpad.net/~mbp/>

[Martin Pool (mbp)]

review: Needs Fixing

[Matthew Fuller (fullermd)]

> More precisely ? Any better idea ?

I'd have a separate env var for it.

> We already use +user/-site/+core to give access to the "standard"
> parts of the paths.

Well, but it doesn't give paths to plugins. It gives paths to dirs
CONTAINING plugins. To then mix dirs with plugins, with names of
plugins, is icky.

[Vincent Ladeuil (vila)]

> BZR_DISABLE_PLUGINS=svn:git:qbzr

Bah, of course, fixed.

[Martin Pool (mbp)]

23 +BZR_DISABLE_PLUGINS
24 +~~~~~~~~~~~~~~~~~~~
25 +
26 +Under special circumstances, it's better to disable a plugin (or
27 +several) rather than uninstalling them completely. Such plugins
28 +can be specified in the ``BZR_DISABLE_PLUGINS`` environment
29 +variable.
30 +
31 +In that case, ``bzr`` will stop loading the specified plugins and
32 +will raise an import error if you try to import them explicitly.
33 +
34 +Example:
35 +~~~~~~~~
36 +

The ReST syntax here looks strange, because "Example" will be another heading at the same level as BZR_DISABLE_PLUGINS. Just leave it out and put a :: after the word "explicitly".

Users may not understand what "if you try to import them explicitly" means - indeed it doesn't mean a lot at the external user level. So you could say they will error if for example another plugin depends upon them?

202 + self.warnings = []
203 + def captured_warning(*args, **kwargs):
204 + self.warnings.append((args, kwargs))
205 + self.overrideAttr(trace, 'warning', captured_warning)

It seems like there should already be a facility for this, but maybe not. Maybe you could lift it to a base class to save time?

Otherwise very good, thanks for fixing this!

bb:tweak

review: Needs Fixing

[Vincent Ladeuil (vila)]

> The ReST syntax here looks strange, because "Example" will be another heading
> at the same level as BZR_DISABLE_PLUGINS. Just leave it out and put a ::
> after the word "explicitly".
>

Ok, after some wtf instants and some help from igc, I've fixed that and checked
the results in the sphinx generated html.

> Users may not understand what "if you try to import them explicitly" means -
> indeed it doesn't mean a lot at the external user level. So you could say
> they will error if for example another plugin depends upon them?

Added.

>
> 202 + self.warnings = []
> 203 + def captured_warning(*args, **kwargs):
> 204 + self.warnings.append((args, kwargs))
> 205 + self.overrideAttr(trace, 'warning', captured_warning)
>
> It seems like there should already be a facility for this, but maybe not.
> Maybe you could lift it to a base class to save time?

Well, I struggled a bit with _capture_deprecation_warnings (for symbol_versioning.warn) and callCatchWarnings (for warnings.showwarning) before realizing I wanted *trace.warning*.

So I agree we should provide an easier way :)
May be if all warnings, note, mutter was controlled by UIFactory and friends things will be clearer ?

Since that's the first time I know of that we need to capture trace.warning, I'd rather punt on this one.

I could make a further submission if you prefer, but *I*'d prefer some unification
of various warnings first (if only by providing more explicit names).

=== modified file 'NEWS'
--- NEWS	2010-03-12 19:59:50 +0000
+++ NEWS	2010-03-17 08:15:36 +0000
@@ -51,6 +51,10 @@
   treats backslash as an escape character on Windows.   (Gordon Tyler,
   #392248)
 
+* Plugins can be disabled by defining ``BZR_DISABLE_PLUGINS`` as
+  a list of plugin names separated by ':' (';' on windows).
+  (Vincent Ladeuil, #411413)
+
 * Tree-shape conflicts can be resolved by providing ``--take-this`` and
   ``--take-other`` to the ``bzr resolve`` command. Just marking the conflict
   as resolved is still accessible via the ``--done`` default action.
 
=== modified file 'bzrlib/help_topics/en/configuration.txt'
--- bzrlib/help_topics/en/configuration.txt	2010-01-03 03:33:10 +0000
+++ bzrlib/help_topics/en/configuration.txt	2010-03-17 08:15:36 +0000
@@ -116,6 +116,22 @@
 Overriding the default site plugin directory:
 ``BZR_PLUGIN_PATH='/path/to/my/site/plugins:-site':+user``
 
+BZR_DISABLE_PLUGINS
+~~~~~~~~~~~~~~~~~~~
+
+Under special circumstances, it's better to disable a plugin (or
+several) rather than uninstalling them completely. Such plugins
+can be specified in the ``BZR_DISABLE_PLUGINS`` environment
+variable.
+
+In that case, ``bzr`` will stop loading the specified plugins and
+will raise an import error if you try to import them explicitly.
+
+Example:
+~~~~~~~~
+
+Disabling ``myplugin`` and ``yourplugin``:
+``BZR_DISABLE_PLUGINS='myplugin:yourplugin'``
 
 
 BZRPATH
 
=== modified file 'bzrlib/plugin.py'
--- bzrlib/plugin.py	2010-03-12 13:41:08 +0000
+++ bzrlib/plugin.py	2010-03-17 08:15:36 +0000
@@ -91,6 +91,12 @@
     if path is None:
         path = get_standard_plugins_path()
     _mod_plugins.__path__ = path
+    # Set up a blacklist for disabled plugins if any
+    PluginBlackListImporter.blacklist = {}
+    disabled_plugins = os.environ.get('BZR_DISABLE_PLUGINS', None)
+    if disabled_plugins is not None:
+        for name in disabled_plugins.split(os.pathsep):
+            PluginBlackListImporter.blacklist['bzrlib.plugins.' + name] = True
     return path
 
 
@@ -183,8 +189,8 @@
             try:
                 p = refs[p[1:]]
             except KeyError:
-                # Leave them untouched otherwise, user may have paths starting
-                # with '+'...
+                # Leave them untouched so user can still use paths starting
+                # with '+'
                 pass
         _append_new_path(paths, p)
 
@@ -202,7 +208,7 @@
     files (and whatever other extensions are used in the platform,
     such as *.pyd).
 
-    load_from_dirs() provides the underlying mechanism and is called with
+    load_from_path() provides the underlying mechanism and is called with
     the default directory list to provide the normal behaviour.
 
     :param path: The list of paths to search for plugins.  By default,
@@ -290,6 +296,8 @@
             plugin_names.add(f)
 
     for name in plugin_names:
+        if ('bzrlib.plugins.%s' % name) in PluginBlackListImporter.blacklist:
+            continue
         try:
             exec "import bzrlib.plugins.%s" % name in {}
         except KeyboardInterrupt:
@@ -476,3 +484,19 @@
         return version_string
 
     __version__ = property(_get__version__)
+
+
+class _PluginBlackListImporter(object):
+
+    def __init__(self):
+        self.blacklist = {}
+
+    def find_module(self, fullname, parent_path=None):
+        if fullname in self.blacklist:
+            raise ImportError('%s is disabled' % fullname)
+        return None
+
+PluginBlackListImporter = _PluginBlackListImporter()
+sys.meta_path.append(PluginBlackListImporter)
+
+
 
=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-03-10 06:02:51 +0000
+++ bzrlib/tests/__init__.py	2010-03-17 08:15:36 +0000
@@ -1519,6 +1519,7 @@
             'BZR_PROGRESS_BAR': None,
             'BZR_LOG': None,
             'BZR_PLUGIN_PATH': None,
+            'BZR_DISABLE_PLUGINS': None,
             'BZR_CONCURRENCY': None,
             # Make sure that any text ui tests are consistent regardless of
             # the environment the test case is run in; you may want tests that
 
=== modified file 'bzrlib/tests/test_plugins.py'
--- bzrlib/tests/test_plugins.py	2010-03-15 11:17:44 +0000
+++ bzrlib/tests/test_plugins.py	2010-03-17 08:15:36 +0000
@@ -31,6 +31,7 @@
     plugin,
     plugins,
     tests,
+    trace,
     )
 
 
@@ -38,6 +39,25 @@
 
 class TestPluginMixin(object):
 
+    def create_plugin(self, name, source='', dir='.', file_name=None):
+        if file_name is None:
+            file_name = name + '.py'
+        # 'source' must not fail to load
+        path = osutils.pathjoin(dir, file_name)
+        f = open(path, 'w')
+        self.addCleanup(os.unlink, path)
+        try:
+            f.write(source + '\n')
+        finally:
+            f.close()
+
+    def create_plugin_package(self, name, source='', dir='.'):
+        plugin_dir = osutils.pathjoin(dir, name)
+        os.mkdir(plugin_dir)
+        self.addCleanup(osutils.rmtree, plugin_dir)
+        self.create_plugin(name, source, dir=plugin_dir,
+                           file_name='__init__.py')
+
     def _unregister_plugin(self, name):
         """Remove the plugin from sys.modules and the bzrlib namespace."""
         py_name = 'bzrlib.plugins.%s' % name
@@ -47,11 +67,11 @@
             delattr(bzrlib.plugins, name)
 
     def assertPluginUnknown(self, name):
-        self.failIf(getattr(bzrlib.plugins, 'plugin', None) is not None)
+        self.failIf(getattr(bzrlib.plugins, name, None) is not None)
         self.failIf('bzrlib.plugins.%s' % name in sys.modules)
 
     def assertPluginKnown(self, name):
-        self.failUnless(getattr(bzrlib.plugins, 'plugin', None) is not None)
+        self.failUnless(getattr(bzrlib.plugins, name, None) is not None)
         self.failUnless('bzrlib.plugins.%s' % name in sys.modules)
 
 
@@ -723,3 +743,40 @@
         self.check_path(['+foo', '-bar', self.core, self.site],
                         ['+foo', '-bar'])
 
+
+class TestDisablePlugin(tests.TestCaseInTempDir, TestPluginMixin):
+
+    def setUp(self):
+        super(TestDisablePlugin, self).setUp()
+        self.create_plugin_package('test_foo')
+        # Make sure we don't pollute the plugins namespace
+        self.overrideAttr(plugins, '__path__')
+        # Be paranoid in case a test fail
+        self.addCleanup(self._unregister_plugin, 'test_foo')
+
+    def test_cannot_import(self):
+        osutils.set_or_unset_env('BZR_DISABLE_PLUGINS', 'test_foo')
+        plugin.set_plugins_path(['.'])
+        try:
+            import bzrlib.plugins.test_foo
+        except ImportError:
+            pass
+        self.assertPluginUnknown('test_foo')
+
+    def test_regular_load(self):
+        self.overrideAttr(plugin, '_loaded', False)
+        plugin.load_plugins(['.'])
+        self.assertPluginKnown('test_foo')
+
+    def test_not_loaded(self):
+        self.warnings = []
+        def captured_warning(*args, **kwargs):
+            self.warnings.append((args, kwargs))
+        self.overrideAttr(trace, 'warning', captured_warning)
+        self.overrideAttr(plugin, '_loaded', False)
+        osutils.set_or_unset_env('BZR_DISABLE_PLUGINS', 'test_foo')
+        plugin.load_plugins(plugin.set_plugins_path(['.']))
+        self.assertPluginUnknown('test_foo')
+        # Make sure we don't warn about the plugin ImportError since this has
+        # been *requested* by the user.
+        self.assertLength(0, self.warnings)