Bazaar

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

  1. 411413-disable-plugin
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
Status: Merged
Merged at revision: not available
Proposed branch: lp:~vila/bzr/411413-disable-plugin
Merge into: lp:bzr
Diff against target: 212 lines (+107/-5)
5 files modified
NEWS (+4/-0)
bzrlib/help_topics/en/configuration.txt (+16/-0)
bzrlib/plugin.py (+27/-3)
bzrlib/tests/__init__.py (+1/-0)
bzrlib/tests/test_plugins.py (+59/-2)
To merge this branch: bzr merge lp:~vila/bzr/411413-disable-plugin
Reviewer Review Type Date Requested Status
Martin Pool Needs Fixing
Review via email: mp+21435@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Matthew Fuller (fullermd) wrote :

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

That seems... unclean.

Revision history for this message
Vincent Ladeuil (vila) wrote :

> > [...] 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).

Revision history for this message
Martin Pool (mbp) wrote :

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/>

Revision history for this message
Martin Pool (mbp) :
review: Needs Fixing
Revision history for this message
Matthew Fuller (fullermd) wrote :

> 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.

Revision history for this message
Vincent Ladeuil (vila) wrote :

> BZR_DISABLE_PLUGINS=svn:git:qbzr

Bah, of course, fixed.

Revision history for this message
Martin Pool (mbp) wrote :

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
Revision history for this message
Vincent Ladeuil (vila) wrote :

