Bazaar

Merge lp:~vila/bzr/552922-plugins-at into lp:bzr

  1. 552922-plugins-at
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
Status: Merged
Approved by: Ian Clatworthy
Approved revision: no longer in the source branch.
Merged at revision: 5134
Proposed branch: lp:~vila/bzr/552922-plugins-at
Merge into: lp:bzr
Diff against target: 181 lines (+68/-35)
3 files modified
NEWS (+4/-0)
bzrlib/plugin.py (+27/-25)
bzrlib/tests/test_plugins.py (+37/-10)
To merge this branch: bzr merge lp:~vila/bzr/552922-plugins-at
Reviewer Review Type Date Requested Status
Ian Clatworthy Approve
Gordon Tyler Approve
Review via email: mp+22697@code.launchpad.net

Description of the change

Since https://code.edge.launchpad.net/~vila/bzr/552922-plugins-at/+merge/22694
was broken, I'm proposing against bzr.dev to get a reasonable diff,
but the intent is still to land in 2.2 when it will be updated.

This patch fixes the BZR_PLUGINS_AT handling to not depend on os.listdir() order.
The automated tests couldn't catch the problem and I was lucky enough to not encounter
it before (and neither was Gary).

Unlike _find_module_plugin() which scan the 'plugins' directories and deduce the plugin
names from the loadable files, BZR_PLUGINS_AT *knows* the plugin names and can (and should)
try some file names explicitly.

Tests added.

To post a comment you must log in.
Revision history for this message
Gordon Tyler (doxxx) wrote :

This fixes the problem I was having in bug 552922.

review: Approve
Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote :

Some requested tweaks ...

> name = fullname[len('bzrlib.plugins.'):]

This variable is never used. Perhaps you wanted to use it in the ImportError? Otherwise, just delete this line please.

> def test_loading_from___init__(self):

