Bazaar

Merge lp:~jelmer/bzr/plugin-refactor into lp:~bzr/bzr/trunk-old

  1. plugin-refactor
  2. Merge into trunk-old
Proposed by Jelmer Vernooij
Status: Rejected
Rejected by: Jelmer Vernooij
Proposed branch: lp:~jelmer/bzr/plugin-refactor
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 157 lines
To merge this branch: bzr merge lp:~jelmer/bzr/plugin-refactor
Reviewer Review Type Date Requested Status
Jelmer Vernooij (community) Abstain
Robert Collins (community) Disapprove
Review via email: mp+9005@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

The attached patch refactors the plugin loading code a bit so we
remember the suffix tuple when finding plugins so we can use it for
loading them later. This saves a few stats, and will make it possible
to load individual plugins in the future (this is what I hope to work
on next).

The next step (and my next patch hopefully) will be supporting something like
"bzr --with-plugin /foo/bar/bla".

Revision history for this message
Robert Collins (lifeless) wrote :

On Sat, 2009-07-18 at 21:15 +0000, Jelmer Vernooij wrote:
> Jelmer Vernooij has proposed merging lp:~jelmer/bzr/plugin-refactor into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> The attached patch refactors the plugin loading code a bit so we
> remember the suffix tuple when finding plugins so we can use it for
> loading them later. This saves a few stats, and will make it possible
> to load individual plugins in the future (this is what I hope to work
> on next).

I think this may be buggy/introduce bugs. You change the load logic
from:
calculate the plugin path
calculate names to load
import them all

to
calculate the plugin path
calculate names-in-directories-to-load
import-those-names

You also catch an ImportError and squelch it somewhere where it looks
like we'd want to know that the error occurred.

The patch is also going away from using 'import' to using
'imp.find_module' + 'imp.load_module' - we had bugs in the past
*because* we used those rather than using the main python import code
path.

This doesn't seem like a good conceptual idea to me. I don't want to
stop you achieving what you need to, but - why can't you just put the
plugin you want on the plugins path.

 review -1

review: Disapprove
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

I'll put this in a plugin instead.

review: Abstain

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzrlib/plugin.py'
--- bzrlib/plugin.py 2009-03-24 01:53:42 +0000
+++ bzrlib/plugin.py 2009-07-19 02:36:01 +0000
@@ -187,70 +187,103 @@
187load_from_dirs = load_from_path187load_from_dirs = load_from_path
188188
189189
190def load_by_suffix_info(name, path, suffix_info):
191 """Load a plugin given its path and suffix info.
192
193 :param name: Short name of the plugin (e.g. search)
194 :param path: Path to the module
195 :param suffix_info: Suffix info tuple as used by imp.load_module
196 :return: Module object for the plugin
197 """
198 if re.search('\.|-| ', name):
199 sanitised_name = re.sub('[-. ]', '_', name)
200 if sanitised_name.startswith('bzr_'):
201 sanitised_name = sanitised_name[len('bzr_'):]
202 trace.warning("Unable to load %r in %r as a plugin because the "
203 "file path isn't a valid module name; try renaming "
204 "it to %r.", name, os.path.dirname(path), sanitised_name)
205 return
206 try:
207 if suffix_info[1] != '':
208 # Directory
209 file = open(path, suffix_info[1])
210 else:
211 file = None
212 plugin = imp.load_module("bzrlib.plugins.%s" % name, file, path,
213 suffix_info)
214 setattr(_mod_plugins, name, plugin)
215 return plugin
216 except KeyboardInterrupt:
217 raise
218 except errors.IncompatibleAPI, e:
219 trace.warning("Unable to load plugin %r. It requested API version "
220 "%s of module %s but the minimum exported version is %s, and "
221 "the maximum is %s" %
222 (name, e.wanted, e.api, e.minimum, e.current))
223 except Exception, e:
224 trace.warning("%s" % e)
225 ## import pdb; pdb.set_trace()
226 trace.warning('Unable to load plugin %r from %r',
227 name, os.path.dirname(path))
228 trace.log_exception_quietly()
229 if 'error' in debug.debug_flags:
230 trace.print_exception(sys.exc_info(), sys.stderr)
231
232
233def find_module(file, path):
234 """Find the suffix information for a plugin.
235
236 :param file: Short name of the plugin
237 :param path: Path to the plugin
238 :return: Tuple with short name, path and suffix_info
239 (as used by imp.load_module)
240 """
241 # Get the list of valid python suffixes for __init__.py?
242 # this includes .py, .pyc, and .pyo (depending on if we are running -O)
243 # but it doesn't include compiled modules (.so, .dll, etc)
244 if os.path.isdir(path):
245 for suffix, mod_type, flags in imp.get_suffixes():
246 if flags not in (imp.PY_SOURCE, imp.PY_COMPILED):
247 continue
248 entry = '__init__'+suffix
249 # This directory should be a package, and thus added to
250 # the list
251 entry_path = osutils.pathjoin(path, entry)
252 if os.path.isfile(entry_path):
253 return file, path, ('', '', imp.PKG_DIRECTORY)
254 raise ImportError("plugin %s is not a package" % path)
255 else:
256 for suffix_info in imp.get_suffixes():
257 if file.endswith(suffix_info[0]):
258 file = file[:-len(suffix_info[0])]
259 if suffix_info[2] == imp.C_EXTENSION and f.endswith('module'):
260 file = file[:-len('module')]
261 return file, path, suffix_info
262 raise ImportError("plugin %s is not a module (no known Python suffix)" % path)
263
264
190def load_from_dir(d):265def load_from_dir(d):
191 """Load the plugins in directory d.266 """Load the plugins in directory d.
192267
193 d must be in the plugins module path already.268 d must be in the plugins module path already.
194 """269 """
195 # Get the list of valid python suffixes for __init__.py?270 plugins = {}
196 # this includes .py, .pyc, and .pyo (depending on if we are running -O)
197 # but it doesn't include compiled modules (.so, .dll, etc)
198 valid_suffixes = [suffix for suffix, mod_type, flags in imp.get_suffixes()
199 if flags in (imp.PY_SOURCE, imp.PY_COMPILED)]
200 package_entries = ['__init__'+suffix for suffix in valid_suffixes]
201 plugin_names = set()
202 for f in os.listdir(d):271 for f in os.listdir(d):
203 path = osutils.pathjoin(d, f)272 path = osutils.pathjoin(d, f)
204 if os.path.isdir(path):273 try:
205 for entry in package_entries:274 f, path, suffix_info = find_module(f, path)
206 # This directory should be a package, and thus added to275 except ImportError:
207 # the list276 continue
208 if os.path.isfile(osutils.pathjoin(path, entry)):
209 break
210 else: # This directory is not a package
211 continue
212 else:
213 for suffix_info in imp.get_suffixes():
214 if f.endswith(suffix_info[0]):
215 f = f[:-len(suffix_info[0])]
216 if suffix_info[2] == imp.C_EXTENSION and f.endswith('module'):
217 f = f[:-len('module')]
218 break
219 else:
220 continue
221 if f == '__init__':277 if f == '__init__':
222 continue # We don't load __init__.py again in the plugin dir278 continue # We don't load __init__.py again in the plugin dir
223 elif getattr(_mod_plugins, f, None):279 elif getattr(_mod_plugins, f, None):
224 trace.mutter('Plugin name %s already loaded', f)280 trace.mutter('Plugin name %s already loaded', f)
225 else:281 else:
226 # trace.mutter('add plugin name %s', f)282 # trace.mutter('add plugin name %s', f)
227 plugin_names.add(f)283 plugins[f] = (path, suffix_info)
228284
229 for name in plugin_names:285 for name, (path, suffix_info) in plugins.iteritems():
230 try:286 load_by_suffix_info(name, path, suffix_info)
231 exec "import bzrlib.plugins.%s" % name in {}
232 except KeyboardInterrupt:
233 raise
234 except errors.IncompatibleAPI, e:
235 trace.warning("Unable to load plugin %r. It requested API version "
236 "%s of module %s but the minimum exported version is %s, and "
237 "the maximum is %s" %
238 (name, e.wanted, e.api, e.minimum, e.current))
239 except Exception, e:
240 trace.warning("%s" % e)
241 ## import pdb; pdb.set_trace()
242 if re.search('\.|-| ', name):
243 sanitised_name = re.sub('[-. ]', '_', name)
244 if sanitised_name.startswith('bzr_'):
245 sanitised_name = sanitised_name[len('bzr_'):]
246 trace.warning("Unable to load %r in %r as a plugin because the "
247 "file path isn't a valid module name; try renaming "
248 "it to %r." % (name, d, sanitised_name))
249 else:
250 trace.warning('Unable to load plugin %r from %r' % (name, d))
251 trace.log_exception_quietly()
252 if 'error' in debug.debug_flags:
253 trace.print_exception(sys.exc_info(), sys.stderr)
254287
255288
256def plugins():289def plugins():