> 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).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-03-12 19:59:50 +0000
3+++ NEWS 2010-03-17 08:15:36 +0000
4@@ -51,6 +51,10 @@
5 treats backslash as an escape character on Windows. (Gordon Tyler,
6 #392248)
7
8+* Plugins can be disabled by defining ``BZR_DISABLE_PLUGINS`` as
9+ a list of plugin names separated by ':' (';' on windows).
10+ (Vincent Ladeuil, #411413)
11+
12 * Tree-shape conflicts can be resolved by providing ``--take-this`` and
13 ``--take-other`` to the ``bzr resolve`` command. Just marking the conflict
14 as resolved is still accessible via the ``--done`` default action.
15
16=== modified file 'bzrlib/help_topics/en/configuration.txt'
17--- bzrlib/help_topics/en/configuration.txt 2010-01-03 03:33:10 +0000
18+++ bzrlib/help_topics/en/configuration.txt 2010-03-17 08:15:36 +0000
19@@ -116,6 +116,22 @@
20 Overriding the default site plugin directory:
21 ``BZR_PLUGIN_PATH='/path/to/my/site/plugins:-site':+user``
22
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+
37+Disabling ``myplugin`` and ``yourplugin``:
38+``BZR_DISABLE_PLUGINS='myplugin:yourplugin'``
39
40
41 BZRPATH
42
43=== modified file 'bzrlib/plugin.py'
44--- bzrlib/plugin.py 2010-03-12 13:41:08 +0000
45+++ bzrlib/plugin.py 2010-03-17 08:15:36 +0000
46@@ -91,6 +91,12 @@
47 if path is None:
48 path = get_standard_plugins_path()
49 _mod_plugins.__path__ = path
50+ # Set up a blacklist for disabled plugins if any
51+ PluginBlackListImporter.blacklist = {}
52+ disabled_plugins = os.environ.get('BZR_DISABLE_PLUGINS', None)
53+ if disabled_plugins is not None:
54+ for name in disabled_plugins.split(os.pathsep):
55+ PluginBlackListImporter.blacklist['bzrlib.plugins.' + name] = True
56 return path
57
58
59@@ -183,8 +189,8 @@
60 try:
61 p = refs[p[1:]]
62 except KeyError:
63- # Leave them untouched otherwise, user may have paths starting
64- # with '+'...
65+ # Leave them untouched so user can still use paths starting
66+ # with '+'
67 pass
68 _append_new_path(paths, p)
69
70@@ -202,7 +208,7 @@
71 files (and whatever other extensions are used in the platform,
72 such as *.pyd).
73
74- load_from_dirs() provides the underlying mechanism and is called with
75+ load_from_path() provides the underlying mechanism and is called with
76 the default directory list to provide the normal behaviour.
77
78 :param path: The list of paths to search for plugins. By default,
79@@ -290,6 +296,8 @@
80 plugin_names.add(f)
81
82 for name in plugin_names:
83+ if ('bzrlib.plugins.%s' % name) in PluginBlackListImporter.blacklist:
84+ continue
85 try:
86 exec "import bzrlib.plugins.%s" % name in {}
87 except KeyboardInterrupt:
88@@ -476,3 +484,19 @@
89 return version_string
90
91 __version__ = property(_get__version__)
92+
93+
94+class _PluginBlackListImporter(object):
95+
96+ def __init__(self):
97+ self.blacklist = {}
98+
99+ def find_module(self, fullname, parent_path=None):
100+ if fullname in self.blacklist:
101+ raise ImportError('%s is disabled' % fullname)
102+ return None
103+
104+PluginBlackListImporter = _PluginBlackListImporter()
105+sys.meta_path.append(PluginBlackListImporter)
106+
107+
108
109=== modified file 'bzrlib/tests/__init__.py'
110--- bzrlib/tests/__init__.py 2010-03-10 06:02:51 +0000
111+++ bzrlib/tests/__init__.py 2010-03-17 08:15:36 +0000
112@@ -1519,6 +1519,7 @@
113 'BZR_PROGRESS_BAR': None,
114 'BZR_LOG': None,
115 'BZR_PLUGIN_PATH': None,
116+ 'BZR_DISABLE_PLUGINS': None,
117 'BZR_CONCURRENCY': None,
118 # Make sure that any text ui tests are consistent regardless of
119 # the environment the test case is run in; you may want tests that
120
121=== modified file 'bzrlib/tests/test_plugins.py'
122--- bzrlib/tests/test_plugins.py 2010-03-15 11:17:44 +0000
123+++ bzrlib/tests/test_plugins.py 2010-03-17 08:15:36 +0000
124@@ -31,6 +31,7 @@
125 plugin,
126 plugins,
127 tests,
128+ trace,
129 )
130
131
132@@ -38,6 +39,25 @@
133
134 class TestPluginMixin(object):
135
136+ def create_plugin(self, name, source='', dir='.', file_name=None):
137+ if file_name is None:
138+ file_name = name + '.py'
139+ # 'source' must not fail to load
140+ path = osutils.pathjoin(dir, file_name)
141+ f = open(path, 'w')
142+ self.addCleanup(os.unlink, path)
143+ try:
144+ f.write(source + '\n')
145+ finally:
146+ f.close()
147+
148+ def create_plugin_package(self, name, source='', dir='.'):
149+ plugin_dir = osutils.pathjoin(dir, name)
150+ os.mkdir(plugin_dir)
151+ self.addCleanup(osutils.rmtree, plugin_dir)
152+ self.create_plugin(name, source, dir=plugin_dir,
153+ file_name='__init__.py')
154+
155 def _unregister_plugin(self, name):
156 """Remove the plugin from sys.modules and the bzrlib namespace."""
157 py_name = 'bzrlib.plugins.%s' % name
158@@ -47,11 +67,11 @@
159 delattr(bzrlib.plugins, name)
160
161 def assertPluginUnknown(self, name):
162- self.failIf(getattr(bzrlib.plugins, 'plugin', None) is not None)
163+ self.failIf(getattr(bzrlib.plugins, name, None) is not None)
164 self.failIf('bzrlib.plugins.%s' % name in sys.modules)
165
166 def assertPluginKnown(self, name):
167- self.failUnless(getattr(bzrlib.plugins, 'plugin', None) is not None)
168+ self.failUnless(getattr(bzrlib.plugins, name, None) is not None)
169 self.failUnless('bzrlib.plugins.%s' % name in sys.modules)
170
171
172@@ -723,3 +743,40 @@
173 self.check_path(['+foo', '-bar', self.core, self.site],
174 ['+foo', '-bar'])
175
176+
177+class TestDisablePlugin(tests.TestCaseInTempDir, TestPluginMixin):
178+
179+ def setUp(self):
180+ super(TestDisablePlugin, self).setUp()
181+ self.create_plugin_package('test_foo')
182+ # Make sure we don't pollute the plugins namespace
183+ self.overrideAttr(plugins, '__path__')
184+ # Be paranoid in case a test fail
185+ self.addCleanup(self._unregister_plugin, 'test_foo')
186+
187+ def test_cannot_import(self):
188+ osutils.set_or_unset_env('BZR_DISABLE_PLUGINS', 'test_foo')
189+ plugin.set_plugins_path(['.'])
190+ try:
191+ import bzrlib.plugins.test_foo
192+ except ImportError:
193+ pass
194+ self.assertPluginUnknown('test_foo')
195+
196+ def test_regular_load(self):
197+ self.overrideAttr(plugin, '_loaded', False)
198+ plugin.load_plugins(['.'])
199+ self.assertPluginKnown('test_foo')
200+
201+ def test_not_loaded(self):
202+ self.warnings = []
203+ def captured_warning(*args, **kwargs):
204+ self.warnings.append((args, kwargs))
205+ self.overrideAttr(trace, 'warning', captured_warning)
206+ self.overrideAttr(plugin, '_loaded', False)
207+ osutils.set_or_unset_env('BZR_DISABLE_PLUGINS', 'test_foo')
208+ plugin.load_plugins(plugin.set_plugins_path(['.']))
209+ self.assertPluginUnknown('test_foo')
210+ # Make sure we don't warn about the plugin ImportError since this has
211+ # been *requested* by the user.
212+ self.assertLength(0, self.warnings)