Given this test doesn't actual load from __init__ but fails because that file is not there (after an explicit rename), I'd prefer a better test name, e.g. test_loading_from___init__only() say.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-04-06 06:59:03 +0000
3+++ NEWS 2010-04-06 07:24:38 +0000
4@@ -205,6 +205,10 @@
5 deletion is involved.
6 (Vincent Ladeuil, #531967)
7
8+* Loading a plugin from a given path with ``BZR_PLUGINS_AT`` doesn't depend
9+ on os.lisdir() order and is now reliable.
10+ (Vincent Ladeuil, #552922).
11+
12 * Network transfer amounts and rates are now displayed in SI units according
13 to the Ubuntu Units Policy <https://wiki.ubuntu.com/UnitsPolicy>.
14 (Gordon Tyler, #514399)
15
16=== modified file 'bzrlib/plugin.py'
17--- bzrlib/plugin.py 2010-03-24 13:57:35 +0000
18+++ bzrlib/plugin.py 2010-04-06 07:24:38 +0000
19@@ -303,7 +303,7 @@
20
21
22 def _load_plugin_module(name, dir):
23- """Load plugine name from dir.
24+ """Load plugin name from dir.
25
26 :param name: The plugin name in the bzrlib.plugins namespace.
27 :param dir: The directory the plugin is loaded from for error messages.
28@@ -560,32 +560,34 @@
29 def load_module(self, fullname):
30 """Load a plugin from a specific directory."""
31 # We are called only for specific paths
32- plugin_dir = self.specific_paths[fullname]
33- candidate = None
34- maybe_package = False
35- for p in os.listdir(plugin_dir):
36- if os.path.isdir(osutils.pathjoin(plugin_dir, p)):
37- # We're searching for files only and don't want submodules to
38- # be recognized as plugins (they are submodules inside the
39- # plugin).
40- continue
41- name, path, (
42- suffix, mode, kind) = _find_plugin_module(plugin_dir, p)
43- if name is not None:
44- candidate = (name, path, suffix, mode, kind)
45- if kind == imp.PY_SOURCE:
46- # We favour imp.PY_SOURCE (which will use the compiled
47- # version if available) over imp.PY_COMPILED (which is used
48- # only if the source is not available)
49- break
50- if candidate is None:
51+ plugin_path = self.specific_paths[fullname]
52+ loading_path = None
53+ package = False
54+ if os.path.isdir(plugin_path):
55+ for suffix, mode, kind in imp.get_suffixes():
56+ if kind not in (imp.PY_SOURCE, imp.PY_COMPILED):
57+ # We don't recognize compiled modules (.so, .dll, etc)
58+ continue
59+ init_path = osutils.pathjoin(plugin_path, '__init__' + suffix)
60+ if os.path.isfile(init_path):
61+ loading_path = init_path
62+ package = True
63+ break
64+ else:
65+ for suffix, mode, kind in imp.get_suffixes():
66+ if plugin_path.endswith(suffix):
67+ loading_path = plugin_path
68+ break
69+ if loading_path is None:
70 raise ImportError('%s cannot be loaded from %s'
71- % (fullname, plugin_dir))
72- f = open(path, mode)
73+ % (fullname, plugin_path))
74+ f = open(loading_path, mode)
75 try:
76- mod = imp.load_module(fullname, f, path, (suffix, mode, kind))
77- # The plugin can contain modules, so be ready
78- mod.__path__ = [plugin_dir]
79+ mod = imp.load_module(fullname, f, loading_path,
80+ (suffix, mode, kind))
81+ if package:
82+ # The plugin can contain modules, so be ready
83+ mod.__path__ = [plugin_path]
84 mod.__package__ = fullname
85 return mod
86 finally:
87
88=== modified file 'bzrlib/tests/test_plugins.py'
89--- bzrlib/tests/test_plugins.py 2010-03-24 11:55:49 +0000
90+++ bzrlib/tests/test_plugins.py 2010-04-06 07:24:38 +0000
91@@ -810,21 +810,23 @@
92 self.overrideAttr(plugin, '_loaded', False)
93 # Create the same plugin in two directories
94 self.create_plugin_package('test_foo', dir='non-standard-dir')
95- self.create_plugin_package('test_foo', dir='b/test_foo')
96+ # The "normal" directory, we use 'standard' instead of 'plugins' to
97+ # avoid depending on the precise naming.
98+ self.create_plugin_package('test_foo', dir='standard/test_foo')
99
100- def assertTestFooLoadedFrom(self, dir):
101+ def assertTestFooLoadedFrom(self, path):
102 self.assertPluginKnown('test_foo')
103 self.assertEqual('This is the doc for test_foo',
104 bzrlib.plugins.test_foo.__doc__)
105- self.assertEqual(dir, bzrlib.plugins.test_foo.dir_source)
106+ self.assertEqual(path, bzrlib.plugins.test_foo.dir_source)
107
108 def test_regular_load(self):
109- plugin.load_plugins(['b'])
110- self.assertTestFooLoadedFrom('b/test_foo')
111+ plugin.load_plugins(['standard'])
112+ self.assertTestFooLoadedFrom('standard/test_foo')
113
114 def test_import(self):
115 osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
116- plugin.set_plugins_path(['b'])
117+ plugin.set_plugins_path(['standard'])
118 try:
119 import bzrlib.plugins.test_foo
120 except ImportError:
121@@ -833,12 +835,12 @@
122
123 def test_loading(self):
124 osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
125- plugin.load_plugins(['b'])
126+ plugin.load_plugins(['standard'])
127 self.assertTestFooLoadedFrom('non-standard-dir')
128
129 def test_compiled_loaded(self):
130 osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
131- plugin.load_plugins(['b'])
132+ plugin.load_plugins(['standard'])
133 self.assertTestFooLoadedFrom('non-standard-dir')
134 self.assertEqual('non-standard-dir/__init__.py',
135 bzrlib.plugins.test_foo.__file__)
136@@ -846,7 +848,7 @@
137 # Try importing again now that the source has been compiled
138 self._unregister_plugin('test_foo')
139 plugin._loaded = False
140- plugin.load_plugins(['b'])
141+ plugin.load_plugins(['standard'])
142 self.assertTestFooLoadedFrom('non-standard-dir')
143 if __debug__:
144 suffix = 'pyc'
145@@ -859,10 +861,35 @@
146 # We create an additional directory under the one for test_foo
147 self.create_plugin_package('test_bar', dir='non-standard-dir/test_bar')
148 osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
149- plugin.set_plugins_path(['b'])
150+ plugin.set_plugins_path(['standard'])
151 import bzrlib.plugins.test_foo
152 self.assertEqual('bzrlib.plugins.test_foo',
153 bzrlib.plugins.test_foo.__package__)
154 import bzrlib.plugins.test_foo.test_bar
155 self.assertEqual('non-standard-dir/test_bar/__init__.py',
156 bzrlib.plugins.test_foo.test_bar.__file__)
157+
158+ def test_loading_from___init__only(self):
159+ # We rename the existing __init__.py file to ensure that we don't load
160+ # a random file
161+ init = 'non-standard-dir/__init__.py'
162+ random = 'non-standard-dir/setup.py'
163+ os.rename(init, random)
164+ self.addCleanup(os.rename, random, init)
165+ osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
166+ plugin.load_plugins(['standard'])
167+ self.assertPluginUnknown('test_foo')
168+
169+ def test_loading_from_specific_file(self):
170+ plugin_dir = 'non-standard-dir'
171+ plugin_file_name = 'iamtestfoo.py'
172+ plugin_path = osutils.pathjoin(plugin_dir, plugin_file_name)
173+ source = '''\
174+"""This is the doc for %s"""
175+dir_source = '%s'
176+''' % ('test_foo', plugin_path)
177+ self.create_plugin('test_foo', source=source,
178+ dir=plugin_dir, file_name=plugin_file_name)
179+ osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@%s' % plugin_path)
180+ plugin.load_plugins(['standard'])
181+ self.assertTestFooLoadedFrom(plugin_